RFR(XS): 8245714: "Bad graph detected in build_loop_late" when loads are pinned on loop limit check uncommon branch

[Vladimir Kozlov]

Looks good to me too.

I would suggest to use explicit check if you want to test when C2 is enabled:

@requires vm.compiler2.enabled

Thanks,
Vladimir

On 6/2/20 3:40 AM, Tobias Hartmann wrote:
> Hi Roland,
> 
> Looks good to me. I'll run some testing and report back.
> 
> Maybe add a @requires vm.flavor == "server" to the test because the ArrayCopyLoadStoreMaxElem flag
> is only available with C2 (no new webrev required).
> 
> Best regards,
> Tobias
> 
> On 29.05.20 09:30, Roland Westrelin wrote:
>>
>>> https://bugs.openjdk.java.net/browse/JDK-8245714
>>> http://cr.openjdk.java.net/~roland/8245714/webrev.00/
>>>
>>> This triggers when data nodes are pinned on the uncommon trap path of a
>>> predicate. When a new predicate is added, a region is created to merge
>>> the paths comming from the place holder and the new predicate. Data
>>> nodes pinned on the uncommon path for the place holder are then updated
>>> to be pinned on the new region. That logic updates the control edge but
>>> not the control that loop opts keep track of. This causes a crash with
>>> the test case of the webrev where the predicate is a loop limit check.
>>
>> That fix is incomplete. If the Load that's pinned on the uncommon trap
>> path is a LoadN then there's a DecodeN between the uncommon trap and the
>> Load. The control of the DecodeN also needs to be updated. Here is an
>> updated fix:
>>
>> http://cr.openjdk.java.net/~roland/8245714/webrev.01/
>>
>> This one uses lazy_replace. I'm concerned that other nodes (maybe an
>> AddP) would be assigned the projection as control and need to be
>> moved. With lazy_replace, all nodes are guaranteed to be properly
>> updated.
>>
>> Roland.
>>