[15] RFR(S): 8238438: SuperWord::co_locate_pack picks memory state of first instead of last load

[Vladimir Kozlov]

May be add -XX:+IgnoreUnrecognizedVMOptions flag to test command because LoopMaxUnroll is C2 flag.
Or use @requires vm.compiler2.enabled

I assume you tested this fix with JDK-8233032 test case to make sure that bug did not return.

Thanks,
Vladimir

On 2/17/20 8:32 AM, Christian Hagedorn wrote:
> Hi
> 
> Please review the following patch:
> https://bugs.openjdk.java.net/browse/JDK-8238438
> http://cr.openjdk.java.net/~chagedorn/8238438/webrev.00/
> 
> When processing a load pack in SuperWord::co_locate_pack(), we pick by default the memory state of 
> the last load. But if we find a store that is dependent on an earlier load in the pack, then we need 
> to pick the memory state of the first load.
> 
> The current code, however, checks for each load 'l' if it has a dependency on an earlier store 
> instead of checking if a later store is dependent on 'l' (regression since [1]). At [2], we start at 
> the memory state of the current load and walk the memory graph to the memory state of the first load 
> while checking dependency constraints. This wrongly checks for store->load instead of load->store 
> dependencies. For example, in the test case [3], the load pack consists of LoadI 656, 652,... with 
> its memory states 657 StoreI (first_mem) and 653 StoreI, respectively. When processing the second 
> load in the pack, 652 LoadI, we go up the memory graph to first_mem (657 StoreI). The dependence 
> graph now tells us that 652 LoadI is dependent on 657 StoreI. From that we make the wrong conclusion 
> that we need to pick the memory state of the first load which results in a wrong execution for that 
> test case.
> 
> The fix now first finds the last memory state. Afterwards, it walks the memory graph from the last 
> memory state to the memory state of each load in the pack (but not beyond) while checking for any 
> load->store dependency constraint (as done before [1]).
> 
> Thank you!
> 
> Best regards,
> Christian
> 
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8233032, 
> https://hg.openjdk.java.net/jdk/jdk/rev/3b693618d084
> [2] http://hg.openjdk.java.net/jdk/jdk/file/27e87c000b16/src/hotspot/share/opto/superword.cpp#l2306
> [3] Part of the IR of the test case: 
> https://bugs.openjdk.java.net/secure/attachment/86850/load_pack_memory.png