On Mon, 2 Mar 2026 08:05:44 GMT, Eric Fang <[email protected]> wrote:
>> I'm getting some failures with your new test:
>>
>>
>> Failed IR Rules (1) of Methods (1)
>> ----------------------------------
>> 1) Method
>> "compiler.vectorapi.VectorStoreMaskIdentityTest::testVectorMaskStoreIdentityFloat256"
>> - [Failed IR rules: 1]:
>> * @IR rule 1: "@compiler.lib.ir_framework.IR(phase={DEFAULT},
>> applyIfPlatformAnd={}, applyIfCPUFeatureOr={"sve", "true", "avx2", "true"},
>> counts={"_#V#LOAD_VECTOR_Z#_", "_@4", ">= 3", "_#VECTOR_LOAD_MASK#_", "= 0",
>> "_#VECTOR_STORE_MASK#_", "= 0", "_#VECTOR_MASK_CAST#_", "= 0"}, failOn={},
>> applyIfPlatform={}, applyIfPlatformOr={}, applyIfOr={},
>> applyIfCPUFeatureAnd={}, applyIf={"MaxVectorSize", "> 16"},
>> applyIfCPUFeature={}, applyIfAnd={}, applyIfNot={})"
>> > Phase "PrintIdeal":
>> - counts: Graph contains wrong number of nodes:
>> * Constraint 2: "(\\d+(\\s){2}(VectorLoadMask.*)+(\\s){2}===.*)"
>> - Failed comparison: [found] 1 = 0 [given]
>> - Matched node:
>> * 277 VectorLoadMask === _ 106 [[ 275 ]] #vectormask<F,4>
>> !jvms: VectorMask::fromArray @ bci:47 (line 209)
>> VectorStoreMaskIdentityTest::testTwoCastsKernel @ bci:5 (line 75)
>> VectorStoreMaskIdentityTest::testVectorMaskStoreIdentityFloat256 @ bci:29
>> (line 271)
>> * Constraint 3: "(\\d+(\\s){2}(VectorStoreMask.*)+(\\s){2}===.*)"
>> - Failed comparison: [found] 1 = 0 [given]
>> - Matched node:
>> * 254 VectorStoreMask === _ 271 272 [[ 216 ]]
>> #vectors<Z,4> !jvms: AbstractMask::intoArray @ bci:50 (line 75)
>> VectorStoreMaskIdentityTest::testTwoCastsKernel @ bci:20 (line 77)
>> VectorStoreMaskIdentityTest::testVectorMaskStoreIdentityFloat256 @ bci:29
>> (line 271)
>> * Constraint 4: "(\\d+(\\s){2}(VectorMaskCast.*)+(\\s){2}===.*)"
>> - Failed comparison: [found] 2 = 0 [given]
>> - Matched nodes (2):
>> * 271 VectorMaskCast === _ 275 [[ 254 ]] #vectormask<J,4>
>> !jvms: ShortVector64$ShortMask64::cast @ bci:54 (line 665)
>> VectorStoreMaskIdentityTest::testTwoCastsKernel @ bci:13 (line 77)
>> VectorStoreMaskIdentityTest::testVectorMaskStoreIdentityFloat256 @ bci:29
>> (line 271)
>> * 275 VectorMaskCast === _ 277 [[ 271 ]] #vectormask<S,4>
>> !orig=[5222],[2038] !jvms: FloatVector128$FloatMask128::cast @ bci:54 (line
>> 654) VectorStoreMaskIdentityTest::testTwoCastsKernel @ bci:9 (line 76)
>> VectorStoreMaskIdentityTest::testVectorMaskStoreIdentityFloat256 @ bci:29
>> (line 271)
>>
>>
>> This was run on an x64 machine wit...
>
> 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 all tests pass. Do you think this is okay? If you think
> it's ...
@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.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/28313#issuecomment-3982954331
