[vectorIntrinsics+mask] RFR: 8264563: Add masked vector intrinsics for binary/store operations [v5]

[Vladimir Ivanov]

On Tue, 13 Apr 2021 08:00:23 GMT, Xiaohong Gong <xgong at openjdk.org> wrote:

>> Hi, this is the basic masking support PR for Vector API mask operations on platforms like SVE/AVX-512. The main codes are from [1], which contains:
>> 
>> - The predicate register allocation for Arm SVE, and vector mask type definition.
>> - The basic optimization for parts of the mask operations with masking feature. It contains:   
>>  
>>   1. Vector API java implementation changes for masked binary/store.
>>   2. C2 compiler mid-end changes, including new vector intrinsics implementation and mask IRs.
>> 
>> Note that for easier discussion, this PR only provides the changes for limited masked operations (e.g. binary/store) and the mask generations (e.g. load/compare/maskAll). We will continue working on the following missing parts:
>> 
>> - Mask support for other operations (unary,ternary,reduction,load,etc.)
>> - More mask IRs implementation (and/or/xor, toVector, allTrue, anyTrue, trueCount, eq, etc)
>> - Vector boxing/unboxing support for mask type (deoptimization support for predicate registers)
>> 
>> Also note that this PR doesn't contain any backend implementations. So the blend pattern will be generated as before. Regarding to the AArch64 SVE backend support, we will create a separate PR based on this one in future.
>>   
>> [1] https://github.com/openjdk/panama-vector/pull/40
>> 
>> See more details from:
>> http://cr.openjdk.java.net/~xgong/rfr/mask/Vector%20API%20masking%20support%20proposal%20for%20Arm%20SVE.pdf
>> http://cr.openjdk.java.net/~xgong/rfr/mask/VectorAPI%20masking%20support.pdf
>> 
>> Any suggestions and discussions are welcome! Thanks a lot!
>
> Xiaohong Gong has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove null checking for the mask argument in API implementation

I have one request: `m == null` should be part of internal agreement between JVM and JDK and completely hidden from users. It means that all non-masked operations (when/if they are migrated to masked intrinsics) should pass `null` constant as a mask and all masked operations should perform the null check to ensure mask value is always non-null and clearly communicate that to JITs.

I think it is important because it is a prerequisite for successful intrinsification: unless C2 can't definitely say whether the mask is `null` or non-`null`, it can't make the decision how to implement the operation. So, we should be cautious about possible inlining and profile pollution effects and ensure that all usages of `VectorSupport::binaryMaskedOp()/storeMasked()` provide enough information for C2 to detect proper operation mode.

src/hotspot/share/classfile/vmIntrinsics.hpp line 821:

> 819:    do_name(vector_binary_op_name,     "binaryOp")                                                                                              \
> 820:                                                                                                                                                \
> 821:   do_intrinsic(_VectorBinaryMaskOp, jdk_internal_vm_vector_VectorSupport, vector_binary_mask_op_name, vector_binary_mask_op_sig, F_S)          \

Should it be named `VectorBinaryMaskedOp`?

src/hotspot/share/opto/library_call.hpp line 313:

> 311:   // Vector API support
> 312:   bool inline_vector_nary_operation(int n);
> 313:   bool inline_vector_nary_mask_operation(int n);

Should it be named `inline_vector_nary_masked_operation()`?

src/hotspot/share/opto/matcher.hpp line 320:

> 318:   static const bool match_rule_supported_vector(int opcode, int vlen, BasicType bt);
> 319: 
> 320:   static const bool match_rule_supported_masked_vector(int opcode, int vlen, BasicType bt);

IMO `Matcher::match_rule_supported_vector_masked()` is a better option.

src/hotspot/share/opto/vectornode.cpp line 755:

> 753:     }
> 754:   }
> 755:   return StoreNode::Ideal(phase, can_reshape);

Can you elaborate, please, why this change is needed?

-------------

PR: https://git.openjdk.java.net/panama-vector/pull/57