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

[Christian Hagedorn]

Thank you Vladimir and Tobias for your reviews!

I filed [1] to clean up this code in JDK 16 - if the assertion code is 
not hit in the meantime.

Best regards,
Christian


[1] https://bugs.openjdk.java.net/browse/JDK-8240281

On 02.03.20 09:39, Tobias Hartmann wrote:
> +1
> 
> Best regards,
> Tobias
> 
> On 28.02.20 19:21, Vladimir Kozlov wrote:
>> Looks good.
>>
>> Thanks,
>> Vladimir
>>
>> On 2/28/20 1:21 AM, Christian Hagedorn wrote:
>>> 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