RFR: 8313672: C2: PhaseCCP does not correctly track analysis dependencies involving shift, convert, and mask [v3]

[Emanuel Peter]

On Thu, 2 Nov 2023 09:51:55 GMT, Daniel Lundén <duke at openjdk.org> wrote:

>> src/hotspot/share/opto/phaseX.cpp line 1998:
>> 
>>> 1996:       }
>>> 1997:     };
>>> 1998:     ConstraintCastNode::visit_uncasted_uses(use, push_and_uses_to_worklist_skip_conv);
>> 
>> Maybe it would be nicer to generalize `visit_uncasted_uses`, and allow it to also peek through `ConvI2L` (optionally enabled with flag, false by default). Something like this:
>> 
>> 
>>         if (internal_use->is_ConstraintCast()) {
>>           internals.push(internal_use); // traverse this cast also
>>         } else if (peek_thru_convi2l && internal_use->Opcode() == Op_ConvI2L) {
>>           internals.push(internal_use); // traverse this cast also
>>         } else {
>>           callback(internal_use);
>>         }
>> 
>> 
>> Maybe you'd have to rename `visit_uncasted_uses` a bit. Or just create a common utility method and then call that one from `visit_uncasted_uses` with `peek_thru_convi2l = false`.
>> 
>> Maybe you have a better idea. But I have a sense that this may have to be done again in the future.
>
> I have now pushed a new version along what you suggest. A technicality is that the new version may also step over _more_ than one `ConvI2L`, which doesn't match the logic in `MulNode::AndIL_shift_and_mask_is_always_zero` that only steps over a single `ConvI2L`. However, chaining `ConvI2L` in this way does not make sense type-wise and should be caught by a type checker in some earlier stage?

Yes, this should not be possible to go through multiple `ConvI2L`, without any `ConvL2I`. I think there would be some issues / type mismatch somewhere.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16429#discussion_r1383568617