8300256: C2: vectorization is sometimes skipped on loops where it would succeed #12116

rwestrel · 2023-01-20T15:15:40Z

Vectorization for a counted loop cl only proceeds if
cl->range_checks_present() returns true. The result of that method is
computed lazily and its result cached in the CountedLoopNode and never
re-computed. If PhaseIdealLoop::do_range_check() returns 0 then the
result of that computation is overwritten (no range checks
present). PhaseIdealLoop::do_range_check() counts the number of tests
present in the loop body (which is really what range_checks_present()
is about) and decrements that count for every check it eliminates
except if it's not a comparison with a LoadRange (for a reason that I
don't understand). In the case of the test (a pattern from a
ByteBuffer benchmark), not all tests are with a LoadRange. As a
result, PhaseIdealLoop::do_range_check() returns non zero even though
it eliminates all tests. As a result, vectorization is never
attempted.

There doesn't seem to be a value in caching the result of
range_checks_present() in CountedLoopNode. It's not that expensive to
compute, it's only used during loop opts and it's really hard to keep
in sync with whether the loop has still tests: several different
transformations could remove a test. What I propose instead is to keep
roughly the same approach (compute the result lazily and cache it so
it doesn't have to be re-computed) but to store it on the
IdealLoopTree instead (so it's recomputed on every loop opts pass and
there's no risk that it becomes out of sync).

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-8300256: C2: vectorization is sometimes skipped on loops where it would succeed

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to 2140b251
Tobias Hartmann (@TobiHartmann - Reviewer) ⚠️ Review applies to c48ccd47

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12116

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

Using diff file

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

bridgekeeper · 2023-01-20T15:17:54Z

👋 Welcome back roland! 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 · 2023-01-20T15:21:23Z

@rwestrel 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 · 2023-01-20T15:24:56Z

Webrevs

vnkozlov

Just one comment.

vnkozlov · 2023-01-23T17:37:19Z

src/hotspot/share/opto/loopnode.cpp

@@ -3990,7 +3990,6 @@ void IdealLoopTree::dump_head() const {
    if (cl->is_post_loop()) tty->print(" post");
    if (cl->is_reduction_loop()) tty->print(" reduction");
    if (cl->is_vectorized_loop()) tty->print(" vector");
-    if (cl->range_checks_present()) tty->print(" rc ");


Can you use new range_checks_present() here? The information is still useful.

Thanks for reviewing this.
Indeed, you're right. I updated the change.

vnkozlov

Good.

openjdk · 2023-01-25T17:39:21Z

@rwestrel 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:

8300256: C2: vectorization is sometimes skipped on loops where it would succeed

Reviewed-by: kvn, thartmann

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 32 new commits pushed to the master branch:

a74ebd0: 8299325: java/net/httpclient/CancelRequestTest.java fails "test CancelRequestTest.testGetSendAsync("https://localhost:46509/https1/x/same/interrupt", true, true)"
041a12e: 8301255: Http2Connection may send too many GOAWAY frames
476f58a: 8298424: Remove redundant FOUND_CORES variable in build-performance.m4
ebb84ad: 8301036: RISC-V: Factor out functions baseOffset & baseOffset32 from MacroAssembler
61a5f11: 8301033: RISC-V: Handle special cases for MinI/MaxI nodes for Zbb
82df4a2: 8301148: Serial: Remove ContiguousSpace::reset_saved_mark
c672ed1: 8301248: Less side effects in InstanceRefKlass::trace_reference_gc
cee4bd3: 8301047: Clean up type unsafe uses of oop from compiler code
7fae3a1: 8301229: Clean up SuspendibleThreadSet::_suspend_all
f50cda7: 8301217: Remove FilteringClosure
... and 22 more: https://git.openjdk.org/jdk/compare/f52d35c84b7333809156d201c866793854143888...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.

TobiHartmann

Looks good to me. Maybe add a @bug 8300256 to the test.

rwestrel · 2023-01-27T16:13:26Z

Looks good to me. Maybe add a @bug 8300256 to the test.

Thanks for the review. Yes, I will do that.

rwestrel · 2023-01-30T14:49:04Z

FTR, the test fails on x86 (32 bits) for some reason. I changed the test so it doesn't run on x86 (32 bits).

rwestrel · 2023-02-01T09:47:50Z

/integrate

openjdk · 2023-02-01T09:49:06Z

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

ef0d0a7: 8301402: os::print_location gets is_global_handle assert
4f6f3cc: 8301446: Remove unused includes of gc/shared/genOopClosures
a0aed9b: 8301459: Serial: Merge KeepAliveClosure into FastKeepAliveClosure
d269ebb: 8301570: Test runtime/jni/nativeStack/ needs to detach the native thread
8164cfb: 8300696: [AIX] AttachReturnError fails
09bfbf8: 8300909: Update com/sun/jndi/dns/Test6991580.java manual test instruction
6beadbb: 8293519: deprecation warnings should be emitted for uses of annotation methods inside other annotations
4bef233: 8301549: Fix comment about ClassCircularityError
e1bf471: 8301555: Remove constantPoolCacheKlass friend
5744c91: 8297158: Suspicious collection method call in Types.isSameTypeVisitor
... and 57 more: https://git.openjdk.org/jdk/compare/f52d35c84b7333809156d201c866793854143888...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-02-01T09:49:17Z

@rwestrel Pushed as commit 2a8ae2f.

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

rwestrel added 4 commits January 20, 2023 15:57

vectorization not run

2ae48a1

more

5256f2b

maybe more

6c7df37

more

929306d

openjdk bot added the rfr Pull request is ready for review label Jan 20, 2023

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jan 20, 2023

vnkozlov reviewed Jan 23, 2023

View reviewed changes

rwestrel added 2 commits January 25, 2023 16:37

Merge branch 'master' into JDK-8300256

004bc04

review

2140b25

vnkozlov approved these changes Jan 25, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 25, 2023

TobiHartmann approved these changes Jan 26, 2023

View reviewed changes

Merge branch 'master' into JDK-8300256

be2cb5b

@bug in test

c48ccd4

TobiHartmann approved these changes Jan 30, 2023

View reviewed changes

skip test on 32 bits

6584c9d

openjdk bot added the integrated Pull request has been integrated label Feb 1, 2023

openjdk bot closed this Feb 1, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8300256: C2: vectorization is sometimes skipped on loops where it would succeed #12116

8300256: C2: vectorization is sometimes skipped on loops where it would succeed #12116

rwestrel commented Jan 20, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 20, 2023

openjdk bot commented Jan 20, 2023

mlbridge bot commented Jan 20, 2023 •

edited

Loading

vnkozlov left a comment

vnkozlov Jan 23, 2023

rwestrel Jan 25, 2023

vnkozlov left a comment

openjdk bot commented Jan 25, 2023 •

edited

Loading

TobiHartmann left a comment

rwestrel commented Jan 27, 2023

rwestrel commented Jan 30, 2023

rwestrel commented Feb 1, 2023

openjdk bot commented Feb 1, 2023

openjdk bot commented Feb 1, 2023

8300256: C2: vectorization is sometimes skipped on loops where it would succeed #12116

8300256: C2: vectorization is sometimes skipped on loops where it would succeed #12116

Conversation

rwestrel commented Jan 20, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 20, 2023

openjdk bot commented Jan 20, 2023

mlbridge bot commented Jan 20, 2023 • edited Loading

Webrevs

vnkozlov left a comment

Choose a reason for hiding this comment

vnkozlov Jan 23, 2023

Choose a reason for hiding this comment

rwestrel Jan 25, 2023

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

openjdk bot commented Jan 25, 2023 • edited Loading

TobiHartmann left a comment

Choose a reason for hiding this comment

rwestrel commented Jan 27, 2023

rwestrel commented Jan 30, 2023

rwestrel commented Feb 1, 2023

openjdk bot commented Feb 1, 2023

openjdk bot commented Feb 1, 2023

rwestrel commented Jan 20, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 20, 2023 •

edited

Loading

openjdk bot commented Jan 25, 2023 •

edited

Loading