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

[Paul Sandoz]

On Mon, 26 Apr 2021 16:03:41 GMT, Paul Sandoz <psandoz at openjdk.org> wrote:

>> Hi @iwanowww , I just updated the codes according to your comment. Would you mind having a look again? Thanks so much!
>
>> > @XiaohongGong See the following diff which more accurately depicts what i was suggesting:
>> > https://gist.github.com/PaulSandoz/d93fa2a78c3c7be5fdb2d714371639ce
>> > i.e. we follow the exiting template pattern more directly in both the non-mask and mask.
>> > Perhaps we find later on we can consolidate, but right now i find that pattern easier to follow and it is very clear on the null contract for no mask.
>> 
>> Thanks for your suggestion! I also tried this way before the current version. However, the main concern for me is the species checking for vector mask. See the codes:
>> 
>> ```
>>     @Override
>>     @ForceInline
>>     public final Int128Vector lanewise(Binary op, Vector<Integer> v, VectorMask<Integer> m) {
>>         return (Int128Vector) super.lanewiseTemplate(op, Int128Mask.class, v, (Int128Mask) m);  // specialize
>>     }
>> ```
>> 
>> The override `lanewise` method must cast the mask value to `Int128Mask` before it calls the super class's `lanewiseTemplate` which does the species checking later. It seems more reasonable for me to do the species checking before doing the type casting. That means the species checking must be done in the `lanewise` method while not `lanewiseTemplate`. Add considering adding the species checking for each subclass's `lanewise` seems duplicate, finally I change the codes to the current style.
> 
> Can you explain more about why it is a concern? Is there some limitation you are observing with HotSpot? (I could be forgetting something, this is a long thread!)
> 
> I wonder if the cast is now required given a form of species check. Note that the argument vector (`v` or `that`) is not cast, although there is an equality check on classes in `sameSpecies`:
> 
> 
>     @ForceInline
>     @SuppressWarnings("unchecked")
>     /*package-private*/ final
>     <F> AbstractVector<F> check(Vector<F> other) {
>         if (!sameSpecies(other)) {
>             throw AbstractSpecies.checkFailed(this, other);
>         }
>         return (AbstractVector<F>) this;
>     }
> 
>     @ForceInline
>     private boolean sameSpecies(Vector<?> other) {
>         // It's simpler and faster to do a class check.
>         boolean same = (this.getClass() == other.getClass());
>         // Make sure it works, too!
>         assert(same == (this.species() == other.species())) : same;
>         return same;
>     }
> 
> 
> Can we follow a similar pattern e.g. `maskClass == m.getClass()`? Perhaps with check on `AbstractMask` and do `m.check(maskClass, v);` to induce an NPE before checking?:
> 
> @ForceInline
> @SuppressWarnings("unchecked")
> /*package-private*/ final
> <F> AbstractMask<F> check(Class<? extends VectorMask<F>> maskClass, Vector<F> other) {
>     if (!sameSpecies(maskClass, other)) {
>         throw AbstractSpecies.checkFailed(this, other);
>     }
>     return (AbstractMask<F>) this;
> }
> 
> @ForceInline
> <F> boolean sameSpecies(Class<? extends VectorMask<F>> maskClass, Vector<F> other) {
>     same = this.getClass() == maskClass;
>     assert (same == (vectorSpecies() == v.species())) : same;
>     return same;
> }

> @PaulSandoz , thanks for your comments! It can work well without the casting for mask. I'v updated the codes and separate the `lanewiseTemplate ` for both masked and non-masked operations. Thanks!
> 

Very good!

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

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