RFR: 8320909: C2 compilation fails with "Missed optimization opportunity in PhaseIterGVN"

[Emanuel Peter]

On Tue, 22 Apr 2025 08:06:48 GMT, Marc Chevalier <mchevalier at openjdk.org> wrote:

> The JBS issues has 3 reproducers. Two of them don't reproduce anymore. Let's enumerate:
> 
> - Test, TestSimple:
>   Disappeared with:
>   [JDK-8319372: C2 compilation fails with "Bad immediate dominator info"](https://bugs.openjdk.org/browse/JDK-8319372) in #16844
>   which actually fixed it by removing the handling of the problematic pattern in `CastIINode::Value`.
>   Reverting this fix makes the issue reappear.
> - Reduced2: I fix here
> - Test3, Reduced3:
>   Disappeared with:
>   [JDK-8347459: C2: missing transformation for chain of shifts/multiplications by constants](https://bugs.openjdk.org/browse/JDK-8347459) in #23728
>   which just shadowed it. The bug is essentially the same as Reduced2 otherwise. I also fix these here (even with reverted JDK-8347459)
> 
> The issue comes from the fact that `And[IL]Node::Value` has a special handling when
> an operand is a left-shift: in the expression
> 
> lhs & (X << s)
> 
> if `lhs` fits in less than `s` bits, the result is sure to be 0. This special handling
> also tolerate a `ConvI2LNode` between the `AndLNode` and `LShiftINode`. In this case,
> updating the Shift node during IGVN won't enqueue directly the And node, but only the
> Conv node. If this conv node cannot be improved, the And node is not enqueued, and its
> type is not as good as it could be.
> 
> Such a case is illustrated on the following figures from Reduced2. Node `239 Phi` is a phi with a
> dead branch, so the node is about to be eleminated. On the second figure, we can see
> `152 LShiftI` taking its place. The node `243 ConvI2L` is enqueued, but no change happens
> during its idealization, so the node `244 AndL` is not enqueued, while it could receive an update.
> 
> 
> <img src="https://github.com/user-attachments/assets/a68b27d0-b3cc-4850-b20e-0cff17047ed8" width="400">
> <img src="https://github.com/user-attachments/assets/d98731e8-d85c-413d-a77e-059787b01884" width="400">
> 
> The fix is pretty direct: we recognize this pattern and we enqueue the And node during IGVN
> to make sure it has a chance to be refined.
> 
> The case of Reduced3 is mostly the same, with a twist: the special handling of And nodes
> also can see through casts. See the next figure with a `CastIINode` between the `AndINode` and
> the `LShiftINode`. The fix has to take that into account.
> 
> <img src="https://github.com/user-attachments/assets/ae3d5a01-a0d6-4346-944c-4ba46af64ee2" width="400">
> 
> 
> Overall, the situation can be of the form:
> 
> LShift -> Cast+ -> ConvI2L -> Cast+ -> And
> 
> This second case was shadowed by [JDK-8347459](https://bugs...

@marc-chevalier Thanks for looking into this! I think the solution looks good :)

Did you put in all 3 reproducers, even the ones that do not reproduce any more? It could be interesting if we ever backport, and maybe also more generally because it generates interesting patterns.

@marc-chevalier I also wonder if we could not have a more descriptive title?

BTW: **thank you** for the detailed PR description, very helpful 😊

test/hotspot/jtreg/compiler/c2/gvn/MissingOptWithShiftConvAnd.java line 40:

> 38:  *          -XX:-TieredCompilation -Xbatch
> 39:  *          -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=10
> 40:  *          MissingOptWithShiftConvAnd

It could be good to have a run without flags. Or at lest with fewer flags. Just so we can run it with other flag combinations. Imagine we run it with `-XX:VerifyIterativeGVN=11` from the outside, that would just get downgraded to `-XX:VerifyIterativeGVN=10` immediately here.

test/hotspot/jtreg/compiler/c2/gvn/MissingOptWithShiftConvCastAnd.java line 34:

> 32:  *          -XX:-TieredCompilation -Xbatch
> 33:  *          -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=10
> 34:  *          MissingOptWithShiftConvCastAnd

Same here :)

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24792#pullrequestreview-2783397725
PR Comment: https://git.openjdk.org/jdk/pull/24792#issuecomment-2820672882
PR Review Comment: https://git.openjdk.org/jdk/pull/24792#discussion_r2053680291
PR Review Comment: https://git.openjdk.org/jdk/pull/24792#discussion_r2053680921