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

[Matthias Ernst]

On Mon, 20 Jan 2025 07:42:43 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> 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.
>
> 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?

Because of the details wrt ADD the body feels out of context here. I find it easier to read when it's close to  `AndIL_sum_and_mask` below.

> 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.

I'll let you decide - but I do find the combinatorial of {int, long, i2l} x {const, nonconst mask} x {left, right} x {+shift, +const} pretty unwieldy already, duplicating the tests once more would just be piling on.

I could move the randomized tests to a new file name, keep the old as is, and once you have sufficient confidence, you delete the old ones? I don't see they can be maintained in parallel for long.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22856#discussion_r1921955817
PR Review Comment: https://git.openjdk.org/jdk/pull/22856#discussion_r1921966523