8357530: C2 SuperWord: Diagnostic flag AutoVectorizationOverrideProfitability

[eme64]

I'm adding a diagnostic flag AutoVectorizationOverrideProfitability. The goal is that with it, we can systematically benchmark our Auto Vectorization profitability heuristics. In all cases, we run Auto Vectorization, including packing.

• 0: abort vectorization, as if it was not profitable.
• 1: default, use profitability heuristics to determine if we should vectorize.
• 2: always vectorize when possible, even if profitability heuristic would say that it is not profitable.

In the future, we may change our heuristics. We may for example introduce a cost model JDK-8340093. But at any rate, we need this flag, so that we can override these profitability heuristics, even if just for benchmarking.

I did not yet go through all of SuperWord to check if there may be other decisions that could go under this flag. If we find any later, we can still add them.

Below, I'm showing how it helps to benchmark the some reduction cases we have been working on.

And if you want a small test to experiement with, I have one at the end for you.

Note to reviewer: This patch should not make any behavioral difference, i.e. with the default AutoVectorizationOverrideProfitability=1 the behavior should be as before this patch.

---

Use-Case: investigate Reduction Heuristics

A while back, I have written a comprehensive benchmark for Reductions #21032. I saw that some cases might possibly be profitable, but we have disabled vectorization because of a heuristic.

This heuristic was added a long time ago. The observation at the time was that simple add and mul reductions were not profitable.

• https://bugs.openjdk.org/browse/JDK-8078563
• https://mail.openjdk.org/pipermail/hotspot-compiler-dev/2015-April/017740.html
  From the comments, it becomes clear that "simple reductions" are not profitable, that's why we check if there are more work vectors than reduction vectors. But I'm not sure why 2-element reductions are deemed always not profitable. Maybe it fit the benchmarks at the time, but now with moving reductions out of the loop, this probably does not make sense any more, at least for int/long.

But in the meantime, I have added an improvement, where we move int/long reductions out of the loop. We can do that because int/long reductions can be reordered. See #13056 . We cannot do that with float/double reductions, because there we must keep the strict order of reductions. Otherwise we risk wrong rounding results.

Since then, we have had multiple reports that simple reductions are not vectorized, and I am working on it:
https://bugs.openjdk.org/browse/JDK-8307516

Running the reduction benchmarks from #21032 (please have a look at it now, the results below are only going to be more complicated!), like this:

make test TEST="micro:vm.compiler.VectorReduction2.WithSuperword" CONF=linux-x64 TEST_VM_OPTS="-XX:+UnlockDiagnosticVMOptions -XX:AutoVectorizationOverrideProfitability=2"

I ran the experiments on my x64 / AVX512 machine, and a aarch64 / neon machine.
For each I ran with SuperWord disabled (no), and with SuperWord and AutoVectorizationOverrideProfitability set to 1 (default), 0 (abort vectorization), and 2 (force vectorization).

The orange heuristic tags show where the heuristic makes a difference - in this case we prevent vectorization even though it is would be faster. This is evidence that we need to update the heuristic.

Interestingly, forcing vectorization in the strict cases did not lead to any performance drop.

It seems that forced vectorization is only problematic in one case: longMulSimple on aarch64. I need to investigate. Generally, we do vectorize (if forced - they are 2-element vectors after all) at least some of the long cases (hand checked longAddSimple), but it seems it is just not very fast, no idea why. The problematic longMulSimple does also vectorize (if forced only), but it is consistently slow. The confusing part: longMulDotProduct should be even slower. But a quick investigation showed that we actually do not vectorize it, the packing algorithm gets confused about which multiplications to pack. I suspect that generally 2-element multiplication reduction is very slow on neon / arch64. We will have to be careful about that when we change the heuristic. It is edge cases like these that make me nervous, and are the reason why I have not changed these heuristics sooner.

I would also have to investigate the impact on a few more platforms, especially on AVX and AVX2.

With x64 and byte/char/short, we never vectorize. Still, enabling SuperWord changes the level of unrolling, and it seems in some cases SuperWord enabled leads to over-unrolling, hence you see some slowdowns in some cases. We should investigate that as well.

For now it is clear: this flag would be helpful for improving performance heuristics.

---

Example for the Flag

I played around with an example like this:

java -XX:CompileCommand=compileonly,Test::test2 -XX:CompileCommand=TraceAutoVectorization,Test::test*,ALL -Xbatch -XX:AutoVectorizationOverrideProfitability=0 -XX:MaxVectorSize=64 Test.java

public class Test {
    public static int[] a = new int[10_000];

    public static void main(String[] args) {
        for (int i = 0; i < a.length; i++) {
            a[i] = (int)i;
        }

        for (int i = 0; i < 10_000; i++) {
            test1();
            test2(a, a);
	}
        System.out.println("sum: " + test1());
    }

    public static int test1() {
	int sum = 0;
        for (int i = 0; i < a.length; i++) {
            sum += a[i];
	}
        return sum;
    }

    public static void test2(int[] a, int[] b) {
        for (int i = 0; i < a.length; i++) {
            a[i] = b[i];
        }
    }
}

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8357530: C2 SuperWord: Diagnostic flag AutoVectorizationOverrideProfitability (Enhancement - P4)

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer) Review applies to 54c626e0
• Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25387/head:pull/25387
$ git checkout pull/25387

Update a local copy of the PR:
$ git checkout pull/25387
$ git pull https://git.openjdk.org/jdk.git pull/25387/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25387

View PR using the GUI difftool:
$ git pr show -t 25387

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25387.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back epeter! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@eme64 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8357530: C2 SuperWord: Diagnostic flag AutoVectorizationOverrideProfitability

Reviewed-by: thartmann, kvn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 48 new commits pushed to the master branch:

• ed4cd2a: 8356989: Unexpected null in C2 compiled code
• 9946c85: 8357621: G1: Clean up G1BiasedArray
• 5cdeef8: 8331859: [PPC64] Remove support for Power7 and older
• ... and 45 more: https://git.openjdk.org/jdk/compare/12efc1fe3322d16f0e703b334912241d80dbaba9...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@eme64 this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8357530-SuperWordOverrideProfitability
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[openjdk]

@eme64 The following label will be automatically applied to this pull request:

• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 02: Full - Incremental (8da34fc8)
• 01: Full - Incremental (3f2c2698)
• 00: Full (54c626e0)

[eme64]

@galderz You may be interested in these results ;)

[vnkozlov]

This looks fine. One suggestion I have for separate RFE is to use UL for such outputs.

[TobiHartmann]

Looks good to me otherwise.

[eme64]

This looks fine. One suggestion I have for separate RFE is to use UL for such outputs.

@vnkozlov Thanks for the approval! About your suggestion:
I suppose that means I would be refactoring all TraceSuperWord/TraceAutoVectorization to use UL. Is there now a good way to do the CompileCommand method-level filtering with UL? Because TraceAutoVectorization uses method-based filtering.

@TobiHartmann I applied all your suggestions :)

[eme64]

@TobiHartmann @vnkozlov Thanks for the reviews!

/integrate

[openjdk]

Going to push as commit e8eff4d.
Since your change was applied there have been 53 commits pushed to the master branch:

• a300c35: 8357105: C2: compilation fails with "assert(false) failed: empty program detected during loop optimization"
• bd09589: 8356632: Fix remaining {@link/@linkplain} tags with refer to private/protected types in java.base
• daa8eda: 8357056: RISC-V: Asm fixes - load/store width
• ... and 50 more: https://git.openjdk.org/jdk/compare/12efc1fe3322d16f0e703b334912241d80dbaba9...master

Your commit was automatically rebased without conflicts.

[openjdk]

@eme64 Pushed as commit e8eff4d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.