Bug 192966 – DFGByteCodeParser rules for bitwise operations should consider type of their operands

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Caio Lima]

Current implementation relies into `getPrediction()` to emit ArithBitOp or ValueBitOp. This rule is not efficient because there cases where we don't have profiling information and we sendup always emitting OSRExit or ValueBitOp.

[Caio Lima]

Created attachment 357918 [details]
Patch

[Caio Lima]

Created attachment 357919 [details]
Benchmarks

This patch seems perf neutral on x86_64

[Yusuke Suzuki]

Comment on attachment 357918 [details]
Patch

r=me

[Caio Lima]

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

[Caio Lima]

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

[Caio Lima]

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

[Caio Lima]

Ping discussion on comment #5

[Caio Lima]

Created attachment 358784 [details]
Patch

[Caio Lima]

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.

[Caio Lima]

Ping Review

[Yusuke Suzuki]

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.

[Caio Lima]

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.

[Caio Lima]

Created attachment 359115 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 359115 [details]
Patch

Clearing flags on attachment: 359115

Committed r239980: <https://trac.webkit.org/changeset/239980>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47279811>