RFR: 8291466: C2: assert(false) failed: infinite loop in PhaseIterGVN::transform_old with -XX:+StressIGVN [v4]

[Christian Hagedorn]

On Mon, 29 Aug 2022 11:34:02 GMT, Pengfei Li <pli at openjdk.org> wrote:

>> Recently, an igvn infinite loop issue was reported. Git bisection shows
>> it appears after our JDK-8289996 patch. But after that patch is backed
>> out, we find similar test still fails. We have attached a jtreg case to
>> reproduce this issue.
>> 
>> The ideal graph of the problematic method has a `MulINode` multiplying a
>> `PhiNode` by a `ConINode`. For better optimizations, `MulINode::Ideal()`
>> moves the constant input to the right hand side. And `Ideal()` function
>> in its parent class `MulNode` has the similar logic. In some code paths,
>> `MulINode::Ideal()` calls `MulNode::Ideal()`. The problem here is that,
>> `MulINode` and `MulNode` use different ways to check constant. One calls
>> `type->singleton()` and the other calls `node->find_int_con(val)` which
>> accepts constant in `PhiNode`. So in some corner cases where a `PhiNode`
>> can be evaluated to a constant, the two inputs of the `MulNode` will be
>> swapped back and forth in `Ideal()` calls. It eventually causes the igvn
>> infinite loop issue.
>> 
>> This patch removes the `swap_edges()` logic in `MulINode` and `MulLNode`
>> because it's enough to do this by calling `MulNode::Ideal()`. We also do
>> some code cleanup in this patch as we have done in JDK-8289996.
>> 
>> Tested hotspot::hotspot_all_no_apps, jdk::tier1~3 and langtools::tier1.
>
> Pengfei Li has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits:
> 
>  - Add some minor fixes
>  - Merge branch 'master' into igvn
>  - Make the code more readable
>  - Reword some comments
>  - 8291466: C2: assert(false) failed: infinite loop in PhaseIterGVN::transform_old with -XX:+StressIGVN
>    
>    Recently, an igvn infinite loop issue was reported. Git bisection shows
>    it appears after our JDK-8289996 patch. But after that patch is backed
>    out, we find similar test still fails. We have attached a jtreg case to
>    reproduce this issue.
>    
>    The ideal graph of the problematic method has a `MulINode` multiplying a
>    `PhiNode` by a `ConINode`. For better optimizations, `MulINode::Ideal()`
>    moves the constant input to the right hand side. And `Ideal()` function
>    in its parent class `MulNode` has the similar logic. In some code paths,
>    `MulINode::Ideal()` calls `MulNode::Ideal()`. The problem here is that,
>    `MulINode` and `MulNode` use different ways to check constant. One calls
>    `type->singleton()` and the other calls `node->find_int_con(val)` which
>    accepts constant in `PhiNode`. So in some corner cases where a `PhiNode`
>    can be evaluated to a constant, the two inputs of the `MulNode` will be
>    swapped back and forth in `Ideal()` calls. It eventually causes the igvn
>    infinite loop issue.
>    
>    This patch removes the `swap_edges()` logic in `MulINode` and `MulLNode`
>    because it's enough to do this by calling `MulNode::Ideal()`. We also do
>    some code cleanup in this patch as we have done in JDK-8289996.
>    
>    Tested hotspot::hotspot_all_no_apps, jdk::tier1~3 and langtools::tier1.

That looks good, thanks for adding the suggestions!

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

Marked as reviewed by chagedorn (Reviewer).

PR: https://git.openjdk.org/jdk/pull/9820