8337791: VectorAPI jtreg ABSMaskedByteMaxVectorTests crashes with UseAVX=0 -XX:MaxVectorSize=8

[jatin-bhateja]

This bug patch fixes a crash seen while querying the bottom type of MachTempNode corresponding to rxmm0 operand of blend pattern during late scheduling. Here, MaxVectorSize is contrainted to 8 bytes thus during C2 type system initialization, TypeVect::VECTX guarded by target supprted vector size remains uninitialized.

Its better to reject matching of VectorBlend in such a scenario.

All exisitng VectorAPI jtreg tests are passing with -XX:UseAVX=0 and -XX:MaxVectorSize=8

Kindly review and share your feedback.

Best Regards,
Jatin

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8337791: VectorAPI jtreg ABSMaskedByteMaxVectorTests crashes with UseAVX=0 -XX:MaxVectorSize=8 (Bug - P3)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28533/head:pull/28533
$ git checkout pull/28533

Update a local copy of the PR:
$ git checkout pull/28533
$ git pull https://git.openjdk.org/jdk.git pull/28533/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28533

View PR using the GUI difftool:
$ git pr show -t 28533

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28533.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jbhateja! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@jatin-bhateja The following label will be automatically applied to this pull request:

• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 07: Full - Incremental (ef84ffa7)
• 06: Full - Incremental (2f773133)
• 05: Full - Incremental (c84f473e)
• 04: Full - Incremental (a0e008de)
• 03: Full - Incremental (be24b1af)
• 02: Full - Incremental (2c08c7db)
• 01: Full - Incremental (6ca95427)
• 00: Full (70600498)

[openjdk]

@jatin-bhateja Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[jatin-bhateja]

@sviswa7, @eme64 , kindly check and approve.

[jatin-bhateja]

While its technically fesable to create a 64-bit wide byte vector blend for AVX level 0 using blendvp selection pattern and emit pblendv instruction for it, but the machine nodes inputs will not be iso-morphic, ie. MachTemp node for xmm0 will be 128 bit wide TypeVect::VECTX and other input will be of type 64bit TypeVect::VECTD type. Given that blend is a lanewise operation hence IR type must be compatible with input types. Its better to use size_in_bits rather than MaxVectorSize in guard check.

We could have pushed this check to actual pattern predicate but its better to uplift the checks to match_rule_supported_vector since it prevents creation non-matchable IR during construction.

[eme64]

@jatin-bhateja Thanks for looking into this!

I think it is fine to just disable the small vector size for AVX=0. Those platforms are quite rare now anyway.

Should we also adjust the predicate in the matching rule here?

  22509 instruct blendvp(vec dst, vec src, vec mask, rxmm0 tmp) %{
  22510   predicate(UseAVX == 0);   
  22511   match(Set dst (VectorBlend (Binary dst src) mask));                                                                                                                                                                                 
  22512   format %{ "vector_blend  $dst,$src,$mask\t! using $tmp as TEMP" %}
  22513   effect(TEMP tmp);  
  22514   ins_encode %{
  22515     assert(UseSSE >= 4, "required");
  22516 
  22517     if ($mask$$XMMRegister != $tmp$$XMMRegister) {
  22518       __ movdqu($tmp$$XMMRegister, $mask$$XMMRegister);
  22519     }
  22520     __ pblendvb($dst$$XMMRegister, $src$$XMMRegister); // uses xmm0 as mask
  22521   %}
  22522   ins_pipe( pipe_slow );
  22523 %}

[jatin-bhateja]

@jatin-bhateja Thanks for looking into this!

I think it is fine to just disable the small vector size for AVX=0. Those platforms are quite rare now anyway.

Should we also adjust the predicate in the matching rule here?

As mentioned above #28533 (comment)
Its better to lift predicates to match rule supported vector i.e. AVX == 0 and size_in_bits < 128.

[eme64]

Ok, that's fine with me too.

It would be nice if you could also attach a regression test, or maybe add an additional run to the existing test, with the required flags for reproducing this issue.

[jatin-bhateja]

Ok, that's fine with me too.

It would be nice if you could also attach a regression test, or maybe add an additional run to the existing test, with the required flags for reproducing this issue.

@eme64 addressed.

[eme64]

Are you sure this is going to pass on all platforms? Does this test run ok on aarch64 where there is no UseAVX flag?

[jatin-bhateja]

Thanks, fixed

I intend to pass UseAVX=0 as a run flag to reproduce exact bug scenario, our framework is not sensitive to IgnoreUnrecoginzedVMOptions.

[eme64]

You could also just limit the rules to sse4.1 platforms. Then you can run the tests everywhere, but limit IR rules to what is easy to test for you ;)

Platform features get tested before flags, so that helps with platform specific flags ;)

[jatin-bhateja]

Thanks!! , that is much better!
https://github.com/openjdk/jdk/tree/master/test/hotspot/jtreg/compiler/lib/ir_framework#disableenable-ir-rules-based-on-platform

[openjdk]

@jatin-bhateja Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[openjdk]

@jatin-bhateja Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[openjdk]

@jatin-bhateja Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[jatin-bhateja]

Hi @eme64 , kindly verify latest changes.

[eme64]

Testing submitted! Code looks good to me :)