[16] RFR(M): 8249607: C2: assert(!had_error) failed: bad dominance

[Christian Hagedorn]

On 25.08.20 19:42, Christian Hagedorn wrote:
> On 25.08.20 16:13, Roland Westrelin wrote:
>>
>>> In the testcase, a LoadSNode is cloned in
>>> PhaseIdealLoop::split_if_with_blocks_post() for each use such that they
>>> can float out of a loop. To ensure that these loads cannot float back
>>> into the loop, we pin them by setting their control input [1]. In the
>>> testcase, all 3 new clones are pinned to a loop exit node that is part
>>> of an outer strip mined loop (see [2]).
>>
>> Do I understand this right, that all 3 clones are pinned with the same
>> control? So they common and only of them is kept?
> 
> Yes, exactly. All are pinned to the inner loop exit node. But at the 
> time we hit the assertion failure, we still got one cloned load (903 
> LoadS) that is an input to the store (575 StoreI) that's going into the 
> outer strip mined loop safepoint, and one load (901 LoadS) that is 
> triggering the dominance failure. LoadS 902 was removed at some point in 
> between due to other optimizations.

As Roland and I have discussed offline, it seems to be better and safer 
to do a simpler fix that does not change the original behavior of the 
optimization. The new fix suggests not yank AddP nodes (which are inputs 
to the cloned LoadSNodes in the testcase) and also to not yank gc 
barriers. In the testcase, the cloned LoadSNodes are still pinned at the 
loop exit but now they can be optimized and common up to one node during 
igvn that only belongs to the safepoint in the outer strip mined loop 
(i.e. no load after the loop anymore). The load is still successfully 
removed from the inner loop:

http://cr.openjdk.java.net/~chagedorn/8249607/webrev.02/

I left the improved dominance failure dumping as it is.

We think that it would be a good idea to revisit this cloning 
optimization in an RFE and also consider webrev.01 there as it seems to 
be more like an enhancement for loop strip mining rather than a bug fix. 
I filed [1] which summarizes some thoughts about it.

What do others think about that?

Best regards,
Christian


[1] https://bugs.openjdk.java.net/browse/JDK-8252372