RFR: 8275326: C2: assert(no_dead_loop) failed: dead loop detected [v2]

[Christian Hagedorn]

On Mon, 8 Nov 2021 09:18:14 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> In the test case, we apply the following optimization in `PhiNode::Ideal()` for the memory phi 989 that is on a dead path but still has both its inputs set to non-top nodes:
>> https://github.com/openjdk/jdk/blob/3c0faa73522bd004b66cb9e477f43e15a29842e6/src/hotspot/share/opto/cfgnode.cpp#L2269-L2270
>> ![Screenshot from 2021-11-05 11-57-49](https://user-images.githubusercontent.com/17833009/140502849-9f00fd62-9714-4f54-8f98-f22f74d11430.png)
>> 
>> In this process, we create `11853 Phi` for the new `11850 MergeMem` which is going to replace `989 Phi` (`this`). We then transform `11853 Phi` before returning:
>> https://github.com/openjdk/jdk/blob/3c0faa73522bd004b66cb9e477f43e15a29842e6/src/hotspot/share/opto/cfgnode.cpp#L2314
>> 
>> During `Ideal()` for `11853 Phi`, we transform `11769 MergeMem` into top (because the base memory is top) and use this as new input instead:
>> https://github.com/openjdk/jdk/blob/3c0faa73522bd004b66cb9e477f43e15a29842e6/src/hotspot/share/opto/cfgnode.cpp#L2230-L2240
>> 
>> But even if the `MergeMem` node would not be transformed into top, the slice itself could be top (L2237) and we would still replace the phi input with top. This replacement by top will fold the `11853 Phi` and we will build a cycle `11850 MergeMem` <-> `1064 StoreB` because `989 Phi` will be replaced by `11850 MergeMem`. This results in the assertion failure.
>> 
>> I tried some approaches by marking `11853 Phi` and/or `989 Phi` to specially treat them during the optimizations in `Ideal()` (e.g. skipping `989 Phi` during the dead loop detection etc.) or to improve the dead loop detection before applying the `MergeMem` optimization in `Ideal()`. But that seemed rather complicated/fragile.
>> 
>> I therefore propose to simply not transform the newly created phi nodes directly but wait instead for IGVN to revisit them again. This allows the `this` phi to be replaced with the new `MergeMem` node and the dead loop detection will work correctly when processing the new phis again later in IGVN.
>> 
>> I could only reproduce this bug with the replay file for the attached test case in the JBS issue. The test case itself did not trigger with repeated runs with `StressIGVN` + `RepeatCompilation`.
>> 
>> Thanks,
>> Christian
>
> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   handle GVN

Thanks Vladimir for reviewing it again. Unfortunately, I hit some repeatable performance regressions in some micro crypto benchmarks. I will have another look to find a better solution. It looks like it is a problem when the new phis are not directly transformed. I could imagine that other node transformations might interfere, possibly resulting in a different IR. Maybe another fix could be to somehow force the new phis to be transformed as immediate next nodes in IGVN right after the old phi node is completely processed and subsumed by the new `MergeMem` node.

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

PR: https://git.openjdk.java.net/jdk/pull/6276