[10] RFR(S): 8189067: SuperWord optimization crashes with "assert(out == prev || prev == __null) failed: no branches off of store slice"

[Tobias Hartmann]

Hi Vladimir,

thanks for the review!

On 12.10.2017 19:33, Vladimir Kozlov wrote:
> Good. I think we should leave this conservative fix without optimizing it. We should not spend a lot of time optimizing 
> C2 now.

Okay, that's fine with me. I anyone wants to optimize that later, he/she can file an RFE.

Best regards,
Tobias
> On 10/12/17 3:04 AM, Tobias Hartmann wrote:
>> Hi,
>>
>> please review the following patch:
>> https://bugs.openjdk.java.net/browse/JDK-8189067
>> http://cr.openjdk.java.net/~thartmann/8189067/webrev.01/
>>
>> The problem is in the C2 optimization that moves stores out of a loop [1].
>>
>> We move stores out of a loop by creating clones at all loop exit paths that observe the stored value. When walking up 
>> the dominator chain from observers of a store and placing clones of the store at the top (right after the loop), we 
>> may end up placing multiple cloned stores at the same location. This confuses/crashes the SuperWord optimization and 
>> also affects performance of the generated code due to double execution of the same store (see details in the bug 
>> comments).
>>
>> My initial prototype fix (webrev.00) just checked if there already is a cloned store with the same control input and 
>> reused it instead of creating another one. I've discussed this with Roland in detail and we came to the conclusion 
>> that this is not sufficient. We may still end up with a broken memory graph: If one use of the store is a load, we 
>> create a clone of the store and connect it to the load but this store is not connected to the rest of the memory 
>> graph, i.e. the memory effect of the store is not propagated. Although this may not cause incorrect execution (at 
>> least we were not able to trigger that), it may cause problems if other optimizations kick in and in some cases we 
>> still end up with the same store being executed twice.
>>
>> We agreed to go with the safer version of computing the LCA and move the store there (only the initial store without 
>> creating clones) if it's outside of the loop. This is a bit too strict in cases where there's an uncommon trap in the 
>> loop (see 'TestMoveStoresOutOfLoops::test_after_6') but it works fine in the other cases exercised by the test. If 
>> people agree, I'll file a follow up RFE to improve the optimization but would like to go with the safe fix for now 
>> (this also affects JDK 9).
>>
>> Thanks,
>> Tobias
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8080289