RFR: 8316594: C2 SuperWord: wrong result with hand unrolled loops

[Tobias Hartmann]

On Thu, 21 Sep 2023 15:26:25 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> Please read description in https://github.com/openjdk/jdk/pull/15864.
> 
> Currently, we only compare the first element of the load-pack with the StoreVector. This is not correct, we need to compare all elements of the load-pack with the StoreVector. Otherwise it may be that the first element lays below/before the memory region of the StoreVector (Less), but the last element of the load-pack for example could have overlap with the StoreVector (Equal).
> 
> Tier1-6 + stress-testing (Running)
> Performance testing (Running)

Looks good to me. To avoid merge conflicts when pushing https://github.com/openjdk/jdk/pull/15864, you could fix both issues in the same PR since they are in the same code.

test/hotspot/jtreg/compiler/loopopts/superword/TestMovingLoadBeforeStore.java line 29:

> 27:  * @requires vm.compiler2.enabled
> 28:  * @bug 8316679
> 29:  * @summary In SuperWord::output, LoadVector can be moved before StoreVector, but only if it is proven to be safe.

The test should have `@key randomness`

test/hotspot/jtreg/compiler/loopopts/superword/TestMovingLoadBeforeStore.java line 96:

> 94:             b[inv + i +  1]++;
> 95:             b[inv + i +  2]++;
> 96:             b[inv + i +  3]++;

You might want to remove the extra whitespaces in the index expressions.

test/hotspot/jtreg/compiler/loopopts/superword/TestMovingLoadBeforeStore2.java line 41:

> 39: import jdk.test.lib.Utils;
> 40: 
> 41: public class TestMovingLoadBeforeStore2 {

I think the two tests should be merged or is there a reason to have them separate? If you want different command line arguments, you can add two `@run`. Even if they are not needed for both, I think it doesn't hurt.

-------------

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15866#pullrequestreview-1654420543
PR Review Comment: https://git.openjdk.org/jdk/pull/15866#discussion_r1343551771
PR Review Comment: https://git.openjdk.org/jdk/pull/15866#discussion_r1343552154
PR Review Comment: https://git.openjdk.org/jdk/pull/15866#discussion_r1343553171