RFR: 8346664: C2: Optimize mask check with constant offset [v4]
Emanuel Peter
epeter at openjdk.org
Thu Jan 23 10:10:53 UTC 2025
On Thu, 23 Jan 2025 09:06:09 GMT, Matthias Ernst <duke at openjdk.org> wrote:
>> Fixes [JDK-8346664](https://bugs.openjdk.org/browse/JDK-8346664): extends the optimization of masked sums introduced in #6697 to cover constant values, which currently break the optimization.
>>
>> Such constant values arise in an expression of the following form, for example from `MemorySegmentImpl#isAlignedForElement`:
>>
>>
>> (base + (index + 1) << 8) & 255
>> => MulNode
>> (base + (index << 8 + 256)) & 255
>> => AddNode
>> ((base + index << 8) + 256) & 255
>>
>>
>> Currently, `256` is not being recognized as a shifted value. This PR enables further reduction:
>>
>>
>> ((base + index << 8) + 256) & 255
>> => MulNode (this PR)
>> (base + index << 8) & 255
>> => MulNode (PR #6697)
>> base & 255 (loop invariant)
>>
>>
>> Implementation notes:
>> * I verified that the originating issue "scaled varhandle indexed with i+1" (https://mail.openjdk.org/pipermail/panama-dev/2024-December/020835.html) is resolved with this PR.
>> * ~in order to stay with the flow of the current implementation, I refrained from solving general (const & mask)==0 cases, but only those where const == _ << shift.~
>> * ~I modified existing test cases adding/subtracting from the index var (which would fail with current C2). Let me know if would like to see separate cases for these.~
>
> Matthias Ernst has updated the pull request incrementally with two additional commits since the last revision:
>
> - JLS: only the lower bits of the shift are taken into account (aka we don't assert).
> - (c)
Actually, it would be fantastic if you could use our new `Generators.java` for the random value generation. The idea is that normal `Random.nextInt` generates the edge-case values like power-of-2 very rarely, and for `Random.nextLong` that is even less likely to happen. But here, masks are values that are close to powers-of-2 (often 64-1 or 128-1 etc). With our `Generators.java` we generate such specual values more often, which could trigger edge cases, and then find more bugs / give better coverage.
See `./test/hotspot/jtreg/compiler/lib/generators/Generators.java`
test/hotspot/jtreg/compiler/c2/irTests/TestShiftAndMask.java line 180:
> 178: @IR(counts = { IRNode.ADD_I, "1", IRNode.LSHIFT_I, "1" })
> 179: public static int addShiftPlusConstOverlyLargeShift(int i, int j) {
> 180: return (j + i << 129) & 32; // NOT transformed, only lower 5 bits of shift count.
Can we have a test where both the shift and the mask are completely random values, and do result verification?
-------------
Changes requested by epeter (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/22856#pullrequestreview-2569407314
PR Review Comment: https://git.openjdk.org/jdk/pull/22856#discussion_r1926703905
More information about the hotspot-compiler-dev
mailing list