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

[Daniel Lundén]

On Tue, 31 Oct 2023 10:33:03 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Daniel Lundén has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix push_and to step through all ConvI2L children
>
> 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?

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

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