8328181: C2: assert(MaxVectorSize >= 32) failed: vector length should be >= 32 #18464

jatin-bhateja · 2024-03-24T09:58:59Z

This bug fix patch tightens the predication check for small constant length clear array pattern and relaxes associated feature checks. Modified few comments for clarity.

Kindly review and approve.

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-8328181: C2: assert(MaxVectorSize >= 32) failed: vector length should be >= 32 (Bug - P3)

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer)
Vladimir Ivanov (@iwanowww - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18464

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

Using diff file

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

Webrev

Link to Webrev Comment

… be >= 32

bridgekeeper · 2024-03-24T09:59:34Z

👋 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 · 2024-03-24T10:00:21Z

@jatin-bhateja This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8328181: C2: assert(MaxVectorSize >= 32) failed: vector length should be >= 32

Reviewed-by: kvn, vlivanov

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 182 new commits pushed to the master branch:

e702646: 8200566: DistributionPointFetcher fails to fetch CRLs if the DistributionPoints field contains more than one DistributionPoint and the first one fails
7e5ef79: 8323116: [REDO] Computational test more than 2x slower when AVX instructions are used
9467720: 8329875: Serial: Move preservedMarks.inline.hpp to serialFullGC.cpp
a4dd2e9: 8329766: Serial: Refactor SerialBlockOffsetTable API
212a253: 8329623: NegativeArraySizeException encoding large String to UTF-8
dd930c5: 8329787: Fix typo in CLDRConverter
115f419: 8329659: Serial: Extract allowed_dead_ratio from ContiguousSpace
9ac3b77: 8329775: Serial: Remove unused declarations in serialFullGC.hpp
7475824: 8329510: Update ProblemList for JFileChooser/8194044/FileSystemRootTest.java
6439375: 8329533: TestCDSVMCrash fails on libgraal
... and 172 more: https://git.openjdk.org/jdk/compare/bc73963974a824d77d54b8b0edbf8f05262b721c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-03-24T10:00:40Z

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

hotspot

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.

jatin-bhateja · 2024-03-24T10:01:06Z

/label add hotspot-compiler-dev

openjdk · 2024-03-24T10:01:44Z

@jatin-bhateja
The hotspot-compiler label was successfully added.

mlbridge · 2024-03-24T10:04:48Z

Webrevs

01: Full - Incremental (9154491a)
00: Full (05ccc786)

vnkozlov · 2024-03-26T16:40:31Z

src/hotspot/cpu/x86/x86.ad

+      if ((size_in_bits != 512) && !VM_Version::supports_avx512vl()) {
+        return false;


Please add comment to clarify condition. I am reading it as ClearArray will not be supported for NOT avx512 because we can have vector length 512 bits for not avx512.

This is only pertinent to known sized clear arrays which are optimized for AVX-512 targets, we already have such a check as part of matcher predicate, so removing it.

vnkozlov

This looks good. You need second review.

iwanowww

Looks good.

jatin-bhateja · 2024-04-09T01:37:04Z

/integrate

openjdk · 2024-04-09T01:37:39Z

Going to push as commit fbc1e66.
Since your change was applied there have been 183 commits pushed to the master branch:

a887fd2: 8316991: Reduce nullable allocation merges
e702646: 8200566: DistributionPointFetcher fails to fetch CRLs if the DistributionPoints field contains more than one DistributionPoint and the first one fails
7e5ef79: 8323116: [REDO] Computational test more than 2x slower when AVX instructions are used
9467720: 8329875: Serial: Move preservedMarks.inline.hpp to serialFullGC.cpp
a4dd2e9: 8329766: Serial: Refactor SerialBlockOffsetTable API
212a253: 8329623: NegativeArraySizeException encoding large String to UTF-8
dd930c5: 8329787: Fix typo in CLDRConverter
115f419: 8329659: Serial: Extract allowed_dead_ratio from ContiguousSpace
9ac3b77: 8329775: Serial: Remove unused declarations in serialFullGC.hpp
7475824: 8329510: Update ProblemList for JFileChooser/8194044/FileSystemRootTest.java
... and 173 more: https://git.openjdk.org/jdk/compare/bc73963974a824d77d54b8b0edbf8f05262b721c...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-09T01:37:46Z

@jatin-bhateja Pushed as commit fbc1e66.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

TobiHartmann · 2024-05-21T05:23:06Z

This introduced a performance regression, see JDK-8332487. @jatin-bhateja, could you please have a look? Thanks.

eme64 · 2024-05-28T07:30:54Z

src/hotspot/cpu/x86/x86.ad

@@ -1750,7 +1750,6 @@ bool Matcher::match_rule_supported_vector(int opcode, int vlen, BasicType bt) {
        return false;
      }
      break;
-    case Op_ClearArray:


This seems problematic, and may lead to the regression in https://bugs.openjdk.org/browse/JDK-8332487

On non-AVX512 platforms, this is now always true instead of always false. Probably this was not intended, and you thought this way going to be default false?

I don't understand what you are implying. Are you saying this is not the reason for the regression?

Yes, this can cause regression since now on non-AVX512 targets compiler may not emit StoreL based instruction sequence and select one of the clear array pattern based on target feature checks, but on the hind side may bloat the JIT code size for know size clear array operations if InitArrayShortSize is set to a larger value.

jatin-bhateja added 3 commits March 24, 2024 14:10

8328181: C2: assert(MaxVectorSize >= 32) failed: vector length should…

0a26887

… be >= 32

Some comments modifications

7eb1aad

Adding Testpoint

05ccc78

openjdk bot added the rfr Pull request is ready for review label Mar 24, 2024

openjdk bot added the hotspot hotspot-dev@openjdk.org label Mar 24, 2024

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Mar 24, 2024

vnkozlov reviewed Mar 26, 2024

View reviewed changes

Cleanup predicates.

9154491

vnkozlov approved these changes Apr 8, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 8, 2024

iwanowww approved these changes Apr 8, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 9, 2024

openjdk bot closed this Apr 9, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 9, 2024

eme64 reviewed May 28, 2024

View reviewed changes

jatin-bhateja deleted the JDK-8328181 branch August 1, 2024 08:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8328181: C2: assert(MaxVectorSize >= 32) failed: vector length should be >= 32 #18464

8328181: C2: assert(MaxVectorSize >= 32) failed: vector length should be >= 32 #18464

jatin-bhateja commented Mar 24, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 24, 2024

openjdk bot commented Mar 24, 2024 •

edited

Loading

openjdk bot commented Mar 24, 2024

jatin-bhateja commented Mar 24, 2024

openjdk bot commented Mar 24, 2024

mlbridge bot commented Mar 24, 2024 •

edited

Loading

vnkozlov Mar 26, 2024

jatin-bhateja Apr 8, 2024

vnkozlov left a comment

iwanowww left a comment

jatin-bhateja commented Apr 9, 2024

openjdk bot commented Apr 9, 2024

openjdk bot commented Apr 9, 2024

TobiHartmann commented May 21, 2024

eme64 May 28, 2024

eme64 May 28, 2024

jatin-bhateja May 28, 2024 •

edited

Loading

		if ((size_in_bits != 512) && !VM_Version::supports_avx512vl()) {
		return false;

8328181: C2: assert(MaxVectorSize >= 32) failed: vector length should be >= 32 #18464

8328181: C2: assert(MaxVectorSize >= 32) failed: vector length should be >= 32 #18464

Conversation

jatin-bhateja commented Mar 24, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Mar 24, 2024

openjdk bot commented Mar 24, 2024 • edited Loading

openjdk bot commented Mar 24, 2024

jatin-bhateja commented Mar 24, 2024

openjdk bot commented Mar 24, 2024

mlbridge bot commented Mar 24, 2024 • edited Loading

Webrevs

vnkozlov Mar 26, 2024

Choose a reason for hiding this comment

jatin-bhateja Apr 8, 2024

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

iwanowww left a comment

Choose a reason for hiding this comment

jatin-bhateja commented Apr 9, 2024

openjdk bot commented Apr 9, 2024

openjdk bot commented Apr 9, 2024

TobiHartmann commented May 21, 2024

eme64 May 28, 2024

Choose a reason for hiding this comment

eme64 May 28, 2024

Choose a reason for hiding this comment

jatin-bhateja May 28, 2024 • edited Loading

Choose a reason for hiding this comment

jatin-bhateja commented Mar 24, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Mar 24, 2024 •

edited

Loading

mlbridge bot commented Mar 24, 2024 •

edited

Loading

jatin-bhateja May 28, 2024 •

edited

Loading