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

[Vladimir Kozlov]

Good. I think we should leave this conservative fix without optimizing it. We should not spend a lot of time optimizing 
C2 now.

Thanks,
Vladimir

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