On Mon, 2 Mar 2026 08:41:23 GMT, Emanuel Peter <[email protected]> wrote:
>> Hi @eme64 , I looked at this test failure. I couldn't reproduce it with a >> small java case, but I suspect the problem is related to inlining and the >> IGVN processing order. >> >> The relevant optimization is `VectorStoreMask (VectorMaskCast* >> VectorLoadMask bv) elem_size ==> bv` (in >> [VectorStoreMaskNode::Identity()](https://github.com/openjdk/jdk/pull/28313/changes#diff-692826251cae892bc4737919579c6afbd317551cd507f99c7bd29d585c1282e2R1515)). >> **This optimization doesn't seem to be triggered when the test fails**. I >> suspect the reason might be this: >> >> Because the test uses three @ForceInline helper functions >> `testOneCastKernel`, `testTwoCastKernel` and `testThreeCastKernel`, which >> might affect the inline order of the test functions. If the >> `VectorStoreMaskNode` is generated before `VectorMaskCastNode` or >> `VectorLoadMaskNode`, then the IR pattern won't match when running the >> `VectorStoreMaskNode::Identity()` function. If no further optimizations >> trigger `VectorStoreMaskNode::Identity()` to run again, then this >> optimization will ultimately not be triggered, so the relevant IR nodes are >> retained. >> >> I tried the following: >> 1. Removed all IR checks and ran the test 100 times, without failure. This >> indicates that the test failure is only due to IR matching failure, and >> there is no correctness issue. This can also be confirmed by the IR graph >> dumped after the failure. >> >> 2. Delay the optimization `VectorStoreMaskNode::Identity()` until after loop >> optimization, then run the test 100 times; all tests passed. >> >> git diff >> diff --git a/src/hotspot/share/opto/vectornode.cpp >> b/src/hotspot/share/opto/vectornode.cpp >> index e5c8e87bc25..d9ffa4835af 100644 >> --- a/src/hotspot/share/opto/vectornode.cpp >> +++ b/src/hotspot/share/opto/vectornode.cpp >> @@ -1508,6 +1508,10 @@ Node* VectorLoadMaskNode::Identity(PhaseGVN* phase) { >> } >> >> Node* VectorStoreMaskNode::Identity(PhaseGVN* phase) { >> + if (!phase->C->post_loop_opts_phase()) { >> + phase->C->record_for_post_loop_opts_igvn(this); >> + return this; >> + } >> // Identity transformation on boolean vectors. >> // VectorStoreMask (VectorMaskCast* VectorLoadMask bv) elem_size ==> bv >> // vector[n]{bool} => vector[n]{t} => vector[n]{bool} >> >> 3. Remove the three @ForceInline helper functions, run the test 100 times, >> and all passed. >> >> If my guess is correct, I believe this issue is unrelated to this PR. >> Therefore, my current approach is to remove the three @ForceInline helper >> functions. This makes... > > @erifan >> If no further optimizations trigger VectorStoreMaskNode::Identity() to run >> again, then this optimization will ultimately not be triggered, so the >> relevant IR nodes are retained. > > If this is the case, then you should find out how to trigger the optimization > again. Probably, this is a classic "missed optimization opportunity". In > theory, this should probably be caught by `-XX:VerifyIterativeGVN=1110` and > lead to an assert. But we have currently disabled the verification for all > Vector nodes, see `PhaseIterGVN::verify_Value_for`. This is an issue we > should address at some point, because it hurts us also right now with your PR. > > The missed optimization opportunity is not surprising, because your patterns > has more than a 1-hop traversal, rather it has 2+. so you'd have to implement > the notification pattern in IGVN as well. Hi @eme64 thank you for the detailed explanation. > In theory, this should probably be caught by -XX:VerifyIterativeGVN=1110 and > lead to an assert. But we have currently disabled the verification for all > Vector nodes, see PhaseIterGVN::verify_Value_for Yes, with the following change I can see the optimization opportunity was caught by `-XX:VerifyIterativeGVN=1110`: git diff diff --git a/src/hotspot/share/opto/phaseX.cpp b/src/hotspot/share/opto/phaseX.cpp index 9a9a731a022..a632065d9ba 100644 --- a/src/hotspot/share/opto/phaseX.cpp +++ b/src/hotspot/share/opto/phaseX.cpp @@ -2075,7 +2075,9 @@ void PhaseIterGVN::verify_Identity_for(Node* n) { if (n->is_Vector()) { // Found with tier1-3. Not investigated yet. // The observed issue was with AndVNode::Identity - return; + if (n->Opcode() != Op_VectorStoreMask) { + return; + } } Node* i = n->Identity(this); ``` The assert log: ``` Missed Identity optimization: Old node: dist dump --------------------------------------------- 1 4802 ConI === 0 [[ 4803 ]] #int:2 1 4652 VectorMaskCast === _ 4631 [[ 4803 ]] #vectormask<S,4> !jvms: FloatVector128$FloatMask128::cast @ bci:54 (line 654) VectorStoreMaskIdentityTest2::testThreeCastsKernel @ bci:17 (line 90) VectorStoreMaskIdentityTest2::testVectorMaskStoreIdentityLong256 @ bci:55 (line 240) 0 4803 VectorStoreMask === _ 4652 4802 [[ 4804 ]] #vectord<Z,4> !jvms: AbstractMask::intoArray @ bci:50 (line 75) VectorStoreMaskIdentityTest2::testThreeCastsKernel @ bci:24 (line 90) VectorStoreMaskIdentityTest2::testVectorMaskStoreIdentityLong256 @ bci:55 (line 240) New node: dist dump --------------------------------------------- 1 4819 AddP === _ 4818 4818 206 [[ 4820 ]] !jvms: AbstractMask::intoArray @ bci:50 (line 75) VectorStoreMaskIdentityTest2::testTwoCastsKernel @ bci:20 (line 79) VectorStoreMaskIdentityTest2::testVectorMaskStoreIdentityLong256 @ bci:29 (line 237) 1 3034 Proj === 3020 [[ 3632 4801 3048 3235 3070 4804 4185 4816 4820 4244 4072 4227 ]] #2 Memory: @ptr:BotPTR+bot, idx=Bot; !jvms: LongVector256$LongMask256::cast @ bci:54 (line 655) VectorStoreMaskIdentityTest2::testThreeCastsKernel @ bci:9 (line 88) VectorStoreMaskIdentityTest2::testVectorMaskStoreIdentityLong256 @ bci:55 (line 240) 0 4820 LoadVector === _ 3034 4819 [[ 4821 4244 4227 ]] @aryptr:bool[int:>=0] (java/lang/Cloneable,java/io/Serializable):NotNull:exact[0],iid=bot, idx=13; mismatched #vectord<Z,4> !jvms: AbstractMask::intoArray @ bci:50 (line 75) VectorStoreMaskIdentityTest2::testTwoCastsKernel @ bci:20 (line 79) VectorStoreMaskIdentityTest2::testVectorMaskStoreIdentityLong256 @ bci:29 (line 237) # # A fatal error has been detected by the Java Runtime Environment: # # Internal Error (/home/erfang/local-work/jdk/src/hotspot/share/opto/phaseX.cpp:2100), pid=82521, tid=82542 # assert(false) failed: Missed Identity optimization opportunity in PhaseIterGVN for VectorStoreMask # ... I see two ways forwad: **Option A - Implement the notification pattern**: Add logic in `add_users_of_use_to_worklist` so that when `VectorLoadMask` (or a node in the `VectorMaskCast` chain) is modified, we traverse the chain and add all `VectorStoreMask` users to the worklist. **Option B - Defer to post_loop_opts_phase (as shown above)**: I have tested this and it passes 100 runs. The downside is that all `VectorStoreMask` nodes are recorded for post_loop_opts. **Option B** is easier to do, but there seems to be no particular reason to postpone this optimization. So I prefer **Option A**. WDYT ? ------------- PR Comment: https://git.openjdk.org/jdk/pull/28313#issuecomment-3983208295
