Bug 186228

Created attachment 341869 [details] Patch

Comment on attachment 341869 [details] Patch Attachment 341869 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7964253 New failing tests: http/tests/preload/onload_event.html

Created attachment 341875 [details] Archive of layout-test-results from ews205 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Comment on attachment 341869 [details] Patch Previously, op_bitand never returns value except for Int32. But this assumption is broken now. We should do arith-profile things well to emit BitAnd effectively in DFG. This means we should profile values (result, and operands) carefully in LLInt and Baseline as the other arithmetic operations do. And please measure the performance once it is done :) And I think we should rename DFG bit op nodes from BitAnd to ArithBitAnd since later we will add ValueBitAnd. (And we should rename other bit nodes too).

Created attachment 342234 [details] Patch

(In reply to Yusuke Suzuki from comment #4) > Comment on attachment 341869 [details] > Patch > > Previously, op_bitand never returns value except for Int32. But this > assumption is broken now. We should do arith-profile things well to emit > BitAnd effectively in DFG. > This means we should profile values (result, and operands) carefully in > LLInt and Baseline as the other arithmetic operations do. > And please measure the performance once it is done :) > > And I think we should rename DFG bit op nodes from BitAnd to ArithBitAnd > since later we will add ValueBitAnd. (And we should rename other bit nodes > too). Cool. I decided to add ValueProfile to op_bitand after talking with Saam. We are profiling operands in op_add, op_sub, etc, because we have IC for such cases. But I don't think there is a necessity to add IC for op_bitand now and on DFG and FTL we already can get operands type prediction with the prediction propagation. Also, I'm just profiling when the result is BigInt, otherwise, it needs to be Int32. Unfortunately, it made the Patch way more big to be reviewed.

Created attachment 342235 [details] Benchmarks Report Based on benchmark results in my machine, this Patch is perf neutral.

Created attachment 342236 [details] Patch

Attachment 342236 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:424: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 3 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 342236 [details] Patch Attachment 342236 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/8072986 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

Created attachment 342243 [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

Comment on attachment 342236 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342236&action=review After looking the current implementation, I think we should show the goal of DFG / FTL implementation first before starting the implementation of bit operations, since it may involve how to profile things effectively. I think completing DFG / FTL implementations for binary operations (like add, sub etc.) first is better to show the goal of the design and implementation for binary operations with BigInt. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1747 > + if (opcodeID == op_bitand) { > + UnlinkedValueProfile profile = emitProfiledOpcode(opcodeID); > + instructions().append(dst->index()); > + instructions().append(src1->index()); > + instructions().append(src2->index()); > + instructions().append(profile); > + return dst; > + } Why not using ArithProfile, which is already emitted in the existing code? To clarify whether ArithProfile is enough, I think completing add / sub etc. for BigInt in DFG and FTL first is better. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1756 > opcodeID == op_add || opcodeID == op_mul || opcodeID == op_sub || opcodeID == op_div) > instructions().append(ArithProfile(types.first(), types.second()).bits()); We already emit ArithProfile for op_bitand. I think what we should do is record necessary information in this profile for op_bitand. > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4641 > + if (getPredictionWithoutOSRExit() != SpecBigInt) > + set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(ArithBitAnd, op1, op2)); > + else > + set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(ValueBitAnd, op1, op2)); > + I think relying on ArithProfile is better since it is the same to the other binary operations including op_sub.

