8300257: C2: vectorization fails on some simple Memory Segment loops

[rwestrel]

In the test case testByteLong1 (that's extracted from a memory
segment micro benchmark), the address of the store is initially:

(AddP#204 base#195 base#195 (AddL#164 (ConvI2L#158 (CastII#157 (LshiftI#107 iv#101))) invar#163))

(#numbers are node numbers to help the discussion).

iv#101 is the Phi of a counted loop. invar#163 is the
baseOffset load.

To eliminate the range check, the loop is transformed into a loop nest
and as a consequence the address above becomes:

(AddP#204 base#195 base#195 (AddL#164 (ConvI2L#158 (CastII#157 (LShiftI#107 (AddI#326 invar#308 iv#321)))) invar#163))

invar#308 is some expression from a Phi of the outer loop.

That AddP is transformed multiple times to push the invariants out of loop:

(AddP#568 base#195 (AddP#556 base#195 base#195 invar#163) (ConvI2L#158 (CastII#157 (AddI#566 (LShiftI#565 iv#321) invar#577))))

then:

(AddP#568 base#195 (AddP#847 (AddP#556 base#195 base#195 invar#163) (AddL#838 (ConvI2L#793 (LShiftL#760 iv#767)) (ConvI2L#818 (CastII#779 invar#577)))))

and finally:

(AddP#568 base#195 (AddP#949 base#195 (AddP#855 base#195 (AddP#556 base#195 base#195 invar#163) (ConvI2L#818 (CastII#809 invar#577))) (ConvI2L#938 (LShiftI#896 iv#908))))

AddP#855 is out of the inner loop.

This doesn't vectorize because:

• there are 2 invariants in the address expression but superword only
  support one (tracked by _invar in SWPointer)

• there are more levels of AddP (4) than superword supports (3)

To fix that, I propose to no longer track the address elements in
_invar, _negate_invar and _invar_scale but instead to have a
single _invar which is an expression built by superword as it
follows chains of addP nodes. I kept the previous _invar,
_negate_invar and _invar_scale as debugging and use them to check
that what vectorized with the previous scheme still does.

I also propose lifting the restriction on 3 levels of AddP entirely.

---

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-8300257: C2: vectorization fails on some simple Memory Segment loops

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to dbf08c38
• Tobias Hartmann (@TobiHartmann - Reviewer) ⚠️ Review applies to 35b94989

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12942

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back roland! 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]

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

• 04: Full (8a08447f)
• 03: Full (35b94989)
• 02: Full - Incremental (d4d07656)
• 01: Full (cdcc181c)
• 00: Full (dbf08c38)

[vnkozlov]

So the final expression should be seen by superword as next since AddP#855 is invariant for inner loop:

(AddP#568 base#195 (AddP#949 base#195 (invar#855) (ConvI2L#938 (LShiftI#896 iv#908))))

First, I think you messed up with (). AddP node should have 3 inputs: base, address and offset. I don't see offset for AddP#568.
Second, if superword can mark AddP#855 as invariant (no need to parse it) your address expression become simple.

[rwestrel]

So the final expression should be seen by superword as next since AddP#855 is invariant for inner loop:

(AddP#568 base#195 (AddP#949 base#195 (invar#855) (ConvI2L#938 (LShiftI#896 iv#908))))

First, I think you messed up with (). AddP node should have 3 inputs: base, address and offset. I don't see offset for AddP#568. Second, if superword can mark AddP#855 as invariant (no need to parse it) your address expression become simple.

I left the constant part of the address out of the expressions. So that was on purpose but confusing, sorry.
The current logic only sets the invariant to the result of an integer operation and otherwise follows through the AddP and checks that all of them have the same base. So If we wanted to use the AddP as invariant, we would need to change the logic so it stops as the first AddP that's loop invariant, we would likely not check that all AddP nodes have the same base and we would need to cast the AddP to an integer to compute the loop alignment.
Do you think that's better that what I'm proposing? I don't really have a strong opinion. Your suggestion is likely a smaller change.

[vnkozlov]

So we need to check all AddP to find base offset. No short cuts then :(
I agree with your proposal with invariants. Last time I touched this code it was one of pain points that we have only one _invar. Any improvements to that is welcome.
I will start testing while looking on changes.

[vnkozlov]

My tier1-4,xcomp and stress testing passed.
I looked and changes and they seems fine.
May be we need to run performance testing too.

[openjdk]

@rwestrel 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-8300257
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[openjdk]

@rwestrel 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:

8300257: C2: vectorization fails on some simple Memory Segment loops

Reviewed-by: kvn, thartmann

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 33 new commits pushed to the master branch:

• c67bbce: 8305711: Arm: C2 always enters slowpath for monitorexit
• ce6e746: 8305118: Add RISC-V related content to building.md
• 314e9b3: 8300829: Make CtwRunner available as an independent tool
• 0a34018: 8305680: Remove Permissions from jcmd help output
• 8db1dd0: 8305622: Remove Permission details from jcmd man page
• 1d517af: 8305209: JDWP exit error AGENT_ERROR_INVALID_THREAD(203): missing entry in running thread table
• 08fbb7b: 8272119: Typo in JDK documentation (a -> an)
• 536ad9d: 8305461: [vectorapi] Add VectorMask::xor
• ddd50d0: 8305608: Change VMConnection to use "test.class.path"instead of "test.classes"
• ce10460: 8274166: Some CDS tests ignore -Dtest.cds.runtime.options
• ... and 23 more: https://git.openjdk.org/jdk/compare/b659c35395fd3ac284a8fccfc04a4adf97438fb5...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.

[TobiHartmann]

Looks good to me. I'll run some perf testing and report back.

[TobiHartmann]

It feels like maybe_negate_invar should be moved into maybe_add_to_invar and be controlled by a negate argument.

[rwestrel]

Thanks for the review. I applied your suggestions and made that change too.

[TobiHartmann]

Performance and correctness testing looks good.

[TobiHartmann]

Thanks for making these changes. SWPointer::maybe_negate_invar could now be removed as it has only one user but I'm also fine with leaving it as is.

[rwestrel]

Thanks for making these changes. SWPointer::maybe_negate_invar could now be removed as it has only one user but I'm also fine with leaving it as is.

Thanks for looking at this again.
Because it has a single use? The method is fairly simple but non trivial so I'll leave it as a separate method.

[rwestrel]

/integrate

[openjdk]

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

• dc81603: 8305666: Add system property for fair AWT lock
• c67bbce: 8305711: Arm: C2 always enters slowpath for monitorexit
• ce6e746: 8305118: Add RISC-V related content to building.md
• 314e9b3: 8300829: Make CtwRunner available as an independent tool
• 0a34018: 8305680: Remove Permissions from jcmd help output
• 8db1dd0: 8305622: Remove Permission details from jcmd man page
• 1d517af: 8305209: JDWP exit error AGENT_ERROR_INVALID_THREAD(203): missing entry in running thread table
• 08fbb7b: 8272119: Typo in JDK documentation (a -> an)
• 536ad9d: 8305461: [vectorapi] Add VectorMask::xor
• ddd50d0: 8305608: Change VMConnection to use "test.class.path"instead of "test.classes"
• ... and 24 more: https://git.openjdk.org/jdk/compare/b659c35395fd3ac284a8fccfc04a4adf97438fb5...master

Your commit was automatically rebased without conflicts.

[openjdk]

@rwestrel Pushed as commit 6b2a86a.

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