...
Created attachment 333502 [details] Patch
Comment on attachment 333502 [details] Patch Attachment 333502 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/6436599 New failing tests: stress/arrowfunction-lexical-bind-superproperty.js.ftl-eager
Created attachment 333571 [details] Patch
Comment on attachment 333571 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333571&action=review r- just because of a few comments, but the patch LGTM overall. It'd be good to have some inferred type related tests. > Source/JavaScriptCore/ChangeLog:3 > + [ESNext][BigInt] We should add support to BigInt into speculation "We should add support to BigInt into speculation" => "Add support for BigInt in SpeculatedType" > Source/JavaScriptCore/ChangeLog:8 > + This patch is introducing SpecBigInt type to DFG to enable BigInt is introducing => introduces the > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:10755 > + case InferredType::BigInt: > + speculate(BadType, jsValueValue(value), edge.node(), isNotCell(value, provenType(edge))); > + speculate(BadType, jsValueValue(value), edge.node(), isNotBigInt(value, provenType(edge))); > + return; Do you have a test for this? > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14904 > + You also need to add support in: void speculate(Edge edge) Should lead to compile time a crash, so you should add code that tests this. > Source/JavaScriptCore/runtime/JSBigInt.h:40 > + static const unsigned StructureFlags = Base::StructureFlags | OverridesToThis; Why? You're not overriding it. Should you be?
Comment on attachment 333571 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333571&action=review Thank you for the review. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:10755 >> + return; > > Do you have a test for this? No. I don't think it can be tested into current implementation, since "checkInferredType" is only being called by MultiPutByOffset, and we don't emit this Node if one of the variants is not an Object. That is basically the case of all Cell primitives. And also I couldn't find any test covering other InferredType::(String|Symbol|Int...). I added this line to keep consistency, but maybe I'm missing something. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14904 >> + > > You also need to add support in: > void speculate(Edge edge) > > Should lead to compile time a crash, so you should add code that tests this. Oops. Thank you for that. >> Source/JavaScriptCore/runtime/JSBigInt.h:40 >> + static const unsigned StructureFlags = Base::StructureFlags | OverridesToThis; > > Why? You're not overriding it. Should you be? This flag is checked into ToThis when the code is in StrictMode to return the primitive value instead. If we don't do that, the test "JSTests/stress/big-int-strict-spec-to-this.js" will fail when DFG code is installed. This is also being set into Symbol without overriding the ToThis method. Is it not correct?
Created attachment 334103 [details] Patch
Created attachment 334104 [details] Patch
Comment on attachment 334104 [details] Patch Attachment 334104 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/6551317 New failing tests: stress/ftl-put-by-id-setter-exception-interesting-live-state.js.dfg-eager-no-cjit-validate
Created attachment 334194 [details] Patch
Ping review
Created attachment 334519 [details] Patch
Ping Review
Created attachment 334981 [details] Patch Rebasing Patch
Created attachment 334983 [details] Patch Fixing Bug found into compileBigIntEquality because it was missing speculation check if its operands are Cell.
Created attachment 334984 [details] Patch Fixing build error into 32-bits.
Ping Review?
Created attachment 335357 [details] Patch
Created attachment 336010 [details] Patch Rebasing Patch.
Comment on attachment 336010 [details] Patch Attachment 336010 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7008446 New failing tests: http/tests/preload/download_resources.html
Created attachment 336015 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 336244 [details] Patch Rebasing Patch
Comment on attachment 336244 [details] Patch Attachment 336244 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/7057578 New failing tests: stress/ftl-put-by-id-setter-exception-interesting-live-state.js.dfg-eager-no-cjit-validate
(In reply to Build Bot from comment #22) > Comment on attachment 336244 [details] > Patch > > Attachment 336244 [details] did not pass jsc-ews (mac): > Output: http://webkit-queues.webkit.org/results/7057578 > > New failing tests: > stress/ftl-put-by-id-setter-exception-interesting-live-state.js.dfg-eager-no- > cjit-validate I tried reproduce this error locally, but it was not possible. I don't think it is related with my Patch.
Created attachment 336500 [details] Patch
Comment on attachment 336500 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336500&action=review r=me with comments/suggestions. Also, there is a place inside DFGAbstractInterpreterInlines for CompareStrictEqual that you want to hook into. The line like: if (node->child1() == node->child2()) { ... } Inside the ... you want to add a rule for BigInt speculations too. > Source/JavaScriptCore/ChangeLog:15 > + patches is to implement BigInt equallity check directly in equallity => equality > Source/JavaScriptCore/bytecode/SpeculatedType.h:71 > +static const SpeculatedType SpecCellOther = 1ull << 26; // It's definitely a JSCell but not a subclass of JSObject and definitely not a JSString, BigInt or a Symbol. JSString, BigInt or a Symbol => JSString, BigInt, or Symbol > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1471 > + if (isBigIntSpeculation(abstractChild.m_type)) { > + setConstant(node, *m_graph.freeze(m_vm.smallStrings.bigintString())); > + break; > + } Do you have a test for this? > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:2499 > + if (node->child1()->shouldSpeculateBigInt()) { Do you have a test for this? > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5868 > + callOperation(operationCompareStrictEqCell, resultGPR, leftGPR, rightGPR); You're sure no exception will ever happen in this compare? > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5871 > + m_jit.and64(JITCompiler::TrustedImm32(1), resultGPR); Why is this necessary? > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6921 > + LValue left = lowJSValue(m_node->child1(), ManualOperandSpeculation); > + LValue right = lowJSValue(m_node->child2(), ManualOperandSpeculation); This is an anti pattern. You should add a lowBigInt function and use it instead of speculate() above. See lowSymbol, lowObject, etc. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6927 > + ValueFromBlock fastResult = m_out.anchor(isEqualValue); By making this the result, there is a chance B3/Air won't emit a fused compare and branch. It's worth ensuring it does. You can always make fastResult just be m_out.constInt32(1) if B3 doesn't emit a fused compare/branch. Actually, it's probably just making this one since we know the result if the compare is true. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6928 > + m_out.branch(isEqualValue, rarely(continuation), usually(slowPath)); If this is actually rarely/usually, you may want to give these blocks different names.
Comment on attachment 336500 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336500&action=review Thank you very much for the review and comments >> Source/JavaScriptCore/ChangeLog:15 >> + patches is to implement BigInt equallity check directly in > > equallity => equality Oops. >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1471 >> + } > > Do you have a test for this? Yes. It is the "JSTests/stress/big-int-strict-spec-to-this.js" >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:2499 >> + if (node->child1()->shouldSpeculateBigInt()) { > > Do you have a test for this? Yes. It is the "JSTests/stress/big-int-strict-spec-to-this.js" >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5868 >> + callOperation(operationCompareStrictEqCell, resultGPR, leftGPR, rightGPR); > > You're sure no exception will ever happen in this compare? Yes. AFAIK, when we generate this code, both operands are BigInts and there is no exception on StrictEquals operation on BigInt. >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5871 >> + m_jit.and64(JITCompiler::TrustedImm32(1), resultGPR); > > Why is this necessary? I'm following the same approach of "SpeculativeJIT::nonSpeculativeNonPeepholeStrictEq" into "dfg/DFGSpeculativeJIT64.cpp". If I understand it correctly, it is sanitizing the result from "operationCompareStrictEqCell". Is it the real meaning of this operation? If not, I will remove in both places. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6921 >> + LValue right = lowJSValue(m_node->child2(), ManualOperandSpeculation); > > This is an anti pattern. You should add a lowBigInt function and use it instead of speculate() above. See lowSymbol, lowObject, etc. Ok! I decided to do that because otherwise we won't have any other trace to test the case into "speculate(Edge edge)". But I agree that introducing lowBigInt makes more sense. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6927 >> + ValueFromBlock fastResult = m_out.anchor(isEqualValue); > > By making this the result, there is a chance B3/Air won't emit a fused compare and branch. It's worth ensuring it does. You can always make fastResult just be m_out.constInt32(1) if B3 doesn't emit a fused compare/branch. Actually, it's probably just making this one since we know the result if the compare is true. You are right. The comparison isn't being fused here. Thanks for that. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6928 >> + m_out.branch(isEqualValue, rarely(continuation), usually(slowPath)); > > If this is actually rarely/usually, you may want to give these blocks different names. Done.
Created attachment 336764 [details] Patch
Ping. I have a question before committing this Patch. >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5871 >> + m_jit.and64(JITCompiler::TrustedImm32(1), resultGPR); > > Why is this necessary? I'm following the same approach of "SpeculativeJIT::nonSpeculativeNonPeepholeStrictEq" into "dfg/DFGSpeculativeJIT64.cpp". If I understand it correctly, it is sanitizing the result from "operationCompareStrictEqCell". Is it the real meaning of this operation? If not, I will remove in both places.
(In reply to Caio Lima from comment #29) > Ping. I have a question before committing this Patch. > > >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5871 > >> + m_jit.and64(JITCompiler::TrustedImm32(1), resultGPR); > > > > Why is this necessary? > > I'm following the same approach of > "SpeculativeJIT::nonSpeculativeNonPeepholeStrictEq" into > "dfg/DFGSpeculativeJIT64.cpp". If I understand it correctly, it is > sanitizing the result from "operationCompareStrictEqCell". Is it the real > meaning of this operation? If not, I will remove in both places. It seems that is indeed what it's doing. It just seems peculiar that it does that given we assume static_cast<size_t>(true)==1 elsewhere in the code. You can probably just keep it as is.
Created attachment 337674 [details] Patch
Created attachment 337677 [details] Benchmark It is perf neutral
Comment on attachment 337674 [details] Patch Clearing flags on attachment: 337674 Committed r230516: <https://trac.webkit.org/changeset/230516>
All reviewed patches have been landed. Closing bug.
<rdar://problem/39338249>
I skipped the JIT tests when it isn't enabled in https://trac.webkit.org/changeset/230564
(In reply to Tomas Popela from comment #36) > I skipped the JIT tests when it isn't enabled in > https://trac.webkit.org/changeset/230564 Why? Just to make tests run faster?
(In reply to Saam Barati from comment #37) > (In reply to Tomas Popela from comment #36) > > I skipped the JIT tests when it isn't enabled in > > https://trac.webkit.org/changeset/230564 > > Why? Just to make tests run faster? No, look at bug 182730, the tests that are using numberOfDFGCompiles() will fail without JIT enabled.
(In reply to Tomas Popela from comment #38) > (In reply to Saam Barati from comment #37) > > (In reply to Tomas Popela from comment #36) > > > I skipped the JIT tests when it isn't enabled in > > > https://trac.webkit.org/changeset/230564 > > > > Why? Just to make tests run faster? > > No, look at bug 182730, the tests that are using numberOfDFGCompiles() will > fail without JIT enabled. Ah. These type of tests are really something else... They fail for so many weird reasons. This LGTM