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

[Christian Hagedorn]

Hi Vladimir

Thank you for your review!

On 20.02.20 03:04, Vladimir Kozlov wrote:
> May be add -XX:+IgnoreUnrecognizedVMOptions flag to test command because 
> LoopMaxUnroll is C2 flag.
> Or use @requires vm.compiler2.enabled

Fixed in place.

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

Yes, I tested tier1-4 and as part of trying to figure out if the code 
path is dead on line 2289, as mentioned in Tobias' review, up to tier7 
(was never reached):

2288       if (!independent(current, ld)) {
2289         return first_mem; // A later store depends on this load, 
pick memory state of first load
2290       }

Best regards,
Christian

> 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 
>>