Summary: | [BigInt] Add ValueBitLShift into DFG | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Caio Lima <ticaiolima> | ||||||||||||||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Caio Lima <ticaiolima> | ||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | cdumez, commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, rniwa, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||
Bug Depends on: | 192966 | ||||||||||||||||||||||||||||||||||||
Bug Blocks: | 186173 | ||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Caio Lima
2018-12-13 04:35:32 PST
Created attachment 358148 [details]
WIP - Patch
Created attachment 358149 [details]
Benchmarks
This change is perf neutral on x86_64 macOS build.
Comment on attachment 358148 [details] WIP - Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358148&action=review quick review. The direction looks really nice. Please make the code consistent with the existing bit operations :) > Source/JavaScriptCore/dfg/DFGBackwardsPropagationPhase.cpp:123 > + case ValueBitLShift: Is it correct? If it is correct, why aren't ValueBitOr and ValueBitXor included? > Source/JavaScriptCore/dfg/DFGBackwardsPropagationPhase.cpp:223 > + case ValueBitLShift: Ditto. > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3439 > // We can use a BitLShift here because typed arrays will never have a byteLength Change this to `ArithBitLShift`. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:220 > + if (Node::shouldSpeculateUntypedForBitOps(node->child1().node(), node->child2().node())) { > + fixEdge<UntypedUse>(node->child1()); > + fixEdge<UntypedUse>(node->child2()); > + break; > + } > + > + node->setOp(ArithBitLShift); > + node->clearFlags(NodeMustGenerate); > + node->setResult(NodeResultInt32); > + fixIntConvertingEdge(node->child1()); > + fixIntConvertingEdge(node->child2()); > + break; > + } I'm not sure why this is separated from ValueBitXor, ValueBitOr and ValueBitAnd. This checks shouldSpeculateUntypedForBitOps. But ValueBitXor, ValueBitOr and ValueBitAnd do not. (In reply to Yusuke Suzuki from comment #3) > Comment on attachment 358148 [details] > WIP - Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=358148&action=review > > quick review. The direction looks really nice. Please make the code > consistent with the existing bit operations :) Thx for the review! This is the goal. I'm thinking in handle other BitOps into https://bugs.webkit.org/show_bug.cgi?id=192966 > > Source/JavaScriptCore/dfg/DFGBackwardsPropagationPhase.cpp:123 > > + case ValueBitLShift: > > Is it correct? If it is correct, why aren't ValueBitOr and ValueBitXor > included? > > > Source/JavaScriptCore/dfg/DFGBackwardsPropagationPhase.cpp:223 > > + case ValueBitLShift: > > Ditto. > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3439 > > // We can use a BitLShift here because typed arrays will never have a byteLength > > Change this to `ArithBitLShift`. > > > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:220 > > + if (Node::shouldSpeculateUntypedForBitOps(node->child1().node(), node->child2().node())) { > > + fixEdge<UntypedUse>(node->child1()); > > + fixEdge<UntypedUse>(node->child2()); > > + break; > > + } > > + > > + node->setOp(ArithBitLShift); > > + node->clearFlags(NodeMustGenerate); > > + node->setResult(NodeResultInt32); > > + fixIntConvertingEdge(node->child1()); > > + fixIntConvertingEdge(node->child2()); > > + break; > > + } > > I'm not sure why this is separated from ValueBitXor, ValueBitOr and > ValueBitAnd. > This checks shouldSpeculateUntypedForBitOps. But ValueBitXor, ValueBitOr and > ValueBitAnd do not. Ditto. Created attachment 359271 [details]
Patch
Created attachment 359272 [details]
Benchmarks
These changes seems to be perf neutral on macOS x86_ 64
Created attachment 359605 [details]
Patch
Comment on attachment 359605 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359605&action=review > Source/JavaScriptCore/dfg/DFGDoesGC.cpp:103 > + case ValueBitLShift: same comment as I've written in other bugs. Should return true if BigIntUse. Created attachment 359976 [details]
Patch
Created attachment 360009 [details]
Patch
Created attachment 360941 [details]
Patch
Created attachment 362256 [details]
Patch
Created attachment 364993 [details]
Patch
Created attachment 372229 [details]
Patch
Comment on attachment 372229 [details] Patch Attachment 372229 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12493738 New failing tests: stress/urshift-int32-overflow.js.ftl-no-cjit-b3o0 stress/urshift-int32-overflow.js.bytecode-cache stress/urshift-int32-overflow.js.ftl-no-cjit-small-pool stress/urshift-int32-overflow.js.ftl-no-cjit-no-put-stack-validate stress/urshift-int32-overflow.js.ftl-eager-no-cjit-b3o1 stress/urshift-int32-overflow.js.ftl-no-cjit-validate-sampling-profiler stress/urshift-int32-overflow.js.ftl-eager stress/urshift-int32-overflow.js.ftl-eager-no-cjit stress/urshift-int32-overflow.js.ftl-no-cjit-no-inline-validate stress/urshift-int32-overflow.js.default apiTests Created attachment 372240 [details]
Patch
Ping Review Ping Review. Comment on attachment 372240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372240&action=review > Source/JavaScriptCore/ChangeLog:15 > + not using `getHeapPrediction()` during prediction propagation, because > + it causes performance regression on Octane's zlib and mandreel. Why does this cause a regression? (In reply to Keith Miller from comment #19) > Comment on attachment 372240 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=372240&action=review > > > Source/JavaScriptCore/ChangeLog:15 > > + not using `getHeapPrediction()` during prediction propagation, because > > + it causes performance regression on Octane's zlib and mandreel. > > Why does this cause a regression? The problem is when we use getPrediction() to populate “getHeapTopPrediction()” and the instruction was never executed before, we emit a ForceOSRExit. It means that the program will have more OSRExits than desired. Such case seems to be very common for those benchmarks. Another option would be to use getPredictionWithoutOSR(), but in this case we will have Untyped as the prediction for instructions never executed. This means that we will stop the prediction propagation for those instructions and will emit ValueBitLShift in cases where it would totally possible to speculate Int32 or other types. IIRC, the best approach is then use the instruction inputs to propagate the predictions. Ping Comment on attachment 372240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372240&action=review > Source/JavaScriptCore/bytecode/BytecodeList.rb:263 > + :lshift, Where are you actually using the value profile? > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:182 > + case ValueBitLShift: { Did you mean to make this just use the value profile? > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:190 > + else if (isFullNumberOrBooleanSpeculationExpectingDefined(left) && isFullNumberOrBooleanSpeculationExpectingDefined(right)) > + changed |= mergePrediction(SpecInt32Only); shouldn't this just be something like: if ((!left & SpecBigInt) && !(right & SpecBigInt)) Since this will always return int32 unless the input might be BigInt? Or maybe ValueOf allows you to return BigInt and therefore I'm wrong? Thx a lot for the feedback. (In reply to Saam Barati from comment #22) > Comment on attachment 372240 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=372240&action=review > > > Source/JavaScriptCore/bytecode/BytecodeList.rb:263 > > + :lshift, > > Where are you actually using the value profile? IIRC, the idea of keeping ValueProfile on this instruction was to get predictions injected during PredictionInjectionPhase and potentially making `PredictionPropagationPhase` converge in less iterations. However, I think the predictions are never injected for those nodes. Even though, leaving this profiler there don't make much sense anymore and I think we should remove it. > > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:182 > > + case ValueBitLShift: { > > Did you mean to make this just use the value profile? Since this version gave up on using getHeapTopPrediction(), I don't think we need to have ValueProfile anymore. > > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:190 > > + else if (isFullNumberOrBooleanSpeculationExpectingDefined(left) && isFullNumberOrBooleanSpeculationExpectingDefined(right)) > > + changed |= mergePrediction(SpecInt32Only); > > shouldn't this just be something like: > if ((!left & SpecBigInt) && !(right & SpecBigInt)) > > Since this will always return int32 unless the input might be BigInt? Or > maybe ValueOf allows you to return BigInt and therefore I'm wrong? Yes, `valueOf` can return BigInt and, as you mentioned, `if ((!left & SpecBigInt) && !(right & SpecBigInt))` don't catch those cases. Created attachment 373741 [details]
Patch
(In reply to Caio Lima from comment #24) > Created attachment 373741 [details] > Patch In this Patch I removed the ValueProfile of ValueBitLShift and also added a test case to cover when BigInt can be a return of a `valueOf` method. Comment on attachment 373741 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373741&action=review > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:192 > + else > + changed |= mergePrediction(SpecBigInt | SpecInt32Only); btw, this is where the value profile would help you out. You could make this mergePrediction(getHeapPrediction()) Created attachment 373840 [details]
Patch
Created attachment 373841 [details]
Benchmarks
Benchmark results for the new version of the Patch. Perf looks neutral.
(In reply to Saam Barati from comment #26) > Comment on attachment 373741 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=373741&action=review > > > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:192 > > + else > > + changed |= mergePrediction(SpecBigInt | SpecInt32Only); > > btw, this is where the value profile would help you out. You could make this > mergePrediction(getHeapPrediction()) Thx a lot for this advice! I uploaded a new version using this idea and there is no performance regression on last report. Comment on attachment 373840 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373840&action=review > Source/JavaScriptCore/ChangeLog:14 > + not using `getHeapPrediction()` during prediction propagation, because not true anymore Created attachment 374006 [details]
Patch
Comment on attachment 374006 [details] Patch Clearing flags on attachment: 374006 Committed r247387: <https://trac.webkit.org/changeset/247387> All reviewed patches have been landed. Closing bug. (In reply to Saam Barati from comment #30) > Comment on attachment 373840 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=373840&action=review > > > Source/JavaScriptCore/ChangeLog:14 > > + not using `getHeapPrediction()` during prediction propagation, because > > not true anymore Thx a lot for the reviews! |