910240 - IonMonkey: Assert popped values have the Folded flag if needed

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

We've had a number of bugs like bug 909505 because IonBuilder is not setting the Folded flag when it has to.

With some debug-only checks, we can catch these immediately in IonBuilder. Doing this caught a number of similar bugs.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Add missing setFoldedUnchecked calls

With these fixes, the next patch passes jit-tests --ion.

Comment 2

[Brian Hackett [Laid off!]]

Comment on attachment 796671 [details] [diff] [review]
Part 1 - Add missing setFoldedUnchecked calls

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

r=me with the below changes to not setFoldedUnchecked() for false positives.

::: js/src/jit/IonBuilder.cpp
@@ +1273,5 @@
>          return pushConstant(NullValue());
>  
> +      case JSOP_VOID: {
> +        MDefinition *def = current->pop();
> +        def->setFoldedUnchecked();

I think you should be marking this as a false positive in the subsequent patch.  When JSOP_VOID executes in the interpreter, the value at the top of the stack is overwritten irrespective of its contents.  We should only be setting the folded flag in cases where the MIR graph does not reflect a value dependency which is actually present in the interpreter, aka setting the folded flag has its own cost in terms of potentially hurting regalloc.

@@ +3591,5 @@
>  IonBuilder::jsop_pos()
>  {
>      if (IsNumberType(current->peek(-1)->type())) {
>          // Already int32 or double.
> +        current->peek(-1)->setFoldedUnchecked();

This shouldn't be necessary, the MIR after the operation reflects the dependency on the top of the stack, since the value at the top of the stack is unchanged.

@@ +8716,5 @@
>  IonBuilder::jsop_toid()
>  {
>      // No-op if the index is an integer.
> +    if (current->peek(-1)->type() == MIRType_Int32) {
> +        current->peek(-1)->setFoldedUnchecked();

Same issue here as JSOP_POS.

Comment 3

[Jan de Mooij [:jandem]]

Agreed about JSOP_VOID, but I think there's a problem with JSOP_POS if you have just this statement:

+foo;

If foo is known-int32, and we bailout, the baseline JIT could instead execute JSOP_POS with an |undefined| operand, no? Not that it matters for correctness in this case, but I think it's a bit weird/undesirable, also for type information (+undefined is NaN).

Comment 4

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> Agreed about JSOP_VOID, but I think there's a problem with JSOP_POS if you
> have just this statement:
> 
> +foo;
> 
> If foo is known-int32, and we bailout, the baseline JIT could instead
> execute JSOP_POS with an |undefined| operand, no? Not that it matters for
> correctness in this case, but I think it's a bit weird/undesirable, also for
> type information (+undefined is NaN).

Sure, but for the purposes of dead code analysis (which this is essentially a variant of) the goal is only to preserve the observable behavior of a piece of code, not its exact semantics; otherwise you couldn't do DCE at all.  A plain '+foo' statement on a number is dead code, and Ion is free to do what it wants here so long as observable behavior is preserved when OSR'ing to baseline, i.e. no exception or anything is thrown.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Add missing setFoldedUnchecked calls

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Assert the flag is set if needed

Comment 7

[Nicolas B. Pierron [:nbp]]

Can we rename it, this flag to its original meaning, which is to capture definitions in resume point to work against jit::EliminateDeadResumePointOperands which eagerly remove stuff which is used during bailouts.

Comment 8

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39a5b258b5c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/67417f81cb92

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/39a5b258b5c1
https://hg.mozilla.org/mozilla-central/rev/67417f81cb92