On Tue, 27 Oct 2026 09:38:18 GMT, Quan Anh Mai <[email protected]> wrote:
>> There are three possible approaches here:
>>
>> 1. Keep `VECTOR` and `PVECTMASK` completely separate, with two macros and
>> two separate `NodeOpsTraits` implementations.
>> - Pros:
>> - - The templates for `VECTOR` and `PVECTMASK` are instantiated at
>> compile time.
>> - - Decouple `vector` and `pvectormask` so they can evolve
>> **independently** in the future.
>> - Cons: Some code duplication.
>>
>> 2. Make no distinction between them (the current implementation).
>> - Pros: A single `NodeOpsTraits` implementation.
>> - Cons: Requires distinguishing between vector and vector mask types at
>> runtime.
>>
>> 3. Define separate `VECTOR` and `PVECTMASK` enum values, but share a single
>> `NodeOpsTraits` implementation. Add an extra template parameter,
>> `IS_PVECTMASK`, and specify its value when specializing for a node.
>> - Pros: Compile-time instantiation.
>> - Cons: An additional layer of template instantiation.
>>
>> So far, there don’t seem to be any **shared** optimizations where we cannot
>> distinguish between `vector` and `pvectormask`. Vector mask nodes typically
>> inherit from vector nodes; for example, `XorVMaskNode` inherits from
>> `XorVNode`. So for simplicity, I chose **option 2**. WDYT?
>
> It is less about writing and more about reading the code. In general, it is
> easier to reason about when the boundary is clear. It is also easier to read:
>
> static bool is_and(const Node* n) {
> return n->Opcode() == Op_AndV && !n->is_predicated_vector();
> }
>
> than:
>
> static bool is_and(const Node* n) {
> return !n->is_predicated_vector() &&
> n->Opcode() == (is_pvectmask(n) ? Op_AndVMask : Op_AndV);
> }
>
> In fact, we should default to seperation, and only sharing when there is a
> compelling reason to do so.
Hi @merykitty, I'd like to discuss this a bit further.
Another reason for having Vector and VectorMask share an implementation was
that some VectorMask nodes, such as `XorVMaskNode`, `OrVMaskNode`, and
`AndVMaskNode`, inherit from their corresponding vector nodes (`XorVNode`,
`OrVNode`, and `AndVNode`). From the node inheritance perspective, a vector
mask is therefore also a Vector. On platforms such as NEON and AVX2, which do
not have `pVectMask`, vector masks are effectively represented as vectors. So
the type distinction between vectors and vector masks is not entirely
clear-cut, perhaps partly because of how C2 defines these nodes. I'm not
opposed to separating them; just wanted to clarify a bit more on the original
approach.
Also, around line 53 of this file, we define a fixed set of interfaces. The
original intention was to require all `NodeTypeCon` types to implement these
interfaces for consistency. However, vectors and vector masks support quite
different sets of operations, and some optimizations may not be shareable
across all types. Given that, it seems reasonable not to require every type to
implement the same set of interfaces. So, should we require all types to
implement a fixed set of interfaces? I'd be interested to hear your thoughts on
this.
Thanks~
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32095#discussion_r4408667779
