RFR: 8371065: C2 SuperWord: VTransformLoopPhiNode::apply set wrong type, led to wrong constant folding of phi [v2]

[Quan Anh Mai]

On Fri, 7 Nov 2025 06:07:22 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> In `VTransformLoopPhiNode::apply`, we may have to modify the type of the phi node, because it may have been turned from a scalar phi to a vector phi by `VTransformReductionVectorNode::optimize_move_non_strict_order_reductions_out_of_loop`. This logic was refactored in https://github.com/openjdk/jdk/pull/27704, and I missed an edges case.
>> 
>> The issue is when we also (uslessly) set the type of phis that stay scalar: the `in1` type can be a constant, and then we set the `phi` type to be constant. And then the phi wrongly constant folds.
>> 
>> I now limit the modification to cases where the `phi` used to be for scalars, but now is for vectors. In those cases we should not have a constant. For good measure, I also added a corresponding assert.
>> 
>> ---------
>> 
>> Thanks @rwestrel for filing this issue and spending a lot of time reproducing it without his changes.
>> I tried to find a simpler reproducer, but it was difficult: We need a constant on the lhs of the phi in the main-loop. But this requires us to constant-fold the pre-loop phi, and somehow magically not constant fold the phi of the main-loop. That is quite tricky, and I gave up.
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
> 
>   add diagnostic flag for product build

That may be more preferable. Or we can track the type in `VTransformLoopPhiNode` and change it when we decide to do the transformation, at the same time as other nodes in the loop? I see that `VTransformLoopPhiNode::apply` returns a `make_scalar`, which seems confusing if it can be a vector, too. Or we can have `VTransformScalarLoopPhi` and `VTransformVectorLoopPhi` as separate classes, but it seems like it will result in some unnecessary duplication.

These are just suggestions, and my expertise in the superword vectorizer is definitely lacking, please make the decision that you think is best.

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

PR Comment: https://git.openjdk.org/jdk/pull/28113#issuecomment-3506668036