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

[Paul Sandoz]

On Mon, 26 Apr 2021 06:35:58 GMT, Xiaohong Gong <xgong at openjdk.org> wrote:

>> It's better to ask @PaulSandoz about how to better shape implementation code on JDK side.
>> 
>> IMO having an explicit null check (e.g., `Objects.requireNonNull()`) as part of argument validation in all public methods for masked operations should be a bare minimum. We already rely on the ability to propagate all the important information from the call site down to the intrinsic call.
>
> 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;
}

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

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