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

[Christian Hagedorn]

Hi Roland

On 21.08.20 17:22, Roland Westrelin wrote:
> 
> Hi Christian,
> 
>> We have two options to fix this. We could either update the wrong
>> control inputs from 876 IfFalse during the creation/merging of
>> pre/main/post loops or directly fix it inside
>> split_if_with_blocks_post(). I think it is makes more sense and is also
>> easier to directly fix it in split_if_with_blocks_post() where we could
>> be less pessimistic when pinning loads.
>>
>> The fix now checks if late_load_ctrl is a loop exit of a loop that has
>> an outer strip mined loop and if it dominates x_ctrl. If that is the
>> case, we use the outer loop exit control instead. This also means that
>> the loads can completely float out of the outer strip mined loop.
>> Applying that to the testcase, we get [3] instead of [2]. LoadS 901 and
>> 902 are both at the outer strip mined loop exit while 903 LoadS is still
>> at the inner loop due to 575 StoreI (x_ctrl is 876 IfFalse and dominates
>> the outer strip mined loop exit). The process of creating pre/main/post
>> loops will then take care of these control inputs of the LoadSNodes and
>> rewires them to the newly created RegionNode such that the dominator
>> information is correct again.
> 
> I agree that fixing it in split_if_with_blocks_post() is the right thing
> to do.
> 
> The load has no edges to the safepoint in the outer strip mined loop so
> why is it in the loop in the first place then? If java code has a load
> in a loop that's live outside the loop then it should be live at the
> safepoint on loop exit. Is anti dependence analysis too conservative?

I maybe should have shared another image of the graph before the LoadS 
clones 901-903 are created. The original 572 LoadS (see [1]) is an input 
into 575 StoreI which is an input of 578 MergeMem which goes into the 
881 SafePoint in the outer strip mined loop. The other two uses (897 Phi 
and 893 Phi) are uses outside of the outer strip mined loop.

> Also why does get_late_ctrl(n, n_ctrl) return a control inside the outer
> strip mined loop? And why is it safe to bypass that result?

Due to 575 StoreI being needed inside the outer strip mined loop, 
get_late_ctrl() of 572 LoadS also returns the inner loop exit 876 
IfFalse. My thinking was that since we now clone 572 LoadS and create a 
new LoadS for each use, then we don't need to pin the LoadS going into 
Phi 893 and 897 to 876 IfFalse, too, if x_ctrl is outside the outer 
strip mined loop but to the outer strip mined loop exit.

But now thinking about it, do we need another get_late_ctrl(x, 
late_load_ctrl) for each clone and check if they can really be put 
outside of the strip mined loop instead of just checking dominance with 
x_ctrl (which is based on get_ctrl(u) of a use of the load)? In 
get_late_ctrl() we do consider anti dependencies. Maybe something like 
this (change on L1473):

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

Best regards,
Christian


[1] 
https://bugs.openjdk.java.net/secure/attachment/89947/before_cloning_LoadS.png