RFR: 8320330: Improve implementation of RShift Value
Christian Hagedorn
chagedorn at openjdk.org
Mon Nov 27 08:17:04 UTC 2023
On Mon, 20 Nov 2023 14:12:01 GMT, Jasmine Karthikeyan <jkarthikeyan at openjdk.org> wrote:
> Hi, I've created this patch to improve the Value() implementation for right shift nodes. The primary change is preserving sign information when calculating the value, even if we can't come to a precise conclusion-- when looking at the right shift `X >> Y`, if `type(X)` is `>=0` then we can know that `type(X >> Y)` can be at most `>=0`, regardless of what `type(Y)` is. The inverse is also true for the `<0` case. Previously, these cases would have returned the entire int/long range as the set of possible values. I tried to find a more general way to approach this transform but the wrapping behavior of shifts made it hard to reason about and I didn't find many cases that would benefit from it, so I've decided to keep it simple.
>
> I also slightly changed the logic that shifts the hi and lo ranges of the type to only operate on non-constant left-hand values, as the case where both are constant can be better handled by the end case that simply shifts the constant. The long variant has also been changed to better match the int version as well.
>
> A common place to find this pattern is String#length:
> https://github.com/openjdk/jdk/blob/db1d82347bb18e21c4c6a18076ffdaf17724c733/src/java.base/share/classes/java/lang/String.java#L1520-L1522
>
> As it's shifting an array length, we should now be able to statically prove that the result will also be `>=0`. I ran some of the existing benchmarks and saw some nice improvements in the realm of string concatenation, due to the sharpened type allowing a branch to become dead:
>
> Baseline Patch Improvement
> Benchmark (intValue) Mode Cnt Score Error Units Score Error Units
> StringConcat.concatConst4String 4711 avgt 15 23.471 ± 0.916 ns/op 21.630 ± 0.093 ns/op (+ 8.16%)
> StringConcat.concatConst6String 4711 avgt 15 29.386 ± 0.454 ns/op 28.292 ± 0.127 ns/op (+ 3.79%)
> StringConcat.concatConstString 4711 avgt 15 6.880 ± 0.051 ns/op 6.606 ± 0.021 ns/op (+ 4.06%)
> StringConcat.concatMethodConstString 4711 avgt 15 6.881 ± 0.029 ns/op 6.665 ± 0.069 ns/op (+ 3.18%)
>
>
> I've also attached an IR test, and tier1-3 testing passes on my linux x64 machine. Reviews would be appreciated!
Otherwise, looks good to me, too!
src/hotspot/share/opto/mulnode.cpp line 1411:
> 1409: }
> 1410:
> 1411: return TypeLong::make((jlong)r1->get_con() >> (r2->get_con() & 63));
The cast to `jlong` can be removed since `get_con()` already returns a `jlong`.
Suggestion:
return TypeLong::make(r1->get_con() >> (r2->get_con() & 63));
-------------
Marked as reviewed by chagedorn (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16736#pullrequestreview-1749848908
PR Review Comment: https://git.openjdk.org/jdk/pull/16736#discussion_r1405781050
More information about the hotspot-compiler-dev
mailing list