On Tue, 27 Oct 2026 08:01:41 GMT, Quan Anh Mai <[email protected]> wrote:
>> Eric Fang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove the unrelated files that were accidentally included
>
> 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?
> src/hotspot/share/opto/genericOpts.hpp line 40:
>
>> 38: // the caller's convention, e.g. nullptr for Ideal and the root for
>> Identity.
>> 39: // So each optimization is declared with this macro.
>> 40: #define DECLARE_GENERIC_OPT(opt_name) template<typename NodeClass> \
>
> I would strongly advise against macro here.
The idea here is to limit the framework’s scope slightly, so that adding all
kinds of optimizations doesn’t also introduce hard-to-maintain workarounds and
make the framework increasingly difficult to maintain. Could you elaborate on
your concerns about using **macros**?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32095#discussion_r4387004777
PR Review Comment: https://git.openjdk.org/jdk/pull/32095#discussion_r4386808183
