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

[Tobias Hartmann]

Hi Christian,

your fix looks good to me but as we've discussed in-depth off-thread, I'm wondering as well if we
actually ever need to use the first memory state or couldn't always go with the last one.

You've explained that you were not able to trigger this code path and I also have a hard time
thinking of a case. Maybe superword was changed since this code was introduced long ago and we now
bail out earlier. I'm hoping that someone remembers in which cases this code was actually required.

Best regards,
Tobias

On 17.02.20 17:32, 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