RFR: 8347645: C2: XOR bounded value handling blocks constant folding [v7]
Emanuel Peter
epeter at openjdk.org
Mon Jan 27 08:26:52 UTC 2025
On Sat, 25 Jan 2025 16:23:00 GMT, Johannes Graham <duke at openjdk.org> wrote:
>> C2 does not eliminate XOR nodes with constant arguments. This has a noticeable effect on `Long.expand` with a constant mask, on architectures that don't have instructions equivalent to `PDEP` to be used in an intrinsic.
>>
>> This patch demonstrates a potential fix to the problem, but there might well be better ways to do it.
>
> Johannes Graham has updated the pull request incrementally with three additional commits since the last revision:
>
> - formatting
> - simplified version of bounds check
> - tests for xor hi=power of 2
@j3graham Thanks for the updates! It's looking better already.
I think the VM code is generally looking quite reasonalbe (modulo some formatting and comments).
We need to work in improving the tests a little, especially because you changed the VM code. I left some comments below :)
Good work, keep it up!
src/hotspot/share/opto/addnode.cpp line 995:
> 993:
> 994: if (r0->is_con() && r1->is_con()) {
> 995: // just XOR them bits.
Suggestion:
// Constant fold: (c1 ^ c2) -> c3
"just XOR them bits." sounds grammatically incorrect anyway, but I'm not a native speaker 😅
src/hotspot/share/opto/addnode.cpp line 999:
> 997: }
> 998:
> 999: // not constants
Well I suppose one of them could still be a constant... You should make your comments here a little more precise ;)
src/hotspot/share/opto/addnode.cpp line 1005:
> 1003:
> 1004: if (r0->_lo >= 0 && r1->_lo >= 0) {
> 1005: // x ^ y cannot have any bit set that is higher than both the highest bits set in x and y
You should have a 2-indentation here, not 4-indentation ;)
src/hotspot/share/opto/addnode.cpp line 1010:
> 1008:
> 1009: // note cast to unsigned happens before +1 to avoid signed overflow, and
> 1010: // round_up is safe because high bit is unset (0 <= lo <= hi)
Suggestion:
// Note: cast to unsigned happens before +1 to avoid signed overflow, and
// round_up is safe because high bit is unset (0 <= lo <= hi)
src/hotspot/share/opto/addnode.cpp line 1011:
> 1009: // note cast to unsigned happens before +1 to avoid signed overflow, and
> 1010: // round_up is safe because high bit is unset (0 <= lo <= hi)
> 1011: juint max = round_up_power_of_2(juint(r0->_hi | r1->_hi) + 1) - 1;
Ok, changes like these will require a lot more testing. Now we have to deal with all sorts of ranges. Imagine we now have some tighter range `r = [a..b]`, but we get it off a little. Then a if-branch could wrongly constant fold badly:
`if (a < r)`
This would seem always `false`.
So we need some Java tests that validate the ranges we produce here.
src/hotspot/share/opto/addnode.cpp line 1016:
> 1014: }
> 1015:
> 1016: return TypeInt::INT; // Any integer, but still no symbols.
I know you copied this. But what does it mean "no symbols"? Feel free to just remove the comment, it's not really helpful 😅
Suggestion:
return TypeInt::INT;
src/hotspot/share/opto/addnode.cpp line 1026:
> 1024:
> 1025: if (r0->is_con() && r1->is_con()) {
> 1026: // just XOR them bits.
Suggestion:
// Constant fold: (c1 ^ c2) -> c3
src/hotspot/share/opto/addnode.cpp line 1040:
> 1038: // y cannot have any bit set that is higher than the highest bit set in r1->_hi
> 1039:
> 1040: // we want to find a value that has all 1 bits everywhere up to and including
Can you please capitalize the first word in your sentences? ;)
test/hotspot/jtreg/compiler/c2/irTests/XorINodeIdealizationTests.java line 36:
> 34: */
> 35: public class XorINodeIdealizationTests {
> 36: private static final int CONST_1 = RunInfo.getRandom().nextInt();
You'll also have to update the copyright of the tests here ;)
test/hotspot/jtreg/compiler/c2/irTests/XorINodeIdealizationTests.java line 240:
> 238: @IR(failOn = {IRNode.XOR})
> 239: @IR(counts = {IRNode.CON_I, "1"})
> 240: // Checks (c ^c) => c (constant folded)
Suggestion:
// Checks (c1 ^ c2) => c3 (constant folded)
test/hotspot/jtreg/compiler/c2/irTests/XorINodeIdealizationTests.java line 256:
> 254: @IR(failOn = {IRNode.XOR})
> 255: @IR(counts = {IRNode.CON_I, "1"})
> 256: // Checks (c ^c) => c (constant folded)
Suggestion:
// Checks (c1 ^ c2) => c3 (constant folded)
test/hotspot/jtreg/compiler/c2/irTests/XorINodeIdealizationTests.java line 273:
> 271: private static int forceMinMax(int value){
> 272: // equivalent to Math.min(CONST_POW_2, Math.max(value, 1))
> 273: return 1 + (value & (CONST_POW_2 - 1));
Why do you chose the complicated way here? What's wrong with using `min / max`? I'm afraid that the XOR gets folded through the ADD or AND here.... I think that would be less likely with `min / max`, right?
test/hotspot/jtreg/compiler/c2/irTests/XorLNodeIdealizationTests.java line 39:
> 37: private static final long CONST_1 = RunInfo.getRandom().nextLong();
> 38: private static final long CONST_2 = RunInfo.getRandom().nextLong();
> 39: private static final long CONST_POW_2 = Math.abs(1L << RunInfo.getRandom().nextInt());
Can you please use our new `Generators.java`? I'll explain why:
Sampling random powers-of-2, or values near powers-of-2 is very very rare with longs. There are about 64 powers-of-2 in a set of `2^64`, so the odds are really really bad. The sampler in `Generators.java` has a bias to pick more powers-of-2 and values nearby.
test/hotspot/jtreg/compiler/c2/irTests/XorLNodeIdealizationTests.java line 284:
> 282: long xor = x ^ y;
> 283: return xor < (CONST_POW_2*2L);
> 284: }
Thanks for adding range-tests like these!
I think we need some smarter tests on top of these though.
For one, we should have some camped ranges that are not restricted to `[1,CONST_POW_2]`, but have any lower and upper limit.
Further: we should check with `if` branches if the resulting range is correct. Not that we get the resulting range off-by-1 and have some edge case that constant-folds the wrong way ;)
-------------
PR Review: https://git.openjdk.org/jdk/pull/23089#pullrequestreview-2574723412
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1930104525
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1930108532
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1930118801
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1930109363
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1930114433
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1930120014
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1930120395
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1930121107
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1930128932
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1930097764
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1930098181
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1930100920
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1930124895
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1930128460
More information about the hotspot-compiler-dev
mailing list