Thank you for the feedback. (In reply to Yusuke Suzuki from comment #12) > Comment on attachment 342236 [details] > Patch > > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1747 > > + if (opcodeID == op_bitand) { > > + UnlinkedValueProfile profile = emitProfiledOpcode(opcodeID); > > + instructions().append(dst->index()); > > + instructions().append(src1->index()); > > + instructions().append(src2->index()); > > + instructions().append(profile); > > + return dst; > > + } > > Why not using ArithProfile, which is already emitted in the existing code? > To clarify whether ArithProfile is enough, I think completing add / sub etc. > for BigInt in DFG and FTL first is better. ArithProfile is much more complex to this use case IMO. While op_add or op_sub profiles their operands for IC and also profile the result to check overflow or negative zero, bitwise operations never have these outcomes. Introducing BigInt will only make bitwise operations return In32 or BigInt, and to return BigInt, both operands need to be be BigInt after ToNumeric operation, which restricts the possibility of operands to be JSCell (but not string or symbol) or BigInt. With ValueProfile we already are able to get the result type as feedback. Maybe I'm missing something here, but my idea is to specialize these nodes for BigInt into DFG and FTL calling slow paths, in the first moment, and then emmiting code that is able to unbox BigInts and perform operations directly in binary. If I'm not wrong, this requires the type information of operands that is already accessible on DFG and FTL. It is not clear to me why implement op_sub first is more important the bitwise operations. Do you mind explain why we should follow this path? > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4641 > > + if (getPredictionWithoutOSRExit() != SpecBigInt) > > + set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(ArithBitAnd, op1, op2)); > > + else > > + set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(ValueBitAnd, op1, op2)); > > + > > I think relying on ArithProfile is better since it is the same to the other > binary operations including op_sub. I mentioned above why I think ValueProfile make more sense here. AFAIC, op_sub is just emitting ArithSub right now. However, we have ArithNegate and ArithAdd that decides to emit ValueAdd or AirthAdd depending of operands type. However, they doesn't use ArithProfile on "DFGByteCodeParser.cpp". They rely on each operand's node type to emit proper node.

Created attachment 342259 [details] Patch

This version is adding the BigInt speculation code generation into DFG and FTL.

Attachment 342259 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:424: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 3 in 35 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 342259 [details] Patch Attachment 342259 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/8081833 New failing tests: stress/ftl-put-by-id-setter-exception-interesting-live-state.js.ftl-eager-no-cjit

Comment on attachment 342259 [details] Patch Attachment 342259 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/8082131 New failing tests: http/tests/resourceLoadStatistics/partitioned-and-unpartitioned-cookie-with-partitioning-timeout.html

Created attachment 342278 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4

Ping

Ping Review

Ping Review

Ping Review here. I also started the implementation of ValueSub into https://bugs.webkit.org/show_bug.cgi?id=186176. I think the last review Yusuke mentioned that we should implement such support before, but comparing this patch with ValueBitAnd JIT implementation, they does not see related. @Yusuke, What do you think about it?

Created attachment 344302 [details] Patch Rebasing Patch.

Attachment 344302 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:424: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 3 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.

Ping

Created attachment 344566 [details] Patch

Attachment 344566 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:424: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 3 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 350526 [details] Patch Patch rebased

Attachment 350526 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:426: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 3 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 350532 [details] Patch

Attachment 350532 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:426: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 3 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.

Ping review

Comment on attachment 350532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350532&action=review Looks nice. I would like to have one more round of reviews. > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4848 > + if (getPredictionWithoutOSRExit() != SpecBigInt) If `type = SpecBigInt | SpecInt32Only`, it goes to `ArithBitAnd`. It is not desirable result. We should use `isInt32Speculation(getPredictionWithoutOSRExit())` => ArithBitAnd. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:103 > + if (Node::shouldSpeculateBigInt(node->child1().node(), node->child1().node())) { Both operands use `child1()`. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:121 > + if (Node::shouldSpeculateUntypedForBitOps(node->child1().node(), node->child2().node())) { > + fixEdge<UntypedUse>(node->child1()); > + fixEdge<UntypedUse>(node->child2()); > + node->setOp(ValueBitAnd); > + node->setResult(NodeResultJS); > + break; > + } If the type is `ValueBitAnd`, it should be `NodeMustBeGenerate`. You need to mark it as `NodeMustBeGenerate`. You can use `setOpAndDefaultFlags(ValueBitAnd)` instead of setOp(ValueBitAnd) and setResult(NodeResultJS);. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3613 > + GPRTemporary result(this); Remove this. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3626 > + silentSpillAllRegisters(resultGPR); > + callOperation(operationBitAndBigInt, resultGPR, leftGPR, rightGPR); > + silentFillAllRegisters(); > + m_jit.exceptionCheck(); > + > + cellResult(resultGPR, node); In such case, you can use, flushRegisters(); GPRFlushedCallResult result(this); GPRReg resultGPR = result.gpr(); callOperation(operationBitAndBigInt, resultGPR, leftGPR, rightGPR); m_jit.exceptionCheck(); cellResult(resultGPR, node); > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:655 > + case DFG::ArithBitAnd: Can we just use `ArithBitAnd` here? (b/c it does not conflict with B3 BitAnd). > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2821 > + Remove this line. > Source/JavaScriptCore/runtime/JSBigInt.cpp:98 > + bigInt->setSign(false); This should be done in JSBigInt::JSBigInt constructor. , m_sign(false) > Source/JavaScriptCore/runtime/JSCJSValueInlines.h:756 > + if (isInt32()) > + return asInt32(); What happens if the `this` is double and it is in range of int32? (e.g. 1.0). I think we can have `if (isDouble())` path too. `canBeInt32` function can be used for this case.

Comment on attachment 350532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350532&action=review > Source/JavaScriptCore/ChangeLog:61 > + * llint/LowLevelInterpreter32_64.asm: Let's add LLInt change. It is required since now op_bitand starts using ValueProfile instead of ArithProfile.

Comment on attachment 350532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350532&action=review >> Source/JavaScriptCore/ChangeLog:61 >> + * llint/LowLevelInterpreter32_64.asm: > > Let's add LLInt change. It is required since now op_bitand starts using ValueProfile instead of ArithProfile. And we also need Baseline JIT Change for ValueProfile.

Comment on attachment 350532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350532&action=review Thank you very much for the review. >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4848 >> + if (getPredictionWithoutOSRExit() != SpecBigInt) > > If `type = SpecBigInt | SpecInt32Only`, it goes to `ArithBitAnd`. It is not desirable result. > We should use `isInt32Speculation(getPredictionWithoutOSRExit())` => ArithBitAnd. Hum...That's a good edge case: ``` function foo(a, b) { return a & b } noInline(foo); for (let i = 0; i < 10000; i++) { if (i % 2) { foo(3, 4); } else { foo(3n, 4n); } } ``` Right now, we only profile BigInt results, which means that the profile will say that only BigInt was observed as outcome of "op_bitand", but it is not true. I will change that. >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:103 >> + if (Node::shouldSpeculateBigInt(node->child1().node(), node->child1().node())) { > > Both operands use `child1()`. Oops >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:121 >> + } > > If the type is `ValueBitAnd`, it should be `NodeMustBeGenerate`. You need to mark it as `NodeMustBeGenerate`. You can use `setOpAndDefaultFlags(ValueBitAnd)` instead of setOp(ValueBitAnd) and setResult(NodeResultJS);. Changed. >> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:756 >> + return asInt32(); > > What happens if the `this` is double and it is in range of int32? (e.g. 1.0). > I think we can have `if (isDouble())` path too. `canBeInt32` function can be used for this case. 1.0 goes through ```isInt32()``` path. However, 223.0 fallback to slow path. I'm adding the case when we have double that canBeInt32.

Created attachment 351082 [details] Patch

Attachment 351082 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:426: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 3 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 351082 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351082&action=review r=me with fixes. > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4848 > + if (isInt32Speculation(getPredictionWithoutOSRExit())) Use `getPrediction()`. We always profile the value appropriately. > Source/JavaScriptCore/jit/JIT.h:797 > + void emitBitBinaryOpFastPath(Instruction* currentInstruction, bool shouldEmitProfiling = false); Let's have `enum class` for profiling mode instead of `bool`. > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1162 > + valueProfile(t0, t3, 16, t2) valueProfile's argument seems wrong. `valueProfile(tag, payload, operand, scratch)`. And can we make `16` `((advance - 1) * 4)`? > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1106 > + valueProfile(t0, 4, t2) Can we use `(advance - 1)` instead of `4`? > Source/JavaScriptCore/runtime/JSBigInt.cpp:112 > } else { > bigInt->setDigit(0, static_cast<Digit>(value)); > - bigInt->setSign(false); > } Remove `{` and `}`. > Source/JavaScriptCore/runtime/JSBigInt.cpp:140 > } else { > bigInt->setDigit(0, static_cast<Digit>(value)); > - bigInt->setSign(false); > } Remove `{` and `}`. > Source/JavaScriptCore/runtime/JSCJSValueInlines.h:758 > + if (isDouble() && canBeInt32(asDouble())) > + return JSC::toInt32(asDouble()); I think `return static_cast<int32_t>(asDouble());` is OK since canBeInt32 goes well. Other cases go to the generic path below.

Created attachment 351167 [details] Patch

Comment on attachment 351167 [details] Patch Thank you very much for the review @Yusuke!

Attachment 351167 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:426: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 3 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 351167 [details] Patch Clearing flags on attachment: 351167 Committed r236637: <https://trac.webkit.org/changeset/236637>

All reviewed patches have been landed. Closing bug.

<rdar://problem/44882601>

Created attachment 351214 [details] benchmarks Patch is perf neutral.