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

[Vladimir Kozlov]

On 8/27/20 12:53 AM, Christian Hagedorn wrote:
> 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.

Good.

> 
> 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?

I agree with that.

Thanks,
Vladimir

> 
> Best regards,
> Christian
> 
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8252372