VectorAPI Scatter Gather Intrinsic Support

[Rukmannagari, Shravya]

Hi Vladimir,
Please find the comments below and a modified patch with the changes suggested:
http://cr.openjdk.java.net/~srukmannagar/VectorAPI_ScatterGather/webrev.01/

>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?
This is a factory method and all the currently implemented intrinsics take array as the input, which is more generic to the user than a vector. Also, this was a use case provided by Adam Pocock and we therefore decided to go with the current implementation. 

>Regarding implementation:
>
>========================================================================
>
>src/hotspot/cpu/x86/x86.ad:
>
>+#ifdef _LP64
>
>Why don't you put the changes in x86_64.ad then?
x86_64.ad file does not have few registers and variables defined. For instance vecZ is undefined.

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

>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.
Done, please find the changes in the latest patch
========================================================================

>+        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.
Done, please find the changes in the latest patch

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.
> 
>