[jdk17] RFR: 8269568: JVM crashes when running VectorMask query tests

[Jie Fu]

On Wed, 30 Jun 2021 01:45:15 GMT, Xiaohong Gong <xgong at openjdk.org> wrote:

>> IIRC we wrote them as smoke tests because they were not intrinsic. We need to think more carefully about converting them from smoke tests.
>> 
>> Ideally we should convert them to kernel tests, but that is more work. Instead we can copy the generated pattern and do the following:
>> 
>> - move the assertion outside of the loops (it will generate garbage with string concatenation)
>> - assert over arrays, thereby also moving the actual scalar computation result outside the loops. The simplest approach is to create an `int[] array` of the same length as the input and write the reduced result at index `i`. Thus it's sparse.
>> 
>> That should result in an inner loop body that is very focused on exercising the intrinsic method. It will also likely reduce the test execution times.
>
>> IIRC we wrote them as smoke tests because they were not intrinsic. We need to think more carefully about converting them from smoke tests.
>> 
> Thanks for looking at this PR @PaulSandoz ! Yes, I think this need more work and more carefully to move them from the smoke tests. Maybe we can revisit them in future?
> 
>> Ideally we should convert them to kernel tests, but that is more work. Instead we can copy the generated pattern and do the following:
>> 
>> * move the assertion outside of the loops (it will generate garbage with string concatenation)
>> * assert over arrays, thereby also moving the actual scalar computation result outside the loops. The simplest approach is to create an `int[] array` of the same length as the input and write the reduced result at index `i`. Thus it's sparse.
>> 
>> That should result in an inner loop body that is very focused on exercising the intrinsic method. It will also likely reduce the test execution times.
> 
> Agree, I will move the assertion outside of the loop first. Thanks so much!

> @XiaohongGong The following should fix this for x86:
> 
> ```
> diff --git a/src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp b/src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp
> index 147bcb8..b548877 100644
> --- a/src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp
> +++ b/src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp
> @@ -3870,6 +3870,9 @@ void C2_MacroAssembler::vector_mask_operation(int opc, Register dst, XMMRegister
>    vpsubb(xtmp, xtmp, mask, vec_enc);
>    evpmovb2m(ktmp, xtmp, vec_enc);
>    kmovql(tmp, ktmp);
> +  if (masklen < 64) {
> +    andq(tmp, (((jlong)1 << masklen) - 1));
> +  }
>    switch(opc) {
>      case Op_VectorMaskTrueCount:
>        popcntq(dst, tmp);
> @@ -3894,6 +3897,9 @@ void C2_MacroAssembler::vector_mask_operation(int opc, Register dst, XMMRegister
>    vpxor(xtmp, xtmp, xtmp, vec_enc);
>    vpsubb(xtmp, xtmp, mask, vec_enc);
>    vpmovmskb(tmp, xtmp, vec_enc);
> +  if (masklen < 64) {
> +    andq(tmp, (((jlong)1 << masklen) - 1));
> +  }
>    switch(opc) {
>      case Op_VectorMaskTrueCount:
>        popcntq(dst, tmp);
> ```
> 
> Please include this in your PR.

The fix seems to be tricky.
So do you think we need to change the code-gen like this https://github.com/openjdk/jdk17/pull/168#issuecomment-871412973 ?  @sviswa7

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

PR: https://git.openjdk.java.net/jdk17/pull/168