RFR: 8332119: Incorrect IllegalArgumentException for C2 compiled permute kernel [v3]

[Emanuel Peter]

On Wed, 5 Jun 2024 02:34:26 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>> Currently inline expansion of vector to shuffle conversion simply type casts the vector holding indexes to byte vector[1] where as fallback implementation[2] also wraps the indexes to a valid index range [0, VEC_LEN-1) or generates a -ve index for exceptional / OOB indices.
>> 
>> This patch extends the conversion inline expander to match the fall back implementation. This imposes around 20% performance tax on Vector.toShuffle() intrinsic but fixes this functional bug.
>> 
>> Kindly review and share your feedback.
>> 
>> Best Regards,
>> Jatin
>> 
>> PS: Patch also fixes an incorrectness issue reported with [JDK-8332118](https://bugs.openjdk.org/browse/JDK-8332118)
>> 
>> [1] https://github.com/openjdk/jdk/blob/master/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/FloatVector.java#L2352
>> [2] https://github.com/openjdk/jdk/blob/master/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/AbstractShuffle.java#L58
>
> Jatin Bhateja has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Review comments resolutions.

Changes requested by epeter (Reviewer).

src/hotspot/share/opto/vectorIntrinsics.cpp line 525:

> 523: // indexes.
> 524: Node* LibraryCallKit::partially_wrap_indexes(Node* index_vec, int num_elem, BasicType elem_bt) {
> 525:   assert(elem_bt == T_BYTE, "");

Write message in assert: why is it limited to byte?

src/hotspot/share/opto/vectorIntrinsics.cpp line 530:

> 528: 
> 529:   Node* mod_val = gvn().makecon(TypeInt::make(num_elem-1));
> 530:   Node* bcast_mod  = gvn().transform(VectorNode::scalar2vector(mod_val, num_elem, type_bt));

Naming issue: this is not the result of the mod, so "mod" is a bit misleading. I would use `mask`, as it is used as a mask in the AndV below.

test/hotspot/jtreg/compiler/vectorapi/TestTwoVectorPermute.java line 29:

> 27: * @summary Incorrect IllegalArgumentException for C2 compiled permute kernel
> 28: * @modules jdk.incubator.vector
> 29: * @requires vm.compiler2.enabled

Is this necessary to restrict to C2? Maybe this test tickles something for other compilers as well.

test/hotspot/jtreg/compiler/vectorapi/TestTwoVectorPermute.java line 32:

> 30: * @library /test/lib /
> 31: * @run main/othervm -XX:+UnlockDiagnosticVMOptions -Xbatch -XX:-TieredCompilation -XX:CompileOnly=TestTwoVectorPermute::micro compiler.vectorapi.TestTwoVectorPermute
> 32: * @run main/othervm -XX:+UnlockDiagnosticVMOptions -Xbatch -XX:-TieredCompilation compiler.vectorapi.TestTwoVectorPermute

I would also add a run without `-XX:-TieredCompilation`, that could lead to different compilation patterns, and increase our test coverage.

test/hotspot/jtreg/compiler/vectorapi/TestTwoVectorPermute.java line 44:

> 42:    public static final VectorSpecies<Float> FSP = FloatVector.SPECIES_256;
> 43: 
> 44:    public static void validate(float [] res, float [] shuf, float [] src1, float [] src2) {

Suggestion:

   public static void validate(float[] res, float[] shuf, float[] src1, float[] src2) {

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

PR Review: https://git.openjdk.org/jdk/pull/19442#pullrequestreview-2098143389
PR Review Comment: https://git.openjdk.org/jdk/pull/19442#discussion_r1627046199
PR Review Comment: https://git.openjdk.org/jdk/pull/19442#discussion_r1627047910
PR Review Comment: https://git.openjdk.org/jdk/pull/19442#discussion_r1627062793
PR Review Comment: https://git.openjdk.org/jdk/pull/19442#discussion_r1627063960
PR Review Comment: https://git.openjdk.org/jdk/pull/19442#discussion_r1627065037