Bug 192966

Created attachment 357918 [details] Patch

Created attachment 357919 [details] Benchmarks This patch seems perf neutral on x86_64

Comment on attachment 357918 [details] Patch r=me

(In reply to Yusuke Suzuki from comment #3) > Comment on attachment 357918 [details] > Patch > > r=me Thx for the review. I'm investigating a regression into zlib before landing.

(In reply to Caio Lima from comment #4) > (In reply to Yusuke Suzuki from comment #3) > > Comment on attachment 357918 [details] > > Patch > > > > r=me > > Thx for the review. I'm investigating a regression into zlib before landing. We had a false alarm into zlib, but there is an odd case into Microbenchmark/slow-ternary. The problem is when we compile a function without executing some bitwise op. Since on ToT we emit ArithBitOp using ```getPrediction()```, generated code end up emitting ForceOSRExit when there is no profiling info. Because of this, we don't emit the rest of code followed by this block and the final version of the code is much simpler than the version we generated with this patch. Even though the ToT compiled code has an ForceOSRexit, the function runs almost all iterations before it fallback to Baseline JIT, and since the code is smaller, I think this makes ToT version run faster. I personally think that such regression comparing ToT and changes is acceptable, since our current rules emitting AirthBitOp and ValueBitOp are wrong, mainly because we don't benefit from prediction propagation on cases that profile is inexistent. What do you think about this result? I'm going to wait for your comments before moving forward.

(In reply to Caio Lima from comment #5) > > We had a false alarm into zlib, but there is an odd case into > Microbenchmark/slow-ternary. The problem is when we compile a function > without executing some bitwise op. Since on ToT we emit ArithBitOp using > ```getPrediction()```, generated code end up emitting ForceOSRExit when > there is no profiling info. Because of this, we don't emit the rest of code > followed by this block and the final version of the code is much simpler > than the version we generated with this patch. Even though the ToT compiled > code has an ForceOSRexit, the function runs almost all iterations before it > fallback to Baseline JIT, and since the code is smaller, I think this makes > ToT version run faster. To illustrate better what I'm saying, here is the DFG code generated by ToT version (https://pastebin.com/3XEcYw7x) and here is DFG code generated with proposed changes (https://pastebin.com/MFRqGwvu). We can find the difference on Block #3 (line 110 and 118 in each file) where ToT version has ForceOSRExit, while changes version keeps generating code properly. ToT version removes Block #4-#9 while our changes version generates almost all of them.

Ping discussion on comment #5

Created attachment 358784 [details] Patch

Created attachment 358786 [details] Benchmarks According this report, these changes are perf neutral. The changes comparing to previous version is that we are using getHeapPrediction() to set prediction propagation of ValueBitOp.

Ping Review

Comment on attachment 358784 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358784&action=review r=me > Source/JavaScriptCore/ChangeLog:11 > + for ValueBitOp to use `getHeapPrediction()`. Can we have a test to ensure that JSC does not perform compile-fail loops? Like the following code would pose the issue if something is wrong. var object = { valueOf() { return integer; } }; // object with bit operation, which will produce Int32, but the operands of bit operation includes object.

Comment on attachment 358784 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358784&action=review Thank you very much for the review! >> Source/JavaScriptCore/ChangeLog:11 >> + for ValueBitOp to use `getHeapPrediction()`. > > Can we have a test to ensure that JSC does not perform compile-fail loops? > Like the following code would pose the issue if something is wrong. > > var object = { valueOf() { return integer; } }; > // object with bit operation, which will produce Int32, but the operands of bit operation includes object. Nice, added.

Created attachment 359115 [details] Patch

Comment on attachment 359115 [details] Patch Clearing flags on attachment: 359115 Committed r239980: <https://trac.webkit.org/changeset/239980>

All reviewed patches have been landed. Closing bug.

<rdar://problem/47279811>