Closed Bug 1004169 Opened 10 years ago Closed 10 years ago

While loops check whether their condition emulates undefined even when it can't

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached file Testcase
The attached testcase shows us outputting a TestVAndBranch:MightEmulateUndefined on the while loop.

It seems like any MTest would do that, unless MTest::infer() is called on it and the operand is in fact something that can't emulate undefined.

But the only caller of MTest::infer() seems to be IonBuilder::jsop_andor, according to dxr.  Notably, IonBuilder::processWhileCondEnd doesn't do it, nor do the many other places that create MTest instances in IonBuilder.

Why can we not just do this in the MTest ctor instead of having the separate infer() method?  Can the operand change later or something?
Flags: needinfo?(hv1989)
jandem might remember more about this, too.
Flags: needinfo?(jdemooij)
Blocks: 1004198
Note that MNot has a similar setup with infer(), but we call that more consistently (like every place where we do an MNot::New).
Something like this, maybe?
Attachment #8415776 - Flags: feedback?(hv1989)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
No longer blocks: 1004198
Depends on: 1004198
Comment on attachment 8415776 [details] [diff] [review]
Make sure MTest always uses TI information for deciding whether its operand might emulate undefined

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

The infer instruction is normally used for operations that get type information from the BaselineInspector, like MCompare, MBinaryBitwiseInstruction, MShiftInstruction ...
The signature of the call should be "void infer(BaselineInspector *inspector, jsbytecode *pc);" in that case.

Now it looks like MNot, MTest, MBitNot and MTypeOf shouldn't use it.

1) I agree with adjusting MNot, MTest to what you did.
2) Please update MTypeOf and MBitNot too.
3) We have had the same issue with MCompare::infer not getting called everywhere. Maybe we should assert in DEBUG code that infer is called for the other operations?
Attachment #8415776 - Flags: feedback?(hv1989) → feedback+
(In reply to Boris Zbarsky [:bz] from comment #0)
> Why can we not just do this in the MTest ctor instead of having the separate
> infer() method?  Can the operand change later or something?

MTest instructions can be created off-thread, after IonBuilder runs. Nowadays we're using TemporaryTypeSets for MIR instructions, which are only accessed by the current compilation, so the only worry is TemporaryTypeSet::maybeEmulatesUndefined() racing with the main thread when it calls getObjectClass.

For a TypeObject that gets its clasp_ directly, for a singleton it ends up doing obj->type_->clasp_. I think these are immutable so this is probably safe, but it still feels safer to do this only on the main thread. Maybe we could add a separate MTest::New method?
Flags: needinfo?(jdemooij)
Alright, so I pushed the above patch to try and it failed my new asserts.

Specifically, it failed because in js/src/jit-test/tests/auto-regress/bug682563.js we construct an MTest whose "ins" is an MPhi.  This MPhi's resultTypeSet has the TYPE_FLAG_ANYOBJECT and TYPE_FLAG_STRING flags set, so we think it can emulate undefined.

However by the time this MTest gets into visitTestVAndBranch its getOperand(0) is an MBox around an MConstant whose Value is a string.  Which makes sense; that's the 

  "$0$00" || this 

bit, and we actually noticed that in practice it's never an object.  The problem is we noticed that after the MTest was constructed with the MPhi.

Now the interesting thing is, we actually create two MTests in this case.  The first one we create has the MConstant in question as an operand.  The second has the MPhi.  Then eventually we go through OptimizeMIR, which does EliminatePhis, which ends up replacing the operand, via MAryControlInstruction<1ul, 2ul>::setOperand.

Seems like we should update our boolean at that point.  Can MTest override setOperand?  Right now it's marked MOZ_FINAL...

For the rest, I can add a separate MTest::New method, but then I have to go through and find all the places to use it.  Is that just "everywhere in IonBuilder"?  But even then, do we end up doing the OptimizeMIR bit on the background thread?

The other option, of course, is I could loosen (read: remove) the assertion and have less-optimal code in this case where an MPhi that could have object type got eliminated in favor of a known non-object...  Though actually, why are we even doing a visitTestVAndBranch if our MTest is testing an MConstant for a string?  Seems like that should also be able to be optimized away.  And if not, why did we end up with the MBox instead of an MConstant?  I suppose the phi was between two boxed types and we didn't unbox because we had no way to tell the consumer that their operand was changing type?
Flags: needinfo?(jdemooij)
OK, per discussion with jandem:

1)  Will introduce IonBuilder::newTest, which does MTest::New and then calls a method to determine whether the operand can emulate undefined.  For the other things comment 4 lists, leave the "infer" calls to save some time in asm.js, but rename "infer" to something that more clearly indicates what it's doing.

2)  Will document that the the "can emulate undefined" thing is conservative and we can end up with a known-not-object type even when that bit is set (e.g. due to the phi elimination above).

3)  Will file a separate bug on the MTest(MBox(MConstant)) bit.
Flags: needinfo?(jdemooij)
Flags: needinfo?(hv1989)
Filed bug 1004606 for comment 7 item 3.
Attachment #8415776 - Attachment is obsolete: true
Whiteboard: [need review]
(In reply to Jan de Mooij [:jandem] from comment #5)
> (In reply to Boris Zbarsky [:bz] from comment #0)
> > Why can we not just do this in the MTest ctor instead of having the separate
> > infer() method?  Can the operand change later or something?
> 
> MTest instructions can be created off-thread, after IonBuilder runs.
> Nowadays we're using TemporaryTypeSets for MIR instructions, which are only
> accessed by the current compilation, so the only worry is
> TemporaryTypeSet::maybeEmulatesUndefined() racing with the main thread when
> it calls getObjectClass.

Oh I had no idea.
Comment on attachment 8416265 [details] [diff] [review]
Make sure MTest always uses TI information for deciding whether its operand might emulate undefined

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

::: js/src/jit/IonBuilder.cpp
@@ +6000,5 @@
>      return block;
>  }
>  
> +MTest *
> +IonBuilder::newTest(TempAllocator &alloc, MDefinition *ins,

You can remove the alloc argument and inside this method use alloc(). Then this also fits on one line (99 chars per line in js/src):

IonBuilder::newTest(MDefinition *ins, MBasicBlock *ifTrue, MBasicBlock *ifFalse)
{

@@ +6003,5 @@
> +MTest *
> +IonBuilder::newTest(TempAllocator &alloc, MDefinition *ins,
> +                    MBasicBlock *ifTrue, MBasicBlock *ifFalse)
> +{
> +    MTest* test = MTest::New(alloc, ins, ifTrue, ifFalse);

Nit: MTest *test in SpiderMonkey
Attachment #8416265 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fbc044027e6 with those bits fixed.
Flags: in-testsuite-
Whiteboard: [need review]
https://hg.mozilla.org/mozilla-central/rev/2fbc044027e6
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.