RFR: 8346664: C2: Optimize mask check with constant offset [v3]

[Emanuel Peter]

On Mon, 20 Jan 2025 08:04:23 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 one additional commit since the last revision:
> 
>   (c)

@mernst-github sorry, dropped this for a few days. I'm currently reviewing about 10 PR's in parallel 🙈

src/hotspot/share/opto/mulnode.hpp line 87:

> 85: 
> 86: protected:
> 87:   Node* AndIL_sum_and_mask(PhaseGVN* phase, BasicType bt);

Can you please update the copyright from 2024 -> 2025?

test/hotspot/jtreg/compiler/c2/irTests/TestShiftAndMask.java line 47:

> 45: 
> 46:     // any X << INT_MASK_WIDTH is zero under any INT_MASK
> 47:     static final int INT_MASK_WIDTH = 1 + RANDOM.nextInt(30);

I wonder if we should additionally have some tests where the mask can have arbitrary width. This may not allow us to do IR rules, but it would ensure we get the correct values.

Because you have some assert above that checks that the shift should have been masked already, for example. It would be good to have a corresponding test.

-------------

PR Review: https://git.openjdk.org/jdk/pull/22856#pullrequestreview-2569097873
PR Review Comment: https://git.openjdk.org/jdk/pull/22856#discussion_r1926515635
PR Review Comment: https://git.openjdk.org/jdk/pull/22856#discussion_r1926522378