Bug 193240

Created attachment 358966 [details] WIP - Patch

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.

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.

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

Created attachment 359025 [details] WIP - Patch

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.

Created attachment 359035 [details] WIP - Patch

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.

Created attachment 382769 [details] WIP Still missing tests.

Created attachment 383081 [details] Patch

Created attachment 383101 [details] Patch rebased now that the ArithProfile patch is back in.

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

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.

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.

(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())".

Created attachment 383164 [details] Patch Patch with fixes based on Caio's comments

Created attachment 383165 [details] diff Diff between the current patch and the previous one (in case it makes reviewing easier).

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

(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.

Created attachment 383526 [details] Patch

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.

Created attachment 383645 [details] Patch

Thanks a lot for the review. I've applied your suggestions.

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()`.

Created attachment 383768 [details] Patch

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.

Created attachment 383813 [details] Patch simple rebase

Created attachment 383818 [details] Patch rebased, this time correctly.

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.

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 ... ```

Created attachment 383913 [details] Patch Fixing the problems pointed by Yusuke and Caio. Thanks for the reviews.

Created attachment 383914 [details] diff from previous patch

Comment on attachment 383913 [details] Patch r=me

Comment on attachment 383913 [details] Patch This version of the patch visibly broke some of the tests.. investigating.

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.

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

Comment on attachment 383934 [details] Patch Clearing flags on attachment: 383934 Committed r252680: <https://trac.webkit.org/changeset/252680>

All reviewed patches have been landed. Closing bug.

<rdar://problem/57346871>