On Fri, 12 Dec 2025 15:12:37 GMT, Emanuel Peter <[email protected]> wrote:
>> Eric Fang has updated the pull request with a new target base due to a merge
>> or a rebase. The pull request now contains eight commits:
>>
>> - Merge branch 'master' into JDK-8370863-mask-cast-opt
>> - Merge branch 'master' into JDK-8370863-mask-cast-opt
>> - Add MaxVectorSize IR test condition for VectorStoreMaskIdentityTest.java
>> - Refine the test code and comments
>> - Merge branch 'master' into JDK-8370863-mask-cast-opt
>> - Don't read and write the same memory in the JMH benchmarks
>> - Merge branch 'master' into JDK-8370863-mask-cast-opt
>> - 8370863: VectorAPI: Optimize the VectorMaskCast chain in specific patterns
>>
>> `VectorMaskCastNode` is used to cast a vector mask from one type to
>> another type. The cast may be generated by calling the vector API `cast`
>> or generated by the compiler. For example, some vector mask operations
>> like `trueCount` require the input mask to be integer types, so for
>> floating point type masks, the compiler will cast the mask to the
>> corresponding integer type mask automatically before doing the mask
>> operation. This kind of cast is very common.
>>
>> If the vector element size is not changed, the `VectorMaskCastNode`
>> don't generate code, otherwise code will be generated to extend or narrow
>> the mask. This IR node is not free no matter it generates code or not
>> because it may block some optimizations. For example:
>> 1. `(VectorStoremask (VectorMaskCast (VectorLoadMask x)))`
>> The middle `VectorMaskCast` prevented the following optimization:
>> `(VectorStoremask (VectorLoadMask x)) => (x)`
>> 2. `(VectorMaskToLong (VectorMaskCast (VectorLongToMask x)))`, which
>> blocks the optimization `(VectorMaskToLong (VectorLongToMask x)) => (x)`.
>>
>> In these IR patterns, the value of the input `x` is not changed, so we
>> can safely do the optimization. But if the input value is changed, we
>> can't eliminate the cast.
>>
>> The general idea of this PR is introducing an `uncast_mask` helper
>> function, which can be used to uncast a chain of `VectorMaskCastNode`,
>> like the existing `Node::uncast(bool)` function. The funtion returns
>> the first non `VectorMaskCastNode`.
>>
>> The intended use case is when the IR pattern to be optimized may
>> contain one or more consecutive `VectorMaskCastNode` and this does not
>> affect the correctness of the optimization. Then this function can be
>> called to eliminate the `VectorMaskCastNode` ch...
>
> src/hotspot/share/opto/vectornode.cpp line 1489:
>
>> 1487: Node* VectorStoreMaskNode::Identity(PhaseGVN* phase) {
>> 1488: // Identity transformation on boolean vectors.
>> 1489: // VectorStoreMask (VectorMaskCast ... VectorLoadMask bv)
>> elem_size ==> bv
>
> Suggestion:
>
> // VectorStoreMask (VectorMaskCast* VectorLoadMask bv) elem_size ==> bv
>
> Would a regex star be more explicit about 0 or more repetitions?
Yeah, done, thanks!
> src/hotspot/share/opto/vectornode.cpp line 1492:
>
>> 1490: // vector[n]{bool} => vector[n]{t} => vector[n]{bool}
>> 1491: Node* in1 = VectorNode::uncast_mask(in(1));
>> 1492: if (in1->Opcode() == Op_VectorLoadMask && length() ==
>> in1->as_Vector()->length()) {
>
> Can there be a mismatch with the length? Can you give me an example?
There are currently no such counterexamples. Because now we require the length
of `VectorMaskCastNode` to be consistent with the length of its input node. But
I'm not sure whether this restriction will be lifted in the future, and this
optimization requires the length to be the same. Because of this requirement, I
added this check. Similarly, in `uncast_mask` I also did the following assert:
`assert(n->as_Vector()->length() == in1->as_Vector()->length(), "vector length
must match");`
Do you think it would be better to change this condition to an assert?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28313#discussion_r2617758364
PR Review Comment: https://git.openjdk.org/jdk/pull/28313#discussion_r2617770569
