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

[Christian Hagedorn]

As discussed with Vladimir K. and Tobias, I added an assert(false) in 
the code path that picks the first memory state together with some node 
dumping:
http://cr.openjdk.java.net/~chagedorn/8238438/webrev.01/

I suggest to file a follow-up RFE to either investigate further and/or 
to clean this code up later, for example in JDK 16, if we have not seen 
this assert being hit. If it's hit at some point and it turns out to be 
valid to use the first memory state, we can still remove the assert 
again and leave the code about selecting the first memory state.

Best regards,
Christian

On 20.02.20 09:26, Christian Hagedorn wrote:
> 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 
>>>