RESOLVED FIXED 193240
[ESNext][BigInt] Add support for op_inc
https://bugs.webkit.org/show_bug.cgi?id=193240
Summary [ESNext][BigInt] Add support for op_inc
Caio Lima
Reported 2019-01-08 09:34:31 PST
...
Attachments
WIP - Patch (64.77 KB, patch)
2019-01-11 16:58 PST, Caio Lima
no flags
Archive of layout-test-results from ews117 for mac-highsierra (1.36 MB, application/zip)
2019-01-11 18:07 PST, EWS Watchlist
no flags
WIP - Patch (63.51 KB, patch)
2019-01-14 03:22 PST, Caio Lima
no flags
WIP - Patch (63.71 KB, patch)
2019-01-14 07:29 PST, Caio Lima
no flags
WIP (43.70 KB, patch)
2019-11-04 13:43 PST, Robin Morisset
no flags
Patch (73.12 KB, patch)
2019-11-07 15:16 PST, Robin Morisset
no flags
Patch (73.12 KB, patch)
2019-11-07 18:50 PST, Robin Morisset
ews-watchlist: commit-queue-
Patch (76.26 KB, patch)
2019-11-08 14:30 PST, Robin Morisset
ews-watchlist: commit-queue-
diff (12.17 KB, patch)
2019-11-08 14:31 PST, Robin Morisset
no flags
Patch (74.77 KB, patch)
2019-11-13 17:34 PST, Robin Morisset
no flags
Patch (77.02 KB, patch)
2019-11-15 13:54 PST, Robin Morisset
no flags
Patch (78.07 KB, patch)
2019-11-18 11:42 PST, Robin Morisset
no flags
Patch (78.96 KB, patch)
2019-11-18 17:19 PST, Robin Morisset
no flags
Patch (78.04 KB, patch)
2019-11-18 17:35 PST, Robin Morisset
no flags
Patch (79.55 KB, patch)
2019-11-19 15:32 PST, Robin Morisset
ysuzuki: review+
rmorisset: commit-queue-
diff from previous patch (5.15 KB, patch)
2019-11-19 15:32 PST, Robin Morisset
no flags
Patch (80.10 KB, patch)
2019-11-19 18:58 PST, Robin Morisset
no flags
Caio Lima
Comment 1 2019-01-11 16:58:05 PST
Created attachment 358966 [details] WIP - Patch
EWS Watchlist
Comment 2 2019-01-11 17:00:02 PST
Attachment 358966 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:1452: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 49 files If any of these errors are false positives, please file a bug against check-webkit-style.
EWS Watchlist
Comment 3 2019-01-11 18:07:01 PST
Comment on attachment 358966 [details] WIP - Patch Attachment 358966 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10720373 Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 4 2019-01-11 18:07:02 PST
Created attachment 358968 [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
Caio Lima
Comment 5 2019-01-14 03:22:30 PST
Created attachment 359025 [details] WIP - Patch
EWS Watchlist
Comment 6 2019-01-14 03:25:18 PST
Attachment 359025 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:1452: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 49 files If any of these errors are false positives, please file a bug against check-webkit-style.
Caio Lima
Comment 7 2019-01-14 07:29:07 PST
Created attachment 359035 [details] WIP - Patch
EWS Watchlist
Comment 8 2019-01-14 07:32:54 PST
Attachment 359035 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:1452: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 49 files If any of these errors are false positives, please file a bug against check-webkit-style.
Robin Morisset
Comment 9 2019-11-04 13:43:34 PST
Created attachment 382769 [details] WIP Still missing tests.
Robin Morisset
Comment 10 2019-11-07 15:16:33 PST
Robin Morisset
Comment 11 2019-11-07 18:50:18 PST
Created attachment 383101 [details] Patch rebased now that the ArithProfile patch is back in.
EWS Watchlist
Comment 12 2019-11-07 21:40:00 PST
Comment on attachment 383101 [details] Patch Attachment 383101 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/13225136 New failing tests: stress/arith-negate-on-various-types.js.default stress/arith-negate-on-various-types.js.no-cjit-collect-continuously stress/arith-negate-on-various-types.js.ftl-no-cjit-validate-sampling-profiler stress/arith-negate-on-various-types.js.no-cjit-validate-phases stress/arith-negate-on-various-types.js.ftl-no-cjit-b3o0 stress/arith-negate-on-various-types.js.ftl-no-cjit-no-inline-validate stress/arith-negate-on-various-types.js.no-ftl stress/arith-negate-on-various-types.js.no-llint
Caio Lima
Comment 13 2019-11-08 06:56:20 PST
Comment on attachment 383101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383101&action=review Thank you very much for doing work on this. I left some comments in the Patch. > Source/JavaScriptCore/ChangeLog:11 > + - teaching FixupPhase how to replace it by ArithAdd/ArithSub when the type is Int32/Double/BigInt When type is BigInt, we actually fixup this to ValueAdd/ValueSub. > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:935 > + case Dec: { We should be able to apply constant folding when we prove that `node->child1()` is a number constant, even when `node->child1().useKind() == UntypedUse`. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:203 > + node->setResult(NodeResultInt32); We can also `node->clearFlags(NodeMustGenerate);` here. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:210 > + fixEdge<BigIntUse>(node->child2()); And here. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:212 > + nit: remove blank line. I also think the comment above is unnecessary. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:213 > + } else { nit: this else is not needed. > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:1181 > + case Dec: I think rules for prediction propagation are missing there. > Source/JavaScriptCore/jit/JITOpcodes.cpp:666 > + auto bytecode = currentInstruction->as<OpToNumber>(); I think you want to do `OpToNumeric`. > Source/JavaScriptCore/jit/JITOpcodes.cpp:671 > + // FIXME: fill in! I don't understand this FIXME here. Can you create a bug and elaborate on that? > Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:844 > + auto bytecode = currentInstruction->as<OpToNumber>(); Ditto. > Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:851 > + SDjkadakdhsakj; I suspect this is a compilation error. > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:464 > + result = JSBigInt::inc(globalObject, WTF::get<JSBigInt*>(resultVariant)); Why do we use here `JSBigInt::inc`, wihle using `JSBigInt::add(..., vm.bigIntConstantOne.get())` into `operationInc` and `operationDec`? > Source/JavaScriptCore/runtime/VM.cpp:396 > + bigIntConstantOne.set(*this, JSBigInt::createFrom(*this, 1)); What happens in case `canUseJIT() == false`? This constant is still being used by LLInt on `JSBigInt::inc` or `JSBigInt::dec` and I suspect we will have an error in such scenario.
Robin Morisset
Comment 14 2019-11-08 09:18:27 PST
Comment on attachment 383101 [details] Patch I clearly missed a couple FIXME, surprising it built at all, even more that it passed tests locally. Will fix, add more tests and re-upload. Thanks Caio for your review, will answer it in details in a few hours.
Robin Morisset
Comment 15 2019-11-08 13:33:40 PST
(In reply to Caio Lima from comment #13) > Comment on attachment 383101 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=383101&action=review > > Thank you very much for doing work on this. I left some comments in the > Patch. > > > Source/JavaScriptCore/ChangeLog:11 > > + - teaching FixupPhase how to replace it by ArithAdd/ArithSub when the type is Int32/Double/BigInt > > When type is BigInt, we actually fixup this to ValueAdd/ValueSub. Fixed. > > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:935 > > + case Dec: { > > We should be able to apply constant folding when we prove that > `node->child1()` is a number constant, even when `node->child1().useKind() > == UntypedUse`. I added a FIXME to look at it later. I doubt it is going to be easy to test, since fixup phase should run before, and in the simple case will replace this by an Add or Sub. > > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:203 > > + node->setResult(NodeResultInt32); > > We can also `node->clearFlags(NodeMustGenerate);` here. It is done a few lines later, after the if/then/else. > > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:210 > > + fixEdge<BigIntUse>(node->child2()); > > And here. ditto. > > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:212 > > + > > nit: remove blank line. I also think the comment above is unnecessary. Removed the blank line, kept the comment. I generally find jsc to have too few comments, and prefer to leave comments when I see some non-obvious code; but I admit it is a personal preference. > > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:213 > > + } else { > > nit: this else is not needed. It is: the if/else if before do not end in a return! > > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:1181 > > + case Dec: > > I think rules for prediction propagation are missing there. Good catch, this is not a phase I understand well, and I had missed it. > > > Source/JavaScriptCore/jit/JITOpcodes.cpp:666 > > + auto bytecode = currentInstruction->as<OpToNumber>(); > > I think you want to do `OpToNumeric`. Good catch. > > Source/JavaScriptCore/jit/JITOpcodes.cpp:671 > > + // FIXME: fill in! > > I don't understand this FIXME here. Can you create a bug and elaborate on > that? oops, this fixme is from before I wrote the function. Removed. > > Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:844 > > + auto bytecode = currentInstruction->as<OpToNumber>(); > > Ditto. Fixed. > > Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:851 > > + SDjkadakdhsakj; > > I suspect this is a compilation error. Oops, yeah I put this in to remember to look again at this function before sending the code for review, expecting it would fail to compile. Turns out it is in an #if and as a result I forgot it in. Fixed. > > > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:464 > > + result = JSBigInt::inc(globalObject, WTF::get<JSBigInt*>(resultVariant)); > > Why do we use here `JSBigInt::inc`, wihle using `JSBigInt::add(..., > vm.bigIntConstantOne.get())` into `operationInc` and `operationDec`? Good observation, I did not yet have JSBigInt::{inc, dec} when I wrote operation{Inc, Dec}. I've fixed them to use the JSBigInt::{inc, dec} functions. This way if I later optimize Inc/Dec it will benefit them. > > Source/JavaScriptCore/runtime/VM.cpp:396 > > + bigIntConstantOne.set(*this, JSBigInt::createFrom(*this, 1)); > > What happens in case `canUseJIT() == false`? This constant is still being > used by LLInt on `JSBigInt::inc` or `JSBigInt::dec` and I suspect we will > have an error in such scenario. Great catch! I moved it out of the "if (canUseJIT())".
Robin Morisset
Comment 16 2019-11-08 14:30:44 PST
Created attachment 383164 [details] Patch Patch with fixes based on Caio's comments
Robin Morisset
Comment 17 2019-11-08 14:31:30 PST
Created attachment 383165 [details] diff Diff between the current patch and the previous one (in case it makes reviewing easier).
EWS Watchlist
Comment 18 2019-11-08 16:55:11 PST
Comment on attachment 383164 [details] Patch Attachment 383164 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/13227941 New failing tests: stress/arith-negate-on-various-types.js.default stress/arith-negate-on-various-types.js.no-cjit-collect-continuously stress/arith-negate-on-various-types.js.ftl-no-cjit-validate-sampling-profiler stress/arith-negate-on-various-types.js.no-cjit-validate-phases stress/arith-negate-on-various-types.js.ftl-no-cjit-b3o0 stress/arith-negate-on-various-types.js.ftl-no-cjit-no-inline-validate stress/arith-negate-on-various-types.js.no-ftl stress/arith-negate-on-various-types.js.no-llint
Robin Morisset
Comment 19 2019-11-11 14:26:40 PST
(In reply to Build Bot from comment #18) > Comment on attachment 383164 [details] > Patch > > Attachment 383164 [details] did not pass jsc-ews (mac): > Output: https://webkit-queues.webkit.org/results/13227941 > > New failing tests: > stress/arith-negate-on-various-types.js.default > stress/arith-negate-on-various-types.js.no-cjit-collect-continuously > stress/arith-negate-on-various-types.js.ftl-no-cjit-validate-sampling- > profiler > stress/arith-negate-on-various-types.js.no-cjit-validate-phases > stress/arith-negate-on-various-types.js.ftl-no-cjit-b3o0 > stress/arith-negate-on-various-types.js.ftl-no-cjit-no-inline-validate > stress/arith-negate-on-various-types.js.no-ftl > stress/arith-negate-on-various-types.js.no-llint I found the problem: I was wrongly using unaryArithShouldSpeculateInt32. Turns out that this function (contrary to unaryArithShouldSpeculateIn52) calls the "ForArithmetic" version of shouldSpeculateInt32OrBoolean, meaning that it proceeds by elimination and declares that anything that is not Int52 or Double must be Int32. I used it in the fixup of ArithNegate/ValueNegate, which can in theory receive any weird thing (such as a string), and this caused an unconditional speculation, leading to a recompilation loop. I have the (trivial, one-line) fix, but before I can send it back to the EWS bots, I must first fix https://bugs.webkit.org/show_bug.cgi?id=202832 that was rolled-out in the meantime.
Robin Morisset
Comment 20 2019-11-13 17:34:04 PST
Yusuke Suzuki
Comment 21 2019-11-13 18:28:55 PST
Comment on attachment 383526 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383526&action=review Nice. Early comments. > Source/JavaScriptCore/dfg/DFGOperations.cpp:499 > + JSBigInt* result = JSBigInt::inc(globalObject, WTF::get<JSBigInt*>(operandNumeric)); > + RETURN_IF_EXCEPTION(scope, encodedJSValue()); > + return JSValue::encode(result); You can do, RELEASE_AND_RETURN(scope, JSBigInt::inc(globalObject, WTF::get<JSBigInt*>(operandNumeric))); > Source/JavaScriptCore/dfg/DFGOperations.cpp:521 > + JSBigInt* result = JSBigInt::dec(globalObject, WTF::get<JSBigInt*>(operandNumeric)); > + RETURN_IF_EXCEPTION(scope, encodedJSValue()); > + return JSValue::encode(result); You can do, RELEASE_AND_RETURN(scope, JSBigInt::dec(globalObject, WTF::get<JSBigInt*>(operandNumeric))); > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:12977 > + MacroAssembler::Jump notNumber = m_jit.branchIfNotNumber(argumentRegs, scratch); Why do we go to the slow path when BigInt comes here while Baseline handles BigInt? > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:762 > break; Don't we need `ToNumeric` implementation in FTL? > Source/JavaScriptCore/jit/JITOpcodes.cpp:673 > + Jump isCell = jump(); Is `isBigInt` better? > Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:852 > + Jump isCell = jump(); Is `isBigInt` better? > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:461 > + Variant<JSBigInt*, double> resultVariant = argument.toNumeric(globalObject); Insert exception check here. > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:475 > + Variant<JSBigInt*, double> resultVariant = argument.toNumeric(globalObject); Insert exception check here. > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:597 > + Variant<JSBigInt*, double> resultVariant = argument.toNumeric(globalObject); Insert exception check here.
Robin Morisset
Comment 22 2019-11-15 13:54:36 PST
Robin Morisset
Comment 23 2019-11-15 13:55:18 PST
Thanks a lot for the review. I've applied your suggestions.
Yusuke Suzuki
Comment 24 2019-11-15 15:27:05 PST
Comment on attachment 383645 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383645&action=review > Source/JavaScriptCore/bytecode/BytecodeList.rb:355 > +op_group :ValueProfiledUnaryOp, I believe, eventually, we should convert it to UnaryArithProfile too. We do not have the case that is getting non numeric values.\ Can you add a FIXME here? > Source/JavaScriptCore/bytecode/CodeBlock.cpp:3117 > switch (pc->opcodeID()) { When using ValueProfile/ArithProfile, you also need linking code in CodeBlock::finishCreationCommon. Let's add OpInc, OpDec, OpToNumeric etc.'s handlings. I recommend grepping all the source with OpNegate / op_negate to find necessary handlings :) And IIRC we also need the other handlings when using ValueProfile. So I recommend grepping ToNumber / op_to_number to add all necessary handlings. (like, FOR_EACH_OPCODE_WITH_VALUE_PROFILE). > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:936 > + // FIXME: support some form of constant folding here. Can you add an URL to bugzilla? > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:952 > + if (BinaryArithProfile* arithProfile = m_inlineStackTop->m_profiledBlock->binaryArithProfileForBytecodeIndex(m_currentIndex)) { > + observed = arithProfile->observedResults(); > + } else if (UnaryArithProfile* arithProfile = m_inlineStackTop->m_profiledBlock->unaryArithProfileForBytecodeIndex(m_currentIndex)) { Don't need `{` and `}`. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:195 > + Node* nodeConstantOne; This should be defined in braces. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:12980 > + addSlowPathGenerator(slowPathCall(notBigInt, this, operationToNumeric, resultRegs, TrustedImmPtr::weakPointer(m_graph, m_graph.globalObjectFor(node->origin.semantic)), argumentRegs)); This slow-path-generator call returns here. So we will call operationToNumeric twice, this is not correct. We should do, JumpList slowCases; slowCases.append(m_jit.branchIfNotBigInt(argumentRegs.payloadGPR())); ... slowCases.append(m_jit.branchIfNotNumber(argumentRegs, scratch)); ... and just before calling jsValueResult, addSlowPathGenerator(slowPathCall(slowCases, this, operationToNumeric, resultRegs, TrustedImmPtr::weakPointer(m_graph, m_graph.globalObjectFor(node->origin.semantic)), argumentRegs)); > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6953 > + } We need `abstractValue(m_node->child1()).m_type & (SpecBytecodeNumber | SpecBigInt)` => false path, which should also set `setJSValue()`.
Robin Morisset
Comment 25 2019-11-18 11:42:57 PST
Robin Morisset
Comment 26 2019-11-18 11:47:24 PST
Thanks a lot for the review! (In reply to Yusuke Suzuki from comment #24) > Comment on attachment 383645 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=383645&action=review > > > Source/JavaScriptCore/bytecode/BytecodeList.rb:355 > > +op_group :ValueProfiledUnaryOp, > > I believe, eventually, we should convert it to UnaryArithProfile too. We do > not have the case that is getting non numeric values.\ > Can you add a FIXME here? We currently only specialize for Number/BigInt/Everything else, but in theory we could have special paths for string for example. I can add a FIXME if you want, but I am not entirely sure that ValueProfile is wrong here. > > Source/JavaScriptCore/bytecode/CodeBlock.cpp:3117 > > switch (pc->opcodeID()) { > > When using ValueProfile/ArithProfile, you also need linking code in > CodeBlock::finishCreationCommon. > Let's add OpInc, OpDec, OpToNumeric etc.'s handlings. > > I recommend grepping all the source with OpNegate / op_negate to find > necessary handlings :) > And IIRC we also need the other handlings when using ValueProfile. So I > recommend grepping ToNumber / op_to_number to add all necessary handlings. > (like, FOR_EACH_OPCODE_WITH_VALUE_PROFILE). I thought I had already grepped, but I visibly missed a bunch of places :-(. Thanks for noticing them. > > > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:936 > > + // FIXME: support some form of constant folding here. > > Can you add an URL to bugzilla? Done. > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:952 > > + if (BinaryArithProfile* arithProfile = m_inlineStackTop->m_profiledBlock->binaryArithProfileForBytecodeIndex(m_currentIndex)) { > > + observed = arithProfile->observedResults(); > > + } else if (UnaryArithProfile* arithProfile = m_inlineStackTop->m_profiledBlock->unaryArithProfileForBytecodeIndex(m_currentIndex)) { > > Don't need `{` and `}`. Fixed > > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:195 > > + Node* nodeConstantOne; > > This should be defined in braces. It is already in braces (around the entire Inc/Dec case). > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:12980 > > + addSlowPathGenerator(slowPathCall(notBigInt, this, operationToNumeric, resultRegs, TrustedImmPtr::weakPointer(m_graph, m_graph.globalObjectFor(node->origin.semantic)), argumentRegs)); > > This slow-path-generator call returns here. So we will call > operationToNumeric twice, this is not correct. > > We should do, > > JumpList slowCases; > slowCases.append(m_jit.branchIfNotBigInt(argumentRegs.payloadGPR())); > ... > slowCases.append(m_jit.branchIfNotNumber(argumentRegs, scratch)); > ... > > and just before calling jsValueResult, > > addSlowPathGenerator(slowPathCall(slowCases, this, operationToNumeric, > resultRegs, TrustedImmPtr::weakPointer(m_graph, > m_graph.globalObjectFor(node->origin.semantic)), argumentRegs)); Thank you for the explanation, I was super confused by this code and what addSlowPathGenerator/slowPathCall was doing. I did not realize it could return (I was seeing it as some kind of OSR exit). Fixed. > > > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6953 > > + } > > We need `abstractValue(m_node->child1()).m_type & (SpecBytecodeNumber | > SpecBigInt)` => false path, which should also set `setJSValue()`. Oops, I had planned for it and forgot. Fixed.
Robin Morisset
Comment 27 2019-11-18 17:19:33 PST
Created attachment 383813 [details] Patch simple rebase
Robin Morisset
Comment 28 2019-11-18 17:35:54 PST
Created attachment 383818 [details] Patch rebased, this time correctly.
Yusuke Suzuki
Comment 29 2019-11-18 19:42:20 PST
Comment on attachment 383818 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383818&action=review Looks good overall. I found several issues in DFG, but other place seems good! > Source/JavaScriptCore/bytecode/ArithProfile.h:84 > + ObservedResults(uint8_t bits) Put `explicit`. > Source/JavaScriptCore/bytecode/ArithProfile.h:98 > + uint8_t m_bits; I recommend using `uint8_t m_bits { 0 }`. And define default constructor like, `ObservedResults() = default;`. > Source/JavaScriptCore/bytecode/ArithProfile.h:177 > */ Let's define default constructor like, `ArithProfile() = default`. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:12966 > + DFG_ASSERT(m_jit.graph(), node, node->child1().useKind() == UntypedUse, node->child1().useKind()); > + JSValueOperand argument(this, node->child1()); > + JSValueRegsTemporary result(this, Reuse, argument); > + > + JSValueRegs argumentRegs = argument.jsValueRegs(); > + JSValueRegs resultRegs = result.regs(); > + > + argument.use(); > + > + GPRTemporary temp(this); > + GPRReg scratch = temp.gpr(); I think this is not correct. After calling `use()`, we allocate a new GPRTemporary. And we are using `Reuse` for JSValueRegsTemporary. This means that argumentRegs, resultRegs, and scratch can be the same. I recommend using simple way first, not using manual `use()` call, Reuse tag, and UseChildrenCalledExplicitly. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:12974 > + slowCases.append(m_jit.branchIfNotBigInt(argumentRegs.payloadGPR())); > + > + notCell.link(&m_jit); > + slowCases.append(m_jit.branchIfNotNumber(argumentRegs, scratch)); If `argumentRegs.payloadGPR()` is JSBigInt, it falls though here, and jumps with `m_jit.branchIfNotNumber`. So, JSBigInt is always taking a slow path.
Caio Lima
Comment 30 2019-11-19 10:33:17 PST
Comment on attachment 383818 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383818&action=review Overall LGTM as well. I left some comments about prediction propagation and fixup phase. > Source/JavaScriptCore/dfg/DFGBackwardsPropagationPhase.cpp:405 > + case ToNumeric: { I think we could also benefit from of backwards propagation for `Inc` and `Dec`. Maybe we can clear out following flags: `NodeBytecodeNeedsNegZero` or `NodeBytecodeUsesAsOther`. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:212 > + } else { It seems that we don't want to have a Int52 specialization here. Did you check if it is perf neutral? If yes, do you mind share the numbers in the bug? > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:306 > + changed |= mergePrediction(SpecInt52Any); I think that having `SpecInt52Any` propagated while not fixing up this use during fixup phase can cause some performance issues. Imagine we have the following DFG: ``` ... @10: Inc(@9:DoubleRepUse,...) // Fixup will change this to double rep use @11: ArithAdd(@10:Int52, @8:Int52) // This will cause an OSR due to BadType ... ```
Robin Morisset
Comment 31 2019-11-19 15:32:05 PST
Created attachment 383913 [details] Patch Fixing the problems pointed by Yusuke and Caio. Thanks for the reviews.
Robin Morisset
Comment 32 2019-11-19 15:32:26 PST
Created attachment 383914 [details] diff from previous patch
Yusuke Suzuki
Comment 33 2019-11-19 15:53:59 PST
Comment on attachment 383913 [details] Patch r=me
Robin Morisset
Comment 34 2019-11-19 16:51:00 PST
Comment on attachment 383913 [details] Patch This version of the patch visibly broke some of the tests.. investigating.
Robin Morisset
Comment 35 2019-11-19 18:24:13 PST
On problem is trivial: setNonCellTypeForNode instead of setTypeForNode in the default case for ToNumeric. But there is another problem, which causes segfaults, only when the FTL is on, that I am still investigating.
Robin Morisset
Comment 36 2019-11-19 18:58:48 PST
Created attachment 383934 [details] Patch This patch fixes three problems: - LowLevelAssembler64.asm used UnaryArithProfile::m_bits where it should have used the BinaryArithProfile::m_bits version. This is harmless currently (and unrelated to this patch) but would have crashed if we ever make these two have different sizes) - I had forgotten to check for isCell in FTLLowerDFGToB3::compileToNumeric, before testing for IsBigInt, causing a crash whenever doing undefined++ or null++ - I was using setNonCellTypeForNode instead of setTypeForNode for the default case in DFGAbstractInterpreterInlines.h
WebKit Commit Bot
Comment 37 2019-11-19 19:42:04 PST
Comment on attachment 383934 [details] Patch Clearing flags on attachment: 383934 Committed r252680: <https://trac.webkit.org/changeset/252680>
WebKit Commit Bot
Comment 38 2019-11-19 19:42:07 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 39 2019-11-19 19:43:20 PST
Note You need to log in before you can comment on or make changes to this bug.