1092544 - IonMonkey: Assert that recover instruction are used.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

Currently, optimizations made with Bug 1003801 are only eye balled with IonGraph, we should copy the way we are doing assertFloat32 and replicate a similar system such as we can assert that we expect recover instructions.

The result should look something like (see js/src/jit-test/tests/ion/dce-with-rinstructions.js):

var uceFault_add_number = eval(uneval(uceFault).replace('uceFault', 'uceFault_add_number'));
function radd_number(i) {
    var x = 1 + i;
    if (uceFault_add_number(i) || uceFault_add_number(i))
        assertEq(x, 100  /* = 1 + 99 */);
    assertRecoveredOnBailout(x);
    return i;
}

The function should be added and handled almost as we do for assertFloat32:
http://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&redirect=true&q=assertFloat32%20ext%3Acpp

We should assert that if there is no uses, by any definitions, then the instruction is not discarded and marked as recovered on bailouts.  As opposed to assertFloat32, the MIR instruction should not use the recover as part of its operands, otherwise it would prevent optimizations made by DCE.

Comment 1

[Quentin Pradet [:quentinp]]

I've started working on this bug during the bug squashing party, and will try to fix it in the next few weeks. Thanks for the help so far!

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Quentin Pradet [:quentinp] from comment #1)
> I've started working on this bug during the bug squashing party, and will
> try to fix it in the next few weeks. Thanks for the help so far!

I am assigning you on this bug, as I saw that you did some work on it ;)

Comment 3

[Quentin Pradet [:quentinp]]

Attachment: [WIP] Assert that instructions are recovered on bailouts

This patch is a WIP to fix the bug. I currently have an issue in MIR.h that I don't know how to solve.

You explained that the MIR instruction should not use the the "recover" as part of its operand to avoid optimization issues. My guess is that it means that the variable x in assertRecoveredOnBailout(x) should not be an operand of the MAssertRecoveredOnBailout type. Instead, I tried just added the RecoveredOnBailout boolean flag. Since the MIR.h classes look rather dumb, I've chosen to do that in MCallOptimize.cpp.

I've tried to use MNullaryInstruction for this since there's no MDefinition* involved, but this doesn't compile with the following error:

In file included from /home/quentin/Projets/gecko-dev/js/src/build_DBG.OBJ/js/src/Unified_cpp_js_src5.cpp:56:0:
/home/quentin/Projets/gecko-dev/js/src/jit/ParallelSafetyAnalysis.cpp: In member function 'bool js::jit::ParallelSafetyAnalysis::analyze()':
/home/quentin/Projets/gecko-dev/js/src/jit/ParallelSafetyAnalysis.cpp:386:27: error: cannot declare variable 'visitor' to be of abstract type 'ParallelSafetyVisitor'
     ParallelSafetyVisitor visitor(graph_);
                           ^
/home/quentin/Projets/gecko-dev/js/src/jit/ParallelSafetyAnalysis.cpp:67:7: note:   because the following virtual functions are pure within 'ParallelSafetyVisitor':
 class ParallelSafetyVisitor : public MDefinitionVisitor
       ^
In file included from /home/quentin/Projets/gecko-dev/js/src/jit/MIR.h:23:0,
                 from /home/quentin/Projets/gecko-dev/js/src/jit/LIR.h:19,
                 from /home/quentin/Projets/gecko-dev/js/src/jit/Lowering.h:13,
                 from /home/quentin/Projets/gecko-dev/js/src/jit/Lowering.cpp:7,
                 from /home/quentin/Projets/gecko-dev/js/src/build_DBG.OBJ/js/src/Unified_cpp_js_src5.cpp:2:
/home/quentin/Projets/gecko-dev/js/src/jit/MOpcodes.h:275:36: note: 	virtual bool js::jit::MDefinitionVisitor::visitAssertRecoveredOnBailout(js::jit::MAssertRecoveredOnBailout*)
 #define VISIT_INS(op) virtual bool visit##op(M##op *) = 0;
                                    ^
