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

[Emanuel Peter]

On Sun, 19 Jan 2025 16:20:51 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:
> 
>   Assert that MulNode::Ideal already masks constant shift amounts for us.
>   Avoid accidental zero mask breaking test.

Thanks for your patience with the OCA @mernst-github !

First little comment: you will need to update the copyright year to 2025 in the files :)

I'm looking at the VM changes now...

src/hotspot/share/opto/mulnode.cpp line 673:

> 671: }
> 672: 
> 673: static bool AndIL_is_zero_element_under_mask(const PhaseGVN* phase, const Node* expr, const Node* mask, BasicType bt);

Why do you split declaration and definition? Could the body just be moved up here?

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

> 590:         int i = RANDOM.nextInt((Integer.MAX_VALUE - 1) >> INT_MASK_WIDTH);
> 591:         int j = RANDOM.nextInt((Integer.MAX_VALUE - 1) >> INT_MASK_WIDTH);
> 592:         long res = addShiftConvMaskCheckIndex2(i, j, (Integer.max(i, j) << INT_MASK_WIDTH) + 1);

I wonder if it would make sense to keep the old tests, and just add new ones with random constants. That way if we mess up with the new changes, we would at least still have the old ones catching us.

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

PR Review: https://git.openjdk.org/jdk/pull/22856#pullrequestreview-2561547089
PR Review Comment: https://git.openjdk.org/jdk/pull/22856#discussion_r1921925693
PR Review Comment: https://git.openjdk.org/jdk/pull/22856#discussion_r1921920670