RFR: 8369646: Detection of redundant conversion patterns in add_users_of_use_to_worklist is too restrictive [v3]
Benoît Maillard
bmaillard at openjdk.org
Mon Oct 20 16:19:37 UTC 2025
> This PR addresses a missed optimization opportunity in `PhaseIterGVN`. The missed optimization is the simplification of redundant conversion patterns of the shape `ConvX2Y->ConvY2X->ConvX2Y`.
>
> This optimization pattern is implemented as an ideal optimization on `ConvX2Y` nodes. Because it depends on the input of the input of the node in question, we need to have an appropriate notification mechanism in `PhaseIterGVN::add_users_of_use_to_worklist`. The notification for this pattern was added in [JDK-8359603](https://bugs.openjdk.org/browse/JDK-8359603).
>
> However, that fix was based on the wrong assumption that in `PhaseIterGVN::add_users_of_use_to_worklist`, argument `n` is already the optimized node. However in some cases this argument is actually the node that is about to get replaced.
>
> This happens for example in `PhaseIterGVN::transform_old`. If we find that node `k` returned by `Ideal` actually already exists by calling `hash_find_insert(k)`, we call `add_users_to_worklist(k)`.
> As we replace node `k` with `i`, and `i` as a different opcode than `k`, then we cannot use the opcode of `k` to detect the redundant conversion pattern.
>
> ```c++
> ...
> // Global Value Numbering
> i = hash_find_insert(k); // Check for pre-existing node
> if (i && (i != k)) {
> // Return the pre-existing node if it isn't dead
> NOT_PRODUCT(set_progress();)
> add_users_to_worklist(k);
> subsume_node(k, i); // Everybody using k now uses i
> return i;
> }
> ...
>
>
> The bug was quite intermittent and only showed up in some cases with `-XX:+StressIGVN`.
>
> ### Proposed Fix
>
> We make the detection of the pattern less specific by only looking at the opcode of the user of `n`, and not directly the opcode of `n`. This is consistent with the detection of other patterns in `PhaseIterGVN::add_users_of_use_to_worklist`.
>
> ### Testing
> - [x] [GitHub Actions](https://github.com/benoitmaillard/jdk/actions?query=branch%3AJDK-8369646)
> - [x] tier1-3, plus some internal testing
> - [x] Added a second run for the existing test with `-XX:+StressIGVN` and a fixed stress seed
>
> Thank you for reviewing!
Benoît Maillard has updated the pull request incrementally with one additional commit since the last revision:
Missing -XX:+UnlockDiagnosticVMOptions
-------------
Changes:
- all: https://git.openjdk.org/jdk/pull/27900/files
- new: https://git.openjdk.org/jdk/pull/27900/files/86ed4661..12706636
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=27900&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=27900&range=01-02
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/27900.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/27900/head:pull/27900
PR: https://git.openjdk.org/jdk/pull/27900
More information about the hotspot-compiler-dev
mailing list