VectorAPI Scatter Gather Intrinsic Support

[Vladimir Ivanov]

Shravya, nice work!


While looking through the implementation, I couldn't find a compelling 
reason for the current API representation (indices as an array):

    void intoArray($type$[] a, int ix, int[] b, int iy)

and not as a vector:

    void intoArray($type$[] a, int ix, Vector<Integer,S> vecInd)

The latter form looks much more flexible and generic.

Can anybody remind me what was the motivation for such particular shape?
Sub-word types (byte, short)? Is it still relevant considering the patch 
removes relevant declarations from ByteVector/ShortVector?


Regarding implementation:

========================================================================

src/hotspot/cpu/x86/x86.ad:

+#ifdef _LP64

Why don't you put the changes in x86_64.ad then?

========================================================================

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/X-VectorBits.java.template:

+        if (VectorIntrinsics.VECTOR_ACCESS_OOB_CHECK != 0 && iy >= 
b.length) {
+          throw new ArrayIndexOutOfBoundsException("Range check failed: 
Index Map length is"
+            + b.length + " and offset is " + iy);
+        }

The check is erroneous - doesn't take vector width into accoudn. But 
it's redundant anyway. Subsequent loads perform OOB checks.

========================================================================

+#if[longOrDouble64]
+        IntVector<Vector.Shape> vecInd = intSpec.broadcast(b[iy]);
+#else[longOrDouble64]
+        IntVector<Vector.Shape> vecInd = intSpec.fromArray(b, iy);
+#end[longOrDouble64]

Why do you need a special case for Long64/Double? Doesn't fromArray 
already cover it?

========================================================================

+        if (VectorIntrinsics.VECTOR_ACCESS_OOB_CHECK != 0) {
+            // In order to do range check for gather, we need to check 
that each index is less than length.
+            IntVector<Vector.Shape> arrLen = intSpec.broadcast(a.length);
+            if (vecInd.lessThan(intSpec.zero()).anyTrue() || 
arrLen.sub(vecInd).lessThanEq(intSpec.zero()).anyTrue()) {
+                throw new ArrayIndexOutOfBoundsException("Range check 
failed: Array length is " + a.length +
+                                                         ". One of 
indices in vector was out of bounds: " + vecInd.toString());
+            }
+        }

I suggest to factor it as a vector OOB check and move it to scalar 
variant (checkIndex()) into VectorIntrinsics.


Otherwise, looks good!

Best regards,
Vladimir Ivanov

[1]
+#if[longOrDouble64]
+        IntVector<Vector.Shape> vecInd = intSpec.broadcast(b[iy]);
+#else[longOrDouble64]
+        IntVector<Vector.Shape> vecInd = intSpec.fromArray(b, iy);
+#end[longOrDouble64]


On 30/11/2018 18:19, Rukmannagari, Shravya wrote:
> Hi All,
> 
> I would like to contribute a patch that adds scatter and gather support in Java for int, float, long and double datatypes.
> 
> 
> 
> Could you please review the patch here:
> 
> http://cr.openjdk.java.net/~srukmannagar/VectorAPI_ScatterGather/webrev.00/
> 
> 
> 
> Thanks,
> 
> Shravya.
> 
>