/home/quentin/Projets/gecko-dev/js/src/jit/MOpcodes.h:66:5: note: in expansion of macro 'VISIT_INS'
     _(AssertRecoveredOnBailout)                                             \
     ^
/home/quentin/Projets/gecko-dev/js/src/jit/MOpcodes.h:276:5: note: in expansion of macro 'MIR_OPCODE_LIST'
     MIR_OPCODE_LIST(VISIT_INS)
     ^

Any pointers? Thanks!

(I've used review? instead of needinfo, not sure what the right flag is for such a non-working patch.)

Comment 4

[Quentin Pradet [:quentinp]]

Comment on attachment 8519481 [details] [diff] [review]
[WIP] Assert that instructions are recovered on bailouts

Try to switch from review? to feedback?.

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Quentin Pradet [:quentinp] from comment #3)
> Created attachment 8519481 [details] [diff] [review]
> [WIP] Assert that instructions are recovered on bailouts
> 
> This patch is a WIP to fix the bug. I currently have an issue in MIR.h that
> I don't know how to solve.
> 
> You explained that the MIR instruction should not use the the "recover" as
> part of its operand to avoid optimization issues. My guess is that it means
> that the variable x in assertRecoveredOnBailout(x) should not be an operand
> of the MAssertRecoveredOnBailout type.

Right, having the instruction as an operand of the MIR Instruction implies that we would have a use of the instruction.  And having a use of the instruction will prevent the optimization which are flagging the instruction as RecoveredOnBailout to kick in.

> Instead, I tried just added the
> RecoveredOnBailout boolean flag. Since the MIR.h classes look rather dumb,
> I've chosen to do that in MCallOptimize.cpp.

Unfortunately MCallOptimize happens too soon in the Pipeline to capute the modification of this flag, so even if this was compiling this would not work because the flag is not yet modified.

> I've tried to use MNullaryInstruction for this since there's no MDefinition*
> involved, but this doesn't compile with the following error:
> 
> In file included from
> /home/quentin/Projets/gecko-dev/js/src/build_DBG.OBJ/js/src/
> Unified_cpp_js_src5.cpp:56:0:
> /home/quentin/Projets/gecko-dev/js/src/jit/ParallelSafetyAnalysis.cpp: In
> member function 'bool js::jit::ParallelSafetyAnalysis::analyze()':
> /home/quentin/Projets/gecko-dev/js/src/jit/ParallelSafetyAnalysis.cpp:386:27:
> error: cannot declare variable 'visitor' to be of abstract type
> 'ParallelSafetyVisitor'
>      ParallelSafetyVisitor visitor(graph_);
>                            ^
> /home/quentin/Projets/gecko-dev/js/src/jit/ParallelSafetyAnalysis.cpp:67:7:
> note:   because the following virtual functions are pure within
> 'ParallelSafetyVisitor':
>  class ParallelSafetyVisitor : public MDefinitionVisitor
>        ^
> In file included from /home/quentin/Projets/gecko-dev/js/src/jit/MIR.h:23:0,
>                  from /home/quentin/Projets/gecko-dev/js/src/jit/LIR.h:19,
>                  from
> /home/quentin/Projets/gecko-dev/js/src/jit/Lowering.h:13,
>                  from
> /home/quentin/Projets/gecko-dev/js/src/jit/Lowering.cpp:7,
>                  from
> /home/quentin/Projets/gecko-dev/js/src/build_DBG.OBJ/js/src/
> Unified_cpp_js_src5.cpp:2:
> /home/quentin/Projets/gecko-dev/js/src/jit/MOpcodes.h:275:36: note: 	virtual
> bool
> js::jit::MDefinitionVisitor::visitAssertRecoveredOnBailout(js::jit::
> MAssertRecoveredOnBailout*)
>  #define VISIT_INS(op) virtual bool visit##op(M##op *) = 0;
>                                     ^
> /home/quentin/Projets/gecko-dev/js/src/jit/MOpcodes.h:66:5: note: in
> expansion of macro 'VISIT_INS'
>      _(AssertRecoveredOnBailout)                                            
> \
>      ^
> /home/quentin/Projets/gecko-dev/js/src/jit/MOpcodes.h:276:5: note: in
> expansion of macro 'MIR_OPCODE_LIST'
>      MIR_OPCODE_LIST(VISIT_INS)
>      ^
> 
> Any pointers? Thanks!

This error is about the fact that there is an file which require you to flag the instruction as Safe for ParallelJS.  Not all instructions are safe to be executed in a parallel execution of JavaScript. Have a look at js/src/jit/ParallelSafetyAnalysis.cpp .

> (I've used review? instead of needinfo, not sure what the right flag is for
> such a non-working patch.)

feedback? is good ;)

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8519481 [details] [diff] [review]
[WIP] Assert that instructions are recovered on bailouts

Review of attachment 8519481 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/MCallOptimize.cpp
@@ +2220,5 @@
> +IonBuilder::inlineAssertRecoveredOnBailout(CallInfo &callInfo)
> +{
> +    callInfo.setImplicitlyUsedUnchecked();
> +
> +    current->add(MAssertRecoveredOnBailout::New(alloc(), callInfo.getArg(0)->isRecoveredOnBailout()));

getArg(0) is unlikely to be recovered on Bailout at the time where this instruction is created.  Thus you should probably keep the pointer to the MDefinition* returned by getArg(0).

::: js/src/jit/MIR.h
@@ +3477,5 @@
> +    static MAssertRecoveredOnBailout *New(TempAllocator &alloc, bool isRecoveredOnBailout) {
> +        return new(alloc) MAssertRecoveredOnBailout(isRecoveredOnBailout);
> +    }
> +
> +    bool isRecoveredOnBailout() const { return isRecoveredOnBailout_; }

Another name might be better, as we are referring to the MDefinition* given as argument of this assertion.

Comment 7

[Quentin Pradet [:quentinp]]

Attachment: 0001-WIP-2-Assert-that-instructions-are-recovered-on-bail.patch

Thank you for the feedback. I tried to address all of it.

I'm now checking the RecoveredOnBailout flag in LIRGenerator::visitAssertRecoveredOnBailout. To do this, I did keep the MDefinition* in MAssertRecoveredOnBailout. Does that count as "keeping it as an operand"? Or is MUnaryInstruction what we want to avoid here?

It compiles! Woaw, progress. However, the assertion fails.

∮ IONFLAGS=logs ./build_DBG.OBJ/dist/bin/js --ion-offthread-compile=off --ion-eager jit-test/tests/ion/dce-with-rinstructions.js
Assertion failure: checkIsRecoveredOnBailout, at /home/quentin/Projets/gecko-dev/js/src/jit/Lowering.cpp:545
zsh: segmentation fault (core dumped)  IONFLAGS=logs ./build_DBG.OBJ/dist/bin/js --ion-offthread-compile=off

(I'm only using the invocation from bug 1003801#c4 here, and don't really know what I'm doing.)

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Quentin Pradet [:quentinp] from comment #7)
> I'm now checking the RecoveredOnBailout flag in
> LIRGenerator::visitAssertRecoveredOnBailout. To do this, I did keep the
> MDefinition* in MAssertRecoveredOnBailout. Does that count as "keeping it as
> an operand"? Or is MUnaryInstruction what we want to avoid here?

We want to avoid MUnaryInstruction, and to have numOperands returns 0.

> Assertion failure: checkIsRecoveredOnBailout, at
> /home/quentin/Projets/gecko-dev/js/src/jit/Lowering.cpp:545
> zsh: segmentation fault (core dumped)  IONFLAGS=logs
> ./build_DBG.OBJ/dist/bin/js --ion-offthread-compile=off

This is good, at least this means that your are in the right direction ;)

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8520789 [details] [diff] [review]
0001-WIP-2-Assert-that-instructions-are-recovered-on-bail.patch

Review of attachment 8520789 [details] [diff] [review]:
-----------------------------------------------------------------

I have no idea what might be wrong in this patch, can you change the dump function of the Assert MIR such as we can see the hidden operand in IonGraph?

::: js/src/jit/Lowering.cpp
@@ +541,5 @@
>  bool
> +LIRGenerator::visitAssertRecoveredOnBailout(MAssertRecoveredOnBailout *assertion)
> +{
> +    DebugOnly<bool> checkIsRecoveredOnBailout = assertion->isVariableRecoveredOnBailout();
> +    MOZ_ASSERT(checkIsRecoveredOnBailout);

No need to create a DebugOnly, you can just use the function as argument of the MOZ_ASSERT.

Comment 10

[Quentin Pradet [:quentinp]]

Attachment: 0001-WIP-3-Assert-that-instructions-are-recovered-on-bail.patch

So, I only removed the DebugOnly and rebased the patch on a more recent commit (Lowering.h changed bools to voids). It now seems that the assertion never triggers. What variable is never recovered on bailout in js/src/jit-test/tests/ion/dce-with-rinstructions.js so that I can make sure I'm not making a mistake?

(As discussed on IRC, I still need to display the opName() and id() of the instruction which is wrapped by the MAssertRecovered. I still wanted to post the current state here.)

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8541744 [details] [diff] [review]
0001-WIP-3-Assert-that-instructions-are-recovered-on-bail.patch

Review of attachment 8541744 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/MCallOptimize.cpp
@@ +2248,5 @@
> +    current->add(MAssertRecoveredOnBailout::New(alloc(), callInfo.getArg(0)));
> +
> +    MConstant *undefined = MConstant::New(alloc(), UndefinedValue());
> +    current->add(undefined);
> +    current->push(undefined);

nit:   current->push(constant(UndefinedValue()));

::: js/src/jit/MIR.h
@@ +3500,5 @@
>  
> +class MAssertRecoveredOnBailout : public MNullaryInstruction
> +{
> +  protected:
> +    MDefinition* ins_;

Add a comment that this is not an operand, as we do not want to add uses to the definition.

@@ +3505,5 @@
> +
> +    MAssertRecoveredOnBailout(MDefinition *ins)
> +      : ins_(ins)
> +    {
> +    }

This instruction might get removed, if it does not produce anything, add a

  setGuard();

to prevent this instruction from being removed bye DCE.  You can verify this in iongraph output by looking if this instruction is present in the last phase before lowering.

Comment 12

[Nicolas B. Pierron [:nbp]]

Attachment: Assert that instructions are recovered on bailouts.

I made this patch on top of Quentin's patch, keeping him as the owner of the
patch.

I modified the logic of MAssertRecoveredOnBailout to use the write function
of the recover instruction instead of the lowering.  This has the advantage
that adding this instruction in the MIR graph does not inhibit Sink
optimization. On the other hand we would have to modify Scalar Replacement
in a follow-up patch to make this assertion work there.

Comment 13

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8581688 [details] [diff] [review]
Assert that instructions are recovered on bailouts.

After more instrumentation of the code, I noticed multiple other issues, such as the fact that this function is not being captured by a resume point cause it to not be serialized, and thus the assertion might not be executed.

Comment 14

[Nicolas B. Pierron [:nbp]]

Attachment: Assert that instructions are recovered on bailouts.

Same as before, except that MBail is reused to only encode a snapshot (and
not bail) which will trigger the assertion.

Comment 15

[Nicolas B. Pierron [:nbp]]

Attachment: Use assertRecoveredOnBailout in the test suite.

Update all instructions which are recovered on bailout, except for the one
created by Scalar Replacement.

Note, all RegExp manipulation are for the moment marked as effectful, as
some language feature of JavaScript cause every manipulation of regexp (even
test) to mutate the state of the global.RegExp object with the result of the
matches.

Comment 16

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8581797 [details] [diff] [review]
Assert that instructions are recovered on bailouts.

Review of attachment 8581797 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/CodeGenerator.cpp
@@ +3372,5 @@
>  
>  void
>  CodeGenerator::visitBail(LBail *lir)
>  {
> +    if (lir->mir()->bailoutKind() == Bailout_OnlyEncodeSnapshot)

fixed: mir() does not work as expected here, because it can either be a MTypeBarrier or a MBail, so I had to use mirRaw and check isBail() before encoding the snapshot and returning.

::: js/src/jit/MCallOptimize.cpp
@@ +2572,5 @@
> +{
> +    callInfo.setImplicitlyUsedUnchecked();
> +
> +    MAssertRecoveredOnBailout *assert =
> +        MAssertRecoveredOnBailout::New(alloc(), callInfo.getArg(0));

fixed: I added a test for js_JitOptions.checkRangeAnalysis, in order to skip this code and only push the undefined constant.
The reason is that assertions added by Range Analysis are adding uses to all simple math operations, and Sink does not work as expected as we have unexpected uses.

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8581797 [details] [diff] [review]
Assert that instructions are recovered on bailouts.

Review of attachment 8581797 [details] [diff] [review]:
-----------------------------------------------------------------

Thinking about it, it seems having another MIR node for just encoding the snapshot would be less hackish, avoiding the condition in MBail's lowering and the new bailout kind. I guess you didn't know it because it was more boilerplate, but that's boilerplate I'd judge fine to have. Please disregard my comments below that apply to the BailoutKind if you agree.

Also, how does this behave with --ion-eager? In particular a situation where we don't have enough type info on the input of the AssertRecoveredOnBailout, and thus don't recover the input of bailout. In this case, will the assert not be inlined?

::: js/src/jit/IonTypes.h
@@ +151,5 @@
>      Bailout_IonExceptionDebugMode,
> +
> +    // A bailout kind which is used by MBail to know that we have to serialize a
> +    // snapshot without taking the bailout.
> +    Bailout_OnlyEncodeSnapshot,

Please add a case in BailoutKindString (same file) and FinishBailoutToBaseline (BaselineBailouts.cpp), even if these cases need to be MOZ_CRASH("this bailout shouldn't be ever be reached").

::: js/src/jit/LIR-Common.h
@@ +1682,5 @@
>    public:
>      LIR_HEADER(Bail)
> +
> +    MBail *mir() const {
> +        return mir_->toBail();

If i understand correctly your own review comment, this function is unused and should be deleted.

::: js/src/jit/Lowering.cpp
@@ +536,5 @@
>  void
> +LIRGenerator::visitAssertRecoveredOnBailout(MAssertRecoveredOnBailout *assertion)
> +{
> +    // AssertRecoveredOnBailout nodes are always recovered on bailouts
> +    MOZ_CRASH("Unexpected AssertRecoveredOnBailout node during Lowering.");

I'd prefer to have the comment as the MOZ_CRASH reason, and no comment at all :)

::: js/src/jit/MCallOptimize.cpp
@@ +2569,5 @@
>  
>  IonBuilder::InliningStatus
> +IonBuilder::inlineAssertRecoveredOnBailout(CallInfo &callInfo)
> +{
> +    callInfo.setImplicitlyUsedUnchecked();

this should go after the last conditional that returns something else than InliningStatus_Inlined (so after the resumeAfter condition below)

Comment 18

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8581801 [details] [diff] [review]
Use assertRecoveredOnBailout in the test suite.

Review of attachment 8581801 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! Two requests for follow up below, that you can fix in this patch if you like to (rs=me for these two modifications).

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +378,5 @@
>  function rinline_arguments_length_1(i) {
>      var x = ret_argumentsLength.apply(this, arguments);
>      if (uceFault_inline_arguments_length_1(i) || uceFault_inline_arguments_length_1(i))
>          assertEq(x, 1);
> +    // We cannot garantee that the function would be inlined

hehe, just create an isInlined intrinsic and use it as a condition here :P

@@ +420,2 @@
>      var x = Math.ceil(-i - 0.12010799100);
>      if (uceFault_ceil_number(i) ||uceFault_ceil_number(i))

while you're here and fixing pre-existing spacing nits: space after the ||, please

@@ +420,4 @@
>      var x = Math.ceil(-i - 0.12010799100);
>      if (uceFault_ceil_number(i) ||uceFault_ceil_number(i))
>          assertEq(x, - i);
> +    // NYI: uses MMathFunction and not MCeil.

That's probably because for i = 0, Math.ceil(x) === -0, which is a double and not an int32, and thus baseline records that it's seen double results and thus can't use MCeil. Can you open a follow up bug about this please? Because this means that we don't have a test for RCeil, as of today.

::: js/src/jit/MCallOptimize.cpp
@@ +2571,5 @@
>  IonBuilder::inlineAssertRecoveredOnBailout(CallInfo &callInfo)
>  {
>      callInfo.setImplicitlyUsedUnchecked();
>  
> +    MDefinition *secondArg = callInfo.getArg(1);

Maybe add a check that argc >= 2 (and if argc < 2, fprintf something to stderr because it's likely to be a bug in the test, only in non-deterministic builds). I saw that this is pre-existing also for inlineAssertFloat32, can you check it there as well? It has probably never been an issue during fuzzing because assertFloat32 is probably fuzzing-unsafe, but that is something to check.

::: js/src/jit/Recover.cpp
@@ +1434,5 @@
>  bool
>  MAssertRecoveredOnBailout::writeRecoverData(CompactBufferWriter &writer) const
>  {
>      MOZ_ASSERT(canRecoverOnBailout());
> +    MOZ_ASSERT(input()->isRecoveredOnBailout() == mustBeRecovered_);

Forgot to say so in the previous review, but can you please add a MOZ_ASSERT reason here? As this is the main assertion of this set of patches!

Comment 19

[Benjamin Bouvier [:bbouvier] (inactive)]

While the approach in these patches is ok, it appears that having JS checks is very error prone for different reasons, namely the check function calls have undesirable side-effect on the behavior of the program itself. A more general approach would be to have another jit spewer that can spew an entire MIR graph in JSON, and then regenerate a C++ code (a la jitTest_*.cpp, that reconstructs the MIR graph) from this JSON file. It would solve this problem and the problem of assertFloat32 more easily. While this seems more useful, it is also a way bigger task, so I am fine with the current approach :)

Comment 20

[Nicolas B. Pierron [:nbp]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #17)
> Review of attachment 8581797 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thinking about it, it seems having another MIR node for just encoding the
> snapshot would be less hackish, avoiding the condition in MBail's lowering
> and the new bailout kind. I guess you didn't know it because it was more
> boilerplate, but that's boilerplate I'd judge fine to have. Please disregard
> my comments below that apply to the BailoutKind if you agree.

Ok, I will add a MEncodeSnapshot, and use it instead of the MBail hack.
Indeed, the reuse of MBail was to avoid defining a new LIR / MIR, but if you think this is fine, then I will persue with this approach.

> Also, how does this behave with --ion-eager? In particular a situation where
> we don't have enough type info on the input of the AssertRecoveredOnBailout,
> and thus don't recover the input of bailout. In this case, will the assert
> not be inlined?

It does not work well with --ion-eager, for the reason that you highlight, but recover-on-bailout test cases are using  `setJitCompilerOption("ion.warmup.trigger", 20);`  which change the when of the compilation.  Note that `20` is the number used by TypeObject (= ObjectGroup) to fold the type information into one which can be used by the Jit.  So by doing more than 20 iteration, we do benefit from the Type information in all compilation.

The other side of the coin, is that we stop the execution of the code after the last bailout, in order to avoid a failure caused by the non-removal of the branch which contains the only live use.  Note that --disable-inlining should work fine as the assert function will not be inlined either.  After all, this assertion works fine, for a well written test case.

Comment 21

[Nicolas B. Pierron [:nbp]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #18)
> ::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
> @@ +378,5 @@
> >  function rinline_arguments_length_1(i) {
> >      var x = ret_argumentsLength.apply(this, arguments);
> >      if (uceFault_inline_arguments_length_1(i) || uceFault_inline_arguments_length_1(i))
> >          assertEq(x, 1);
> > +    // We cannot garantee that the function would be inlined
> 
> hehe, just create an isInlined intrinsic and use it as a condition here :P

Unfortunately, this would not work, as we are interested in the face that ret_argumentsLength is inlined, not rinline_arguments_length_1.

(In reply to Benjamin Bouvier [:bbouvier] from comment #19)
> While the approach in these patches is ok, it appears that having JS checks
> is very error prone for different reasons, namely the check function calls
> have undesirable side-effect on the behavior of the program itself. A more
> general approach would be to have another jit spewer that can spew an entire
> MIR graph in JSON, […]

Up to this point I agree.

> […] and then regenerate a C++ code (a la jitTest_*.cpp, that
> reconstructs the MIR graph) from this JSON file. It would solve this problem
> and the problem of assertFloat32 more easily. While this seems more useful,
> it is also a way bigger task, so I am fine with the current approach :)

But I do not think that having these assertions written in C++ would help us writing more tests, JavaScript should be enough and would make it easier to test without recompiling the test cases.

Also, if we manage to make a library for processing MIR graphs from JS, then we could make addons to do additional static analysis on the compiled code, as Jim suggested 9 months ago.

Comment 22

[Nicolas B. Pierron [:nbp]]

Attachment: Assert that instructions are recovered on bailouts.

Comment 23

[Nicolas B. Pierron [:nbp]]

Attachment: Use assertRecoveredOnBailout in the test suite.

Carry on r=bbouvier
I will open follow-up for other requests, I failed to use MCeil by avoiding
-0 return value :/

Comment 24

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8582414 [details] [diff] [review]
Assert that instructions are recovered on bailouts.

Review of attachment 8582414 [details] [diff] [review]:
-----------------------------------------------------------------

That's cleaner, thanks!

::: js/src/builtin/TestingFunctions.cpp
@@ +1506,5 @@
>      return true;
>  }
>  
> +bool
> +js::testingFunc_assertRecoveredOnBailout(JSContext *cx, unsigned argc, jsval *vp)

Might be worth to make it fuzzing unsafe, to be sure that the fuzzers don't go nuts with this new function, in --ion-eager mode...

::: js/src/jit/MIR.h
@@ +3887,5 @@
> +  protected:
> +    MAssertRecoveredOnBailout(MDefinition *ins)
> +      : MUnaryInstruction(ins)
> +    {
> +        setResultType(MIRType_Value);

MIRType_Undefined, maybe?

::: js/src/jit/Recover.h
@@ +739,5 @@
>  
>      bool recover(JSContext *cx, SnapshotIterator &iter) const;
>  };
>  
> +class RAssertRecoveredOnBailout final : public RInstruction

above class uses MOZ_FINAL, can you change that here as well please?

Comment 25

[Nicolas B. Pierron [:nbp]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #24)
> ::: js/src/builtin/TestingFunctions.cpp
> @@ +1506,5 @@
> >      return true;
> >  }
> >  
> > +bool
> > +js::testingFunc_assertRecoveredOnBailout(JSContext *cx, unsigned argc, jsval *vp)
> 
> Might be worth to make it fuzzing unsafe, to be sure that the fuzzers don't
> go nuts with this new function, in --ion-eager mode...

Good point, I will move it to the JS shell functions.

Comment 26

[Quentin Pradet [:quentinp]]

Comment on attachment 8582414 [details] [diff] [review]
Assert that instructions are recovered on bailouts.

Nicolas, please take ownership of the bug and of the resulting commit. (Sorry, I just managed to log in to Bugzilla; turns out it's impossible to get support when Persona doesn't work.)

Comment 27

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/486e2ec002fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/865c86092a5e

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/486e2ec002fe
https://hg.mozilla.org/mozilla-central/rev/865c86092a5e