...
<rdar://problem/47915654>
Created attachment 363515 [details] Patch
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
Created attachment 363642 [details] Patch
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
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 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 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.
(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.
Created attachment 363756 [details] Patch
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
Created attachment 363786 [details] Patch
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
(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
(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.
(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
Created attachment 363851 [details] Patch
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.
Gonna take this over since Tadeu is on vacation and we want to land a fix for this.
Created attachment 364618 [details] patch
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.
Created attachment 364633 [details] patch for landing
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.
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
Created attachment 364663 [details] patch for landing
Comment on attachment 364663 [details] patch for landing Clearing flags on attachment: 364663 Committed r242954: <https://trac.webkit.org/changeset/242954>
All reviewed patches have been landed. Closing bug.