On Mon, 15 Dec 2025 06:09:26 GMT, Jatin Bhateja <[email protected]> wrote:
>> Eric Fang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Refine code comments
>
> src/hotspot/share/opto/vectornode.cpp line 1062:
>
>> 1060: if (!in1->isa_Vector()) {
>> 1061: break;
>> 1062: }
>
> Can you write a comment here, why you want to avoid handling masks of type
> TypeVectMask ?
Hi, @jatin-bhateja, I didn't quite understand what you meant. I'm not sure if
you mistook `isa_Vector` for `isa_vectormask`. Checking `isa_Vector` here is to
ensure that `in1` is a `VectorNode`, so that it calls the `as_Vector` function.
> src/hotspot/share/opto/vectornode.cpp line 1063:
>
>> 1061: break;
>> 1062: }
>> 1063: assert(n->as_Vector()->length() == in1->as_Vector()->length(),
>> "vector length must match");
>
> While assertions are good to add, but mask cast is a lanewise operation, i.e.
> length compatibility is implied, and adding an assertion for IR invariants is
> redundant.
My main concern here is that the requirement for `VectorMaskCastNode` to have
the same length for both input and output might have been removed in the
future. I'm not sure, but we do require the lengths to be the same here, so I
added this assertion. @eme64 has a similar comment; see
https://github.com/openjdk/jdk/pull/28313/changes#r2614577536. So, if you all
think that the requirement for lane length in `VectorMaskCastNode` won't be
removed, then we can delete this assertion and the condition below.
> test/hotspot/jtreg/compiler/vectorapi/VectorStoreMaskIdentityTest.java line
> 186:
>
>> 184: testThreeCastsKernel(IntVector.SPECIES_128,
>> ShortVector.SPECIES_64, FloatVector.SPECIES_128, LongVector.SPECIES_256);
>> 185: verifyResult(IntVector.SPECIES_128.length());
>> 186: }
>
> A nit, you can define final static species like S128, S64 pointing to fully
> qualified species, it will reduce the verbosity.
Make sense, I'll do the change in the next commit.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28313#discussion_r2618171442
PR Review Comment: https://git.openjdk.org/jdk/pull/28313#discussion_r2618189379
PR Review Comment: https://git.openjdk.org/jdk/pull/28313#discussion_r2618195582
