On Tue, 27 Oct 2026 09:25:54 GMT, Eric Fang <[email protected]> wrote:
>> src/hotspot/share/opto/genericOpts.cpp line 44:
>>
>>> 42:
>>> 43: enum class NodeTypeCon {
>>> 44: VECTOR // Vector and PVectMask (e.g. XorV / XorVMask)
>>
>> I think a separate enum value for `Vector` and `PVectMask` would be clearer.
>
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32095#discussion_r4387124074
