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

[Roland Westrelin]

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.

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.