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

[Christian Hagedorn]

Hi Vladimir

On 27.08.20 19:23, Vladimir Kozlov wrote:
> 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.

Thank you for your review!

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

Great!

Best regards,
Christian

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