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

[Christian Hagedorn]

Thank you Tobias for your review!

Best regards,
Christian

On 19.02.20 11:22, Tobias Hartmann wrote:
> 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