RFR: 8359603: Missed optimization in PhaseIterGVN for redundant ConvX2Y->ConvY2X->ConvX2Y sequences due to missing notification in PhaseIterGVN::add_users_of_use_to_worklist

[Christian Hagedorn]

On Thu, 17 Jul 2025 12:25:33 GMT, Benoît Maillard <bmaillard at openjdk.org> wrote:

> This PR addresses a missed optimization in `PhaseIterGVN` due to the lack of change notification to indirect users within `PhaseIterGVN::add_users_of_use_to_worklist` (again). This is similar to [JDK-8361700](https://bugs.openjdk.org/browse/JDK-8361700?filter=-1).
> 
> The optimization in question is the removal of redundant `ConvX2Y->ConvY2X->ConvX2Y` sequences (where `X` and `Y` are primitive number types), which get replaced by a single `ConvX2Y` as an identity optimization. This missing optimization was originally reported only for `ConvD2LNode`, but it turns out that other conversion nodes have analog optimization patterns. After manual inspection of identity optimizations in `convertnode.cpp`, I was able to reproduce missing optimizations for the following conversion sequences:
> - `ConvD2L->ConvL2D->ConvD2L`
> - `ConvF2I->ConvI2F->ConvF2I`
> - `ConvF2L->ConvL2F->ConvF2L`
> - `ConvI2F->ConvF2I->ConvI2F`
> 
> Similar optimization patterns exist for additional conversion nodes. However, it is not clear if these nodes are subject to the same missed optimization issue. Further investigation may be needed, as I was unable to reproduce such cases with simple tests.
> 
> This is again a case where an optimization depends on the input of its input. Currently, `PhaseIterGVN::add_users_of_use_to_worklist` contains specific logic to handle similar dependencies for other cases, but this specific scenario is not addressed. The proposed fix adds the necessary logic in `add_users_of_use_to_worklist` to ensure proper notification for this optimization pattern. 
> 
> ### Testing
> - [x] [GitHub Actions](https://github.com/benoitmaillard/jdk/actions?query=branch%3AJDK-8359603)
> - [x] tier1-3, plus some internal testing
> - [x] Added test from the fuzzer, and tests for other sequences (manually derived from the original one)
> 
> Thank you for reviewing!

Two minor comments, otherwise, it looks good to me!

test/hotspot/jtreg/compiler/c2/TestEliminateRedundantConversionSequences.java line 30:

> 28:  *          simplified to a single ConvX2Y operation when applicable
> 29:  *          VerifyIterativeGVN checks that this optimization was applied
> 30:  * @run main/othervm -XX:+IgnoreUnrecognizedVMOptions -XX:CompileCommand=quiet

I think you can remove `quiet`:
Suggestion:

 * @run main/othervm -XX:+IgnoreUnrecognizedVMOptions

test/hotspot/jtreg/compiler/c2/TestEliminateRedundantConversionSequences.java line 94:

> 92: 
> 93:     public static void main(String[] strArr) {
> 94:         for (int i = 0; i < 50_000; ++i) {

Do you really need 50000 iterations each? Would less also trigger the bug?

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/26368#pullrequestreview-3046338959
PR Review Comment: https://git.openjdk.org/jdk/pull/26368#discussion_r2224845814
PR Review Comment: https://git.openjdk.org/jdk/pull/26368#discussion_r2224849384