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

I had another go at this. I don't think we should somehow change the IGVN worklist to force the new phi nodes to be processed immediately. We could be adding new nodes during their transformations which could change our modifications again. I had another idea instead: Eagerly replacing the `this` phi node with the new `MergeMem` node. We would replace it anyways as a next step in IGVN when returning from `Phi::Ideal()`. This cuts the `this` phi entirely off from the graph and does not interfere in the dead loop checks during the transformations of the new phis:
https://github.com/openjdk/jdk/blob/3c0faa73522bd004b66cb9e477f43e15a29842e6/src/hotspot/share/opto/cfgnode.cpp#L2314

I think that fix is only needed during IGVN (not at parsing) when control and data can be dying.

However, I have not seen that we do something similar at any other place. It appears to be correct to me and testing looks good but I'm still wondering if this is really feasible to do?

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

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