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

[Matthias Ernst]

On Wed, 5 Feb 2025 07:15:56 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Matthias Ernst has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   jlong, not long
>
> src/hotspot/share/opto/mulnode.cpp line 2059:
> 
>> 2057: 
>> 2058: // Returns a lower bound on the number of trailing zeros in expr, or -1 if the number
>> 2059: // cannot be determined.
> 
> Why not just return `0` if we cannot determine it? That would still be a correct lower bound, right?

Can run into problematic corner case depending on order of checks, see other comment.

> src/hotspot/share/opto/mulnode.cpp line 2104:
> 
>> 2102: static bool AndIL_is_zero_element_under_mask(const PhaseGVN* phase, const Node* expr, const Node* mask, BasicType bt) {
>> 2103:   jint expr_trailing_zeros = AndIL_min_trailing_zeros(phase, expr, bt);
>> 2104:   if (expr_trailing_zeros < 0) {
> 
> It feels a little strange that the number of trailing zeros could be negative...
> That's why I would return 0 if we can prove nothing. It is still clear that we can do nothing here if it is zero, so we can just compare `<= 0`.
> 
> Or what was the reason for returning `-1`?

Two motivations here, that were too implicit, both hinging on the "0 >= 0" case:
* if we just start with "if (mask==0) return true", then we can crash later with a `not monotonic` error in case `expr` is in some form of non-well-formed state (e.g. when shift_t == nullptr). That's what I was trying to distinguish. Not very obvious.
* if we return false on trailing_zeros == 0, it appears like we don't handle the (edge case) "(x + 3) & 0". That irks a reader, but I do agree we should punt this case because that will be handled downstream anyway (the whole AndNode goes away).

I think it's best to reorder as follows:


// we're only trying to cover actual shifts, not << 0
if (mask_lo < 0 || mask_hi == 0) return false     // mask == 0 handled in MulNode::Ideal
mask_width = 64 - count_leading_zeros(mask_hi)
return trailing_zeros(expr) >= mask_width   // don't need to worry about the 0>=0 case here.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22856#discussion_r1943398162
PR Review Comment: https://git.openjdk.org/jdk/pull/22856#discussion_r1943322101