Bug 195279 – Fixup uses KnownInt32 incorrectly in some nodes

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Tadeu Zagallo]

...

[Tadeu Zagallo]

<rdar://problem/47915654>

[Tadeu Zagallo]

Created attachment 363515 [details]
Patch

[EWS Watchlist]

Comment on attachment 363515 [details]
Patch

Attachment 363515 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/11363692

New failing tests:
mozilla-tests.yaml/js1_5/Scope/regress-77578-001.js.mozilla-ftl-eager-no-cjit-validate-phases
mozilla-tests.yaml/js1_5/Scope/regress-77578-001.js.mozilla-dfg-eager-no-cjit-validate-phases

[Tadeu Zagallo]

Created attachment 363642 [details]
Patch

[EWS Watchlist]

Comment on attachment 363642 [details]
Patch

Attachment 363642 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11380561

New failing tests:
webaudio/audioparam-setValueAtTime.html
inspector/model/remote-object-get-properties.html
webaudio/audioparam-linearRampToValueAtTime.html
fast/inline/continuation-with-anon-wrappers.html
js/slow-stress/variadic-closure-call.html
js/dfg-array-push-bad-time.html
webaudio/audioparam-exponentialRampToValueAtTime.html
inspector/codemirror/prettyprinting-css.html
webaudio/audioparam-setTargetAtTime.html
webaudio/audioparam-setValueCurveAtTime.html
imported/w3c/web-platform-tests/dom/events/Event-timestamp-safe-resolution.html
inspector/debugger/call-frame-this-nonstrict.html
inspector/debugger/call-frame-this-strict.html

[EWS Watchlist]

Created attachment 363660 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Saam Barati]

Comment on attachment 363642 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363642&action=review

You need to make sure we do the right thing for these nodes in the other parts of the compiler. As an example, you need to vet and make sure we don't switch on KnownInt32use (and now it's Int32Use). Another example, you need to make SpeculateStrictInt32Operand SpeculateInt32Operand for GetEnumeratorStructurePname in DFGSpeculativeJIT

> Source/JavaScriptCore/ChangeLog:11
> +        For example, if the incoming value is a local and its underlying slot is reused,

I don't think the reused bit here is what's interesting. This can happen for general reasons.

> Source/JavaScriptCore/ChangeLog:12
> +        its type might be widened and it might no longer satify this requirement.

also widened isn't exactly correct. We may pick a different type for it.

[Yusuke Suzuki]

Comment on attachment 363642 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363642&action=review

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1140
> +                    fixEdge<Int32Use>(element);

ArrayPush node relies on the fact that checks are already done (the above `insertCheck`'s Check node) when executing ArrayPush. So it needs to be KnownInt32Use.

[Saam Barati]

(In reply to Yusuke Suzuki from comment #8)
> Comment on attachment 363642 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363642&action=review
> 
> > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1140
> > +                    fixEdge<Int32Use>(element);
> 
> ArrayPush node relies on the fact that checks are already done (the above
> `insertCheck`'s Check node) when executing ArrayPush. So it needs to be
> KnownInt32Use.

We need to make it itself speculate. So we should probably remove the Check(Int32) insertion. Me and Phil talked about this yesterday, and it's slightly wrong, since you could theoretically have different values feeding into the Check and the ArrayPush.

[Tadeu Zagallo]

Created attachment 363756 [details]
Patch

[EWS Watchlist]

Comment on attachment 363756 [details]
Patch

Attachment 363756 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/11399688

New failing tests:
sunspider-1.0/string-tagcloud.js.dfg-maximal-flush-validate-no-cjit
sunspider-1.0/string-fasta.js.dfg-maximal-flush-validate-no-cjit

[Tadeu Zagallo]

Created attachment 363786 [details]
Patch

[Saam Barati]

Comment on attachment 363786 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363786&action=review

Are all the other nodes vetted? I think you still have that StrictInt32 crap that needs to be removed for the EnumeratorPName.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:8562
> +                JSValueOperand value(this, element, ManualOperandSpeculation);
> +                valueRegs = value.jsValueRegs();
> +            }

You should remove the above if and just call speculate(...) here

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4668
> +                    value = boxInt32(lowInt32(element));

just call: speculate(element)

You're still missing code for the elemntCount > 1 case

[Saam Barati]

(In reply to Saam Barati from comment #13)
> Comment on attachment 363786 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363786&action=review
> 
> Are all the other nodes vetted? I think you still have that StrictInt32 crap
> that needs to be removed for the EnumeratorPName.
Specifically, compileGetEnumeratorPname
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:8562
> > +                JSValueOperand value(this, element, ManualOperandSpeculation);
> > +                valueRegs = value.jsValueRegs();
> > +            }
> 
> You should remove the above if and just call speculate(...) here
> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4668
> > +                    value = boxInt32(lowInt32(element));
> 
> just call: speculate(element)
> 
> You're still missing code for the elemntCount > 1 case

[Tadeu Zagallo]

(In reply to Saam Barati from comment #14)
> (In reply to Saam Barati from comment #13)
> > Comment on attachment 363786 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=363786&action=review
> > 
> > Are all the other nodes vetted? I think you still have that StrictInt32 crap
> > that needs to be removed for the EnumeratorPName.
> Specifically, compileGetEnumeratorPname

I forgot to reply to this yesterday, but I don't think that change is correct. We still need to SpeculateStrictInt32Operand in compileGetEnumeratorPname, otherwise we don't clear the high bits and it crashes when we use the index to load.

[Saam Barati]

(In reply to Tadeu Zagallo from comment #15)
> (In reply to Saam Barati from comment #14)
> > (In reply to Saam Barati from comment #13)
> > > Comment on attachment 363786 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=363786&action=review
> > > 
> > > Are all the other nodes vetted? I think you still have that StrictInt32 crap
> > > that needs to be removed for the EnumeratorPName.
> > Specifically, compileGetEnumeratorPname
> 
> I forgot to reply to this yesterday, but I don't think that change is
> correct. We still need to SpeculateStrictInt32Operand in
> compileGetEnumeratorPname, otherwise we don't clear the high bits and it
> crashes when we use the index to load.

Got it. I forgot that SpeculateInt32Operand doesn’t unbox

[Tadeu Zagallo]

Created attachment 363851 [details]
Patch

[Filip Pizlo]

Comment on attachment 363851 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363851&action=review

Currently this will push the same value more than once if you fail speculation, or it will leave the array in a mode where elements past the length are not holes.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:8599
> +            if (node->arrayMode().type() == Array::Int32)
> +                speculate(node, element);

This needs to be a separate pass, before you do anything to the array.  Probably same issue in the FTL.

[Saam Barati]

Gonna take this over since Tadeu is on vacation and we want to land a fix for this.

[Saam Barati]

Created attachment 364618 [details]
patch

[Yusuke Suzuki]

Comment on attachment 364618 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364618&action=review

r=me

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:8678
>  

You can remove `DFG_ASSERT(m_jit.graph(), node, !needsTypeCheck(element, SpecDoubleReal));` in the loop too.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4728
>  

I think removing L4762-4763 and L4767 would be nice for consistency.

[Saam Barati]

Created attachment 364633 [details]
patch for landing

[EWS Watchlist]

Comment on attachment 364633 [details]
patch for landing

Attachment 364633 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11501034

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 364636 [details]
Archive of layout-test-results from ews116 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Saam Barati]

Created attachment 364663 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 364663 [details]
patch for landing

Clearing flags on attachment: 364663

Committed r242954: <https://trac.webkit.org/changeset/242954>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.