RFR: 8369646: Detection of redundant conversion patterns in add_users_of_use_to_worklist is too restrictive [v4]

[Emanuel Peter]

On Mon, 27 Oct 2025 08:42:37 GMT, Benoît Maillard <bmaillard at openjdk.org> wrote:

>> 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:
> 
>   Add -XX:+StressIGVN to run without fixed seed

Looks good to me :)

src/hotspot/share/opto/phaseX.cpp line 2568:

> 2566:   // ConvI2F->ConvF2I->ConvI2F
> 2567:   // Note: there may be other 3-nodes conversion chains that would require to be added here, but these
> 2568:   // are the only ones that are known to trigger missed optimizations otherwise

You may want to update the description, and give a bit of extra information. Because you are saying `n` does not have to be a conversion, but it may be that `n` is about to be replaced with a conversion, right?

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

Marked as reviewed by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/27900#pullrequestreview-3399343300
PR Review Comment: https://git.openjdk.org/jdk/pull/27900#discussion_r2477902652