Bug 195279 - Fixup uses KnownInt32 incorrectly in some nodes
Summary: Fixup uses KnownInt32 incorrectly in some nodes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-04 08:37 PST by Tadeu Zagallo
Modified: 2019-03-14 12:27 PDT (History)
10 users (show)

See Also:


Attachments
Patch (7.62 KB, patch)
2019-03-04 08:44 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (5.71 KB, patch)
2019-03-05 09:22 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-highsierra (3.33 MB, application/zip)
2019-03-05 11:25 PST, Build Bot
no flags Details
Patch (12.14 KB, patch)
2019-03-06 10:50 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (9.90 KB, patch)
2019-03-06 13:35 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (9.64 KB, patch)
2019-03-06 23:55 PST, Tadeu Zagallo
fpizlo: review-
Details | Formatted Diff | Diff
patch (14.19 KB, patch)
2019-03-13 20:20 PDT, Saam Barati
ysuzuki: review+
Details | Formatted Diff | Diff
patch for landing (15.27 KB, patch)
2019-03-13 22:07 PDT, Saam Barati
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-highsierra (2.83 MB, application/zip)
2019-03-13 23:39 PDT, Build Bot
no flags Details
patch for landing (15.48 KB, patch)
2019-03-14 09:57 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tadeu Zagallo 2019-03-04 08:37:53 PST
...
Comment 1 Tadeu Zagallo 2019-03-04 08:41:49 PST
<rdar://problem/47915654>
Comment 2 Tadeu Zagallo 2019-03-04 08:44:08 PST
Created attachment 363515 [details]
Patch
Comment 3 Build Bot 2019-03-04 11:23:37 PST
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
Comment 4 Tadeu Zagallo 2019-03-05 09:22:20 PST
Created attachment 363642 [details]
Patch
Comment 5 Build Bot 2019-03-05 11:25:32 PST
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
Comment 6 Build Bot 2019-03-05 11:25:35 PST
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
Comment 7 Saam Barati 2019-03-05 11:26:21 PST
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.
Comment 8 Yusuke Suzuki 2019-03-05 11:41:10 PST
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.
Comment 9 Saam Barati 2019-03-05 11:58:42 PST
(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.
Comment 10 Tadeu Zagallo 2019-03-06 10:50:21 PST
Created attachment 363756 [details]
Patch
Comment 11 Build Bot 2019-03-06 13:28:36 PST
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
Comment 12 Tadeu Zagallo 2019-03-06 13:35:10 PST
Created attachment 363786 [details]
Patch
Comment 13 Saam Barati 2019-03-06 17:15:09 PST
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
Comment 14 Saam Barati 2019-03-06 17:15:49 PST
(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
Comment 15 Tadeu Zagallo 2019-03-06 23:19:15 PST
(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.
Comment 16 Saam Barati 2019-03-06 23:53:41 PST
(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
Comment 17 Tadeu Zagallo 2019-03-06 23:55:23 PST
Created attachment 363851 [details]
Patch
Comment 18 Filip Pizlo 2019-03-12 11:08:15 PDT
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.
Comment 19 Saam Barati 2019-03-13 19:19:31 PDT
Gonna take this over since Tadeu is on vacation and we want to land a fix for this.
Comment 20 Saam Barati 2019-03-13 20:20:13 PDT
Created attachment 364618 [details]
patch
Comment 21 Yusuke Suzuki 2019-03-13 20:30:08 PDT
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.
Comment 22 Saam Barati 2019-03-13 22:07:43 PDT
Created attachment 364633 [details]
patch for landing
Comment 23 Build Bot 2019-03-13 23:39:27 PDT
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.
Comment 24 Build Bot 2019-03-13 23:39:30 PDT
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
Comment 25 Saam Barati 2019-03-14 09:57:53 PDT
Created attachment 364663 [details]
patch for landing
Comment 26 WebKit Commit Bot 2019-03-14 12:27:34 PDT
Comment on attachment 364663 [details]
patch for landing

Clearing flags on attachment: 364663

Committed r242954: <https://trac.webkit.org/changeset/242954>
Comment 27 WebKit Commit Bot 2019-03-14 12:27:36 PDT
All reviewed patches have been landed.  Closing bug.