8330819: C2 SuperWord: bad dominance after pre-loop limit adjustment with base that has CastLL after pre-loop

[eme64]

Summary: the address adr of the vector we want to align the main-loop for has a CastLL after the pre-loop and before the main-loop. When we use this address to adjust the pre-loop limit, we create a use before the CastLL, which leads to a "bad dominance" assert. Solution: make sure that all such base addresses adr are not just invariant in the main-loop, but also are invariant of/before the pre-loop.

Example where we get the "bad dominance"

This code shape comes from the attached regression tests (no matter if with Unsafe or MemorySegment).

The loop is PreMainPost-ed and the main-loop unrolled. 1326 CountedLoop is the pre-loop, and 1657 CountedLoop is the main-loop, which contains the 1648 LoadI. During SuperWord, we take this load's address to align the main-loop.

The address is parsed into its components by VPointer:
VPointer[mem: 1648 LoadI, base: 1, adr: 1669, base[ 1] + offset( 0) + invar( 0) + scale( 4) * iv]

We note that this is the access to native memory via Unsafe / MemorySegment, and so there is no array pointer base, and the base = 1 TOP. VPointer tries still to find a "base" adress adr by parsing the very left-most input to the chain of AddPs. Here, there is only a single 1711 AddP, and the left input is adr = 1669 CastX2P. The right side of the AddP is also parsed, and determined to be 4 * iv.

The problematic part: 1669 CastX2P is "pinned" down below the pre-loop by the 1513CastLL that is applied to 11 Parm (= long offset parameter in the test).

During SuperWord, we want to align the main-loop vectors. We do this by adjusting the pre-loop limit 1439 Opaque1:

You can see the dark-green IR nodes, which compute the new_limit = old_limit + adjustment, where the adjustment is a modulo 1734 AndI of the address of the 1738 LoadVector for which we are aligning. In this computation we are also using the adr of our VPointer, which depends on the 1513 CastLL which is pinned below the pre-loop. Thus, we are using a node inside the pre-loop that is pinned after the pre-loop. Hence the "bad dominance" assert.

Why does this happen?

Usually, the base and/or adr of a VPointer are invariant not just of the main-loop but also the pre-loop. The pinning after pre-loop and before main-loop via 1513 CastLL seems to be quite rare. The CastLL comes from the long checkIndex for the Unsafe / MemorySegment load, somehow in combination with the constant-size array / constant range of the main-loop.

Another realization: the adr is basically an addition raw_base + offset. I would have expected the offset to be an invariant and end up in the invar. But the VPointer parsing seems to only parse through the AddP, and stop at the CastX2P, and take this as the adr, even though we could parse further through the 1590 AddL, and separate the 11 Parm = long offset and the 602 LoadL = raw_base.

Solution

For now, and for the simplicity of backports, I simply check that the adr is not just is_loop_member (of the main-loop), but that it is invariant (of the main-loop and the pre-loop). We also already do this check for invariants invar, and the base/adr is essencially another invariant. That solution means that in our regression tests, we would mark the problematic VPointers as not valid, and they will not be vectorized. This case is very rare (after all, we have never hit this bad-dominance assert before), and therefore there should not be any relevant performance regression.

In a future RFE, I can then look into improving the VPointer parsing. The VPointer code is already very convoluted, and adding much more functionality will be difficult to manage. I want to completely refactor VPointer in a few months anyway. With improved parsing, this regression test could vectorize again.

See: JDK-8330991 C2 SuperWord: refactor VPointer

---

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-8330819: C2 SuperWord: bad dominance after pre-loop limit adjustment with base that has CastLL after pre-loop (Bug - P3)

Reviewers

• Christian Hagedorn (@chhagedorn - Reviewer) ⚠️ Review applies to d5e51590
• Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18892

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

Using diff file

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

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:

8330819: C2 SuperWord: bad dominance after pre-loop limit adjustment with base that has CastLL after pre-loop

Reviewed-by: chagedorn, 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 250 new commits pushed to the master branch:

• f60798a: 8329222: java.text.NumberFormat (and subclasses) spec updates
• 2555166: 8329113: Deprecate -XX:+UseNotificationThread
• 09b8809: 8327289: Remove unused PrintMethodFlushingStatistics option
• 9cc163a: 8330178: Clean up non-standard use of /** comments in java.base
• 88a5dce: 8330805: ARM32 build is broken after JDK-8139457
• 7157eea: 8327290: Remove unused notproduct option TraceInvocationCounterOverflow
• b4cea70: 8330693: Generational ZGC: Simplify ZAddress::finalizable_good and ZAddress::mark_good
• 412e306: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
• b07e153: 8330362: G1: Inline offset array element accessor in G1BlockOffsetTable
• b6518a5: 8329417: Remove objects with no pointers from relocation bitmap
• ... and 240 more: https://git.openjdk.org/jdk/compare/6bc6392d2b073434d2cfac4c5f6f2908bd8fe77e...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 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 (ffdafa80)
• 01: Full - Incremental (d5e51590)
• 00: Full (9be1e090)

[chhagedorn]

That looks good to me.

In a future RFE, I can then look into improving the VPointer parsing. The VPointer code is already very convoluted, and adding much more functionality will be difficult to manage. I want to completely refactor VPointer in a few months anyway. With improved parsing, this regression test could vectorize again.

Sounds good! Maybe you can link this bug to this RFE (if there is already one) to not forget about updating this test to check if it indeed vectorizes.

[eme64]

JDK-8330991 C2 SuperWord: refactor VPointer
Here the RFE.

[vnkozlov]

Good.

[eme64]

Thanks @vnkozlov @chhagedorn for the reviews!
/integrate

[openjdk]

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

• c439c8c: 8323429: Missing C2 optimization for FP min/max when both inputs are same
• 165ba87: 8330587: IGV: remove ControlFlowTopComponent
• 3ccb64c: 8318446: C2: optimize stores into primitive arrays by combining values into larger store
• 5c38386: 8326541: [AArch64] ZGC C2 load barrier stub should consider the length of live registers when spilling registers
• 438e643: 8329531: compiler/c2/irTests/TestIfMinMax.java fails with IRViolationException: There were one or multiple IR rule failures.
• 80b381e: 8329555: Crash in intrinsifying heap-based MemorySegment Vector store/loads
• 7a89555: 8330844: Add aliases for conditional jumps and additional instruction forms for x86
• f60798a: 8329222: java.text.NumberFormat (and subclasses) spec updates
• 2555166: 8329113: Deprecate -XX:+UseNotificationThread
• 09b8809: 8327289: Remove unused PrintMethodFlushingStatistics option
• ... and 247 more: https://git.openjdk.org/jdk/compare/6bc6392d2b073434d2cfac4c5f6f2908bd8fe77e...master

Your commit was automatically rebased without conflicts.

[openjdk]

@eme64 Pushed as commit e681e9b.

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