995076 - Unconditionally replace null/undefined/Magic-typed OSR slots

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Shu-yu Guo [:shu]]

Type adjustment happens after phi specialization. When adjusting TypeBarrier
types, we unspecialize the type of the instruction back to the Value when
trying to unbox null/undefined/magic values. We need to propagate this to all
phi use sites. I ran into this bug when debugging part 1 of bug 716647 (the
optimized out magic value patch).

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Unspecialize phis when unspecializing TypeBarrier input

Hannes, I think you added this code.

Comment 2

[Hannes Verschore [:h4writer]]

Comment on attachment 8405175 [details] [diff] [review]
Unspecialize phis when unspecializing TypeBarrier input

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

Sorry, it took me some time to understand the issue. But I think I have it now.
The patch for me looked like patching/hiding the actual issue at hand, but it took some time to pinpoint the actual issue without a testcase.

1) So when creating a TypeBarrier the outputType is set. 
This can be anything (<= MIRType_Value). So also MIRType_Undefined, MIRType_Null and MIRType_Magic.

2) During "analyze types" if the inputType is a MIRType_Value a MUnbox is added.

3) MUnbox doesn't work for MIRType_Undefined/Null/Magic. As a result in those cases (in IonBuilder) we replace the pushed statement to a MConstant. See:
- http://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonBuilder.cpp#1065
- http://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonBuilder.cpp#6288

So we shouldn't have a MTypeBarrier with a MIRType_Value with outputType MIRType_Magic that has a use. It never should have an use,
because we should always have replaced it with a MConstant. Looking at it now we should assert that in TypePolicy.
As a result the adjustment in TypePolicy of the TypeBarrier to MIRType_Value is uneffectfull.

So the patch might solve the issue you see in a particular case. But won't solve the issue that I just explained.
We need to find where we created such a MTypeBarrier and adjust it!

E.g. I just noticed that in pushTypeBarrier we don't handle the MIRType_Magic case. It could be we only need to fix that...
http://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonBuilder.cpp#6288

Note: if you have a testcase I'm willing to look myself to fix it. It is indeed my code. So I'm most familiar with it to debug.

Comment 3

[Shu-yu Guo [:shu]]

Attachment: Unconditionally replace null/undefined/Magic-typed OSR slots

Hannes helped debug and come up with a real fix. Thanks, Hannes!

Comment 4

[Hannes Verschore [:h4writer]]

Comment on attachment 8405620 [details] [diff] [review]
Unconditionally replace null/undefined/Magic-typed OSR slots

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

Thanks

Comment 5

[Shu-yu Guo [:shu]]

Attachment: Part 2: Don't specialize phis to magic type due to unobserved operands or when being kept alive by resume points

Turns out the first part wasn't enough. It can happen that cycles of phis kept
alive by resume points in turn keep magic OPTIMIZED_ARGUMENTS alive, causing
asserts when we try to lower the constants. We should unspecialize phis to
Value if this is the case.

I also removed some dead code from addOsrValueTypeBarrier, since we changed the
null/undefined/magic constant replacement to be unconditional, the TypeBarrier
insertion code above is now dead.

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8405712 [details] [diff] [review]
Part 2: Don't specialize phis to magic type due to unobserved operands or when being kept alive by resume points

In a happy coincidence, with bug 996422 this ad hoc fix is no longer necessary.

Comment 7

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c2aaabb1f35

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6c2aaabb1f35