8380424: C2: Fix missing identity optimization for vector nodes

[erifan]

Ideal and Identity optimizations require all input nodes of the IR pattern to be ready for the optimization to take effect. However, node generation in the incremental inlining phase is unordered, so sometimes downstream nodes in the IR pattern are generated before upstream nodes, causing Ideal or Identity optimizations to miss. If no subsequent process triggers the optimization again, the optimization misses forever.

Vector nodes (especially generated by VectorAPI) are often wrapped using VectorBoxNode during generation, and the existence of these box nodes and unbox nodes further hinders the matching of IR optimization patterns. The -XX:VerifyIterativeGVN option allows us to check which IGVN optimizations are missed; however, currently, the verification for Vector nodes is skipped. Enabling the Identity optimization check for vector nodes shows that many tests fail, as shown below.

jdk/incubator/vector/ByteVector128LoadStoreTests.java
jdk/incubator/vector/ByteVector256LoadStoreTests.java
jdk/incubator/vector/ByteVector512LoadStoreTests.java
jdk/incubator/vector/ByteVector64LoadStoreTests.java
jdk/incubator/vector/ByteVectorMaxLoadStoreTests.java
jdk/incubator/vector/ShortVector128LoadStoreTests.java
jdk/incubator/vector/ShortVector256LoadStoreTests.java
jdk/incubator/vector/ShortVector512LoadStoreTests.java
jdk/incubator/vector/ShortVector64LoadStoreTests.java
jdk/incubator/vector/ShortVectorMaxLoadStoreTests.java
jdk/incubator/vector/Vector512ConversionTests.java
jdk/incubator/vector/Vector64ConversionTests.java#id0
jdk/incubator/vector/VectorMaxConversionTests.java#id0

They are caused by the missed optimizations of AndVNode::Identity() and ShiftVNode::Identity(). And from JDK-8370863, we know that VectorStoreMaskNode::Identity() may miss as well.

To recover these potential missed optimizations, we need to trigger them again at appropriate points. Currently, a GVN optimization runs once during node generation, and if no subsequent changes are made, the node will not be added to the IGVN worklist to trigger IGVN optimization again. Therefore, the corresponding nodes need to be added to the IGVN worklist at appropriate points.

Many phases affect the shape of the node tree, but inlining and boxing have a particularly significant impact on vector nodes. After PhaseVector, inlining is complete, and vector boxing/unboxing has been eliminated. At this point, the node tree is fully materialized, with no additional interfering nodes. Therefore, this PR adds all nodes to the IGVN worklist at this point to recover potentially missed GVN optimizations.

However, this modification still cannot handle the situation after PhaseVector, so this PR also enhances the notification of multi-hop IR optimization patterns in add_users_of_use_to_worklist.

With this PR, the above test failures passed in 100 tests, so this PR enables identity optimization verification for vector nodes. We expect that with this PR, there will be very few cases of Vector identity optimization misses; if they do occur, we should fix them rather than skip them.

This PR does not enable Ideal optimization verification for vector nodes because the inputs of some commutative nodes may be swapped in Ideal, causing changes in the hash value, which could lead to verification failure.

We also found many test failures caused by the missing of ShenandoahLoadReferenceBarrierNode::Identity(). This PR skipped the identity verification of the ShenandoahLoadReferenceBarrierNode because it was not investigated in this PR.

This PR tested all tier1 to tier3 jtreg tests on aarch64 (sve, neon) and x64 (avx3, avx2) platforms using options -ea -esa -XX:-TieredCompilation -XX:CompileThreshold=100 -XX:VerifyIterativeGVN=1110, and repeated the test 100 times for the aforementioned error cases. All tests passed.

---

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

Progress

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

Issue

• JDK-8380424: C2: Fix missing identity optimization for vector nodes (Enhancement - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 30529

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@erifan 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 (fc9b8dff)
• 03: Full (96c5e767)
• 02: Full (1e39fcde)
• 01: Full (1167094e)
• 00: Full (1167094e)

[galderz]

Thanks for the PR!

[galderz]

Is there any risk that this produces a lot of nodes that are unrelated to what you are trying to achieve here? From what I can see identity_useful_node seems to add the entire graph of live nodes and that seems to me that it could be a lot. Maybe this works work well when the graph has a lot of vector nodes, but if the graph has a mix of scalar and vector nodes maybe this could get out of hand?

Another thing that is noticeable is that PhaseVector::do_cleanup calls PhaseRemoveUseless just before add_all_nodes_into_igvn_worklist is called, and PhaseRemoveUseless also invokes identify_useful_nodes and stores them into _useful. This is accessible via PhaseRemoveUseless::get_useful. Could you piggyback on that?

[erifan]

Thanks, good point.

I did a quick compile-time check on a Neoverse-V2 machine with the same case:

  // mul add int vd = va * vb + vc
  public static void testMulAddInt() {
    for (int i = 0; i < LENGTH; i += I_SPECIES.length()) {
      IntVector va = IntVector.fromArray(I_SPECIES, ia, i);
      IntVector vb = IntVector.fromArray(I_SPECIES, ib, i);
      IntVector vc = IntVector.fromArray(I_SPECIES, ic, i);
      va.mul(vb).add(vc).intoArray(ir, i);
    }
  }

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

java -Xbatch -XX:-UseOnStackReplacement -XX:CompileCommand="compileonly,Test::test*" -XX:-TieredCompilation -XX:CompileCommand="print,Test::test*" -XX:+UnlockDiagnosticVMOptions -XX:+LogCompilation Test.java > assembly.s

I ran both jdk-master and this PR build 10 times and measured the C2 compile time for Test::testMulAddInt from hotspot_pid*.log. The averages were 38.3 ms on jdk-master and 37.6 ms with this PR, so I do not see a noticeable compile-time regression from this change.

build	avg	min	max	stdev
master	38.3 ms	37.0 ms	40.0 ms	0.82 ms
PR	37.6 ms	37.0 ms	39.0 ms	0.70 ms

Also, as Peter suggested earlier in #28313. "Adding all nodes might be the most reliable in the long-run. We may at some point have idealization rules that start at a scalar node, and traverse up to find vector nodes."

I also took your suggestion and now reuse PhaseRemoveUseless::get_useful() instead of recomputing the useful-node set.

[galderz]

Not in this class, but should there be additional IR tests for AndV and ShiftV?

[erifan]

After we enable identity check for vector nodes, with options -ea -esa -XX:-TieredCompilation -XX:CompileThreshold=100 -XX:VerifyIterativeGVN=1110, test files like jdk/incubator/vector/ByteVector128LoadStoreTests.java consistently fails; therefore I think there's no need to add any additional tests for AndV and ShiftV.

For this test, it fails intermittently, so I feel it is necessary to add a stress test.

[openjdk-notifier]

The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork:

git checkout JDK-8380424-miss-identity-opt
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

[openjdk]

@erifan 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-8380424-miss-identity-opt
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]

The total number of required reviews for this PR has been set to 2 based on the presence of this label: hotspot-compiler. This can be overridden with the /reviewers command.

[erifan]

/template append

[openjdk]

@erifan The pull request template has been appended to the pull request body

[erifan]

Thanks for your review!

[erifan]

Thanks, good point.

I did a quick compile-time check on a Neoverse-V2 machine with the same case:

  // mul add int vd = va * vb + vc
  public static void testMulAddInt() {
    for (int i = 0; i < LENGTH; i += I_SPECIES.length()) {
      IntVector va = IntVector.fromArray(I_SPECIES, ia, i);
      IntVector vb = IntVector.fromArray(I_SPECIES, ib, i);
      IntVector vc = IntVector.fromArray(I_SPECIES, ic, i);
      va.mul(vb).add(vc).intoArray(ir, i);
    }
  }

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

java -Xbatch -XX:-UseOnStackReplacement -XX:CompileCommand="compileonly,Test::test*" -XX:-TieredCompilation -XX:CompileCommand="print,Test::test*" -XX:+UnlockDiagnosticVMOptions -XX:+LogCompilation Test.java > assembly.s

I ran both jdk-master and this PR build 10 times and measured the C2 compile time for Test::testMulAddInt from hotspot_pid*.log. The averages were 38.3 ms on jdk-master and 37.6 ms with this PR, so I do not see a noticeable compile-time regression from this change.

build	avg	min	max	stdev
master	38.3 ms	37.0 ms	40.0 ms	0.82 ms
PR	37.6 ms	37.0 ms	39.0 ms	0.70 ms

Also, as Peter suggested earlier in #28313. "Adding all nodes might be the most reliable in the long-run. We may at some point have idealization rules that start at a scalar node, and traverse up to find vector nodes."

I also took your suggestion and now reuse PhaseRemoveUseless::get_useful() instead of recomputing the useful-node set.

[erifan]

After we enable identity check for vector nodes, with options -ea -esa -XX:-TieredCompilation -XX:CompileThreshold=100 -XX:VerifyIterativeGVN=1110, test files like jdk/incubator/vector/ByteVector128LoadStoreTests.java consistently fails; therefore I think there's no need to add any additional tests for AndV and ShiftV.

For this test, it fails intermittently, so I feel it is necessary to add a stress test.

[erifan]

Hi @eme64 @galderz This PR aims to fix the missing identity optimizations for some vector nodes, would you mind taking look, thanks~

[eme64]

@erifan Thanks for working on this! It is important to get the Vector API nicely optimized.

I'm currently extremely busy with high-priority tasks. I'll get back to this later.
Maybe @iwanowww can also help review here.

[erifan]

@erifan Thanks for working on this! It is important to get the Vector API nicely optimized.

I'm currently extremely busy with high-priority tasks. I'll get back to this later. Maybe @iwanowww can also help review here.

Ok, that's fine.
Hi @iwanowww , when you have a chance, could you please help review this PR? Thanks

[erifan]

Hello, could someone please help review this PR? It fixes some missing vector identity optimizations.