On Fri, 18 Sep 2026 18:16:25 GMT, Vladimir Ivanov <[email protected]> wrote:
>> Eric Fang has updated the pull request with a new target base due to a merge
>> or a rebase. The incremental webrev excludes the unrelated changes brought
>> in by the merge/rebase. The pull request contains four additional commits
>> since the last revision:
>>
>> - Merge branch 'master' into JDK-8384571-vector-blend-opt-pr1
>> - Add JMH and JTReg tests for each optimization pattern and each type
>> - Merge branch 'master' into JDK-8384571-vector-blend-opt-pr1
>> - 8384571: C2: Add some basic IGVN optimization for VectorBlendNode
>>
>> This PR introduces the basic Ideal/Identity transformations for
>> `VectorBlendNode`.
>>
>> The semantic of `VectorBlend(X, Y, M)` is: `M ? Y : X`.
>>
>> **Identity**:
>> ```
>> (VectorBlend X Y (Replicate -1)) => Y
>> (VectorBlend X Y (MaskAll -1)) => Y
>> (VectorBlend X Y (Replicate 0)) => X
>> (VectorBlend X Y (MaskAll 0)) => X
>> ```
>>
>> **Ideal**:
>> ```
>> (VectorBlend (VectorBlend X A M) B M) => (VectorBlend X B M)
>> (VectorBlend A (VectorBlend B X M) M) => (VectorBlend A X M)
>> (VectorBlend A B (XorV/XorVMask M -1)) => (VectorBlend B A M)
>> ```
>>
>> Also corrects the VectorBlendNode header comment: across all backends
>> (X86 SSE/AVX, AArch64 NEON/SVE, RISC-V V) the active mask lane selects
>> `vec2` (in(2)), and the inactive lane selects `vec1` (in(1)).
>>
>> JTReg and JMH tests are also added for each optimization pattern. All
>> tests (tier1, tier2, and tier3) passed on AArch64 and X86 platforms.
>>
>> JMH benchmark test results:
>>
>> On a Nvidia Grace (Neoverse-V2) machine with 128-bit SVE2:
>> ```
>> Benchmark Unit Before Error After Error Uplift
>> blendNegatedMaskInt ops/ms 7990.6 2.8 10215.2 11.0
>> 1.3
>> identityAllOnesInt ops/ms 3574.8 2.6 7967.1 0.3
>> 2.2
>> identityAllZerosLong ops/ms 3575.6 1.0 7966.0 3.6
>> 2.2
>> nestedBlendInnerLong ops/ms 3533.8 2.8 478573.0 3178.5
>> 135.4
>> nestedBlendOuterInt ops/ms 3537.6 3.4 472242.2 3034.2
>> 133.5
>> ```
>>
>> On an AWS Graviton3 (Neoverse-V1) machine with 256-bit SVE1:
>> ```
>> Benchmark Unit Before Error After Error Uplift
>> blendNegatedMaskInt ops/ms 5171.9 5.2 8129.0 17.3
>> 1.6
>> identityAllOnesInt ops/ms 2722.0 0.1 5891.3 0.1
>> 2.2
>> identityAllZerosLong ops/ms 2722.4 0.1 5891.1 0.3
>> 2.2
>> nestedBlendInnerLong ops/ms 2697.6 0.0 312148.7 2366.4
>> 115.7
>> nestedBlendOuterInt ops/ms 2702.7 0.1 308686.0 2709.8
>> 114.2
>> ```
>>
>> On...
>
> src/hotspot/share/opto/vectornode.cpp line 2919:
>
>> 2917:
>> 2918: // (VectorBlend A B (XorV/XorVMask M -1)) => (VectorBlend B A M)
>> 2919: Node* uncasted_mask = uncast_mask(mask);
>
> A simpler alternative is to detect negated mask shape and swap arguments
> along with double negating the mask:
>
> VectorBlend A B (NotV M) ==> VectorBlend B A (NotV (NotV M)) ==> VectorBlend
> B A M
Hi @iwanowww thanks for your review! Your suggestion makes sense to me.
However, after some trials, I do not see a clear simplification compared with
the current approach. I have two questions—could you help clarify them?
1. The optimization `NotV(NotV M) => M` has not been implemented yet. Should we
open a separate PR to add that optimization first, or should we just include it
in this PR?
2. If we convert `VectorBlend A B (NotV M) => VectorBlend B A (NotV (NotV M))`,
then this optimization will depend on `NotV(NotV M) => M`. That means the two
optimizations would not be well decoupled, which does not seem ideal from a
design perspective. Is your concern that the two optimizations may share some
code? If so, perhaps extracting a helper function would be sufficient. What do
you think?
> src/hotspot/share/opto/vectornode.cpp line 2949:
>
>> 2947: // (VectorBlend X Y (Replicate -1)) => Y
>> 2948: // (VectorBlend X Y (MaskAll -1)) => Y
>> 2949: if (VectorNode::is_all_ones_vector(uncast_mask(in(3)))) {
>
> I suggest to either enhance `VectorNode::is_all_ones_vector` to cover vector
> masks or introduce a helper method. Explicit vector mask uncasting
> (`uncast_mask()`) is error-prone and harder to read.
Make sense, I'll add a new helper to do this, avoid changing the semantics of
`VectorNode::is_all_ones_vector`.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/31333#discussion_r4072277539
PR Review Comment: https://git.openjdk.org/jdk/pull/31333#discussion_r4072293030
