RFR(S): 8229483: Sinking load out of loop may trigger: assert(found_sfpt) failed: no node in loop that's not input to safepoint

[Nils Eliasson]

On 2019-08-28 16:32, Roland Westrelin wrote:
> Thanks for reviewing this, Tobias.
>
>>> http://cr.openjdk.java.net/~roland/8229483/webrev.00/
>> I've noticed that field2 in the test is unused.
> Good catch. I'll remove it.
>
>>> The field load in the test case has an early control right below:
>>>
>>> barrier = 1;
>>>
>>> (it's a volatile store)
>>>
>>> and a late control that's conservatively set to be right above:
>>>
>>> array[0] = j;
>>>
>>> by anti dependence analysis.
>>>
>>> The store array[0] is sunk out of the inner loop in the outer strip
>>> mined loop right above the strip mined loop's safepoint. The load
>>> initially scheduled in the outer loop is a candidate for being cloned
>>> and sunk out of loop at the load usages. Because of the anti-dependence
>>> with the array[0], it is sunk into the outer strip mined loop eventhough
>>> it is not referenced by the safepoint. That breaks loop strip mining
>>> verification because the expectation is that any data node in the outer
>>> strip mined loop is there because it's referenced by the safepoint.
>> I don't see a field load in the loop, so which load are you referring to?
> 58         return field + res + field * 2;
>
> the load of static field "field".
>
>>> The fix simply recognizes this special case when the load is being sunk
>>> out of loop and makes sure it is not moved in the outer strip mined
>>> loop.
>> Wouldn't it be better to relax the verification code if possible? If we ever fix dependency analysis
>> to be less restrictive (see JDK-8229449), we should move the load out of the loop, right?
> Dependency analysis is so conservative right now that fixing it seems a
> long way from happening. So it could possibly be improved but I don't
> see it being "fixed" for good.

It sure is a hard problem to get right. I hope I can revisit it at some 
time.

I like the strict verification of strip mined loops, it sure helps with 
tracking down bugs.

Thanks for fixing!

Reviewed.

// Nils


>
> I'm not sure relaxing the verification code is better. Loop strip mining
> works under the assumption that some simple invariants remain
> true. That's what the verification code is here to check. Relaxing the
> verification code and thus the invariants would make reasoning about
> loop strip mining harder and doesn't seem like the right way to fix
> this. The invariant in this case is that nothing is in the outer loop
> other that the control flow for the outer loop, the safepoint and data
> nodes that are referred to from the safepoint.
>
>>> Unrelated to this fix, I wonder if the code at:
>>>
>>> http://hg.openjdk.java.net/jdk/jdk/file/cb836bd08d58/src/hotspot/share/opto/loopopts.cpp#l1346
>>>
>>> really does what the comment says it does for loads (and really does
>>> anything useful actually).
>> I'm not really familiar with that code but maybe file a RFE to investigate this later.
> I'm hoping Vladimir will comment. If not, I'll file a RFE as you
> suggest.
>
> Roland.