8332920: C2: Partial Peeling is wrongly applied for CmpU with negative limit #728

TheRealMDoerr · 2024-06-14T15:38:59Z

Originally clean backport of JDK-8332920.
Unfortunately, -XX:CompileOnly is not available in 21u, so I had to adapt the new test compiler/loopopts/TestPartialPeelAtUnsignedTestsNegativeLimit.java (see 2nd and 3rd commit). Tier 1-4 have passed.

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
JDK-8332920 needs maintainer approval

Issue

JDK-8332920: C2: Partial Peeling is wrongly applied for CmpU with negative limit (Bug - P2 - Approved)

Reviewers

Christian Hagedorn (@chhagedorn - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk21u-dev.git pull/728/head:pull/728
$ git checkout pull/728

Update a local copy of the PR:
$ git checkout pull/728
$ git pull https://git.openjdk.org/jdk21u-dev.git pull/728/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 728

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk21u-dev/pull/728.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-06-14T15:39:53Z

👋 Welcome back mdoerr! 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-06-14T15:40:37Z

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

8332920: C2: Partial Peeling is wrongly applied for CmpU with negative limit

Reviewed-by: chagedorn

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

432144b: 8316211: Open source several manual applet tests
a045d78: 8283223: gc/stringdedup/TestStringDeduplicationFullGC.java#Parallel failed with "RuntimeException: String verification failed"
c77926c: 7022325: TEST_BUG: test/java/util/zip/ZipFile/ReadLongZipFileName.java leaks files if it fails
62c30e6: 8333398: Uncomment the commented test in test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java
d2534fa: 8328110: Allow simultaneous use of PassFailJFrame with split UI and additional windows
420c4b5: 8316389: Open source few AWT applet tests
8094990: 8313674: (fc) java/nio/channels/FileChannel/BlockDeviceSize.java should test for more block devices
2ed8a96: 8315576: compiler/codecache/CodeCacheFullCountTest.java fails after JDK-8314837
edbe6f9: 8314614: jdk/jshell/ImportTest.java failed with "InternalError: Failed remote listen"
d927a6f: 8325384: sun/security/ssl/SSLSessionImpl/ResumptionUpdateBoundValues.java failing intermittently when main thread is a virtual thread
... and 41 more: https://git.openjdk.org/jdk21u-dev/compare/df030542eb11bb3482e2a8c7c8a477a333a409c8...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-06-14T15:40:58Z

This backport pull request has now been updated with issue from the original commit.

openjdk · 2024-06-14T15:41:50Z

⚠️ @TheRealMDoerr This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

mlbridge · 2024-06-14T15:44:35Z

Webrevs

TheRealMDoerr · 2024-06-20T13:18:02Z

@chhagedorn: I'm getting timeouts when trying the new test in JDK 21u:

compiler/loopopts/TestPartialPeelAtUnsignedTestsNegativeLimit.java#Xbatch              Error. Program `/jdk21_opt/images/jdk/bin/java' timed out (timeout set to 480000ms, elapsed time including timeout handling was 480262ms).
compiler/loopopts/TestPartialPeelAtUnsignedTestsNegativeLimit.java#Xcomp-compile-test  Error. Program `/jdk21_opt/images/jdk/bin/java' timed out (timeout set to 480000ms, elapsed time including timeout handling was 480283ms).
compiler/loopopts/TestPartialPeelAtUnsignedTestsNegativeLimit.java#Xcomp-run-inline    Error. Program `/jdk21_opt/images/jdk/bin/java' timed out (timeout set to 480000ms, elapsed time including timeout handling was 480267ms).
compiler/loopopts/TestPartialPeelAtUnsignedTestsNegativeLimit.java#vanilla             Passed. Execution successful

Do you know if there's a bug or a performance problem? The related issues are already integrated.

chhagedorn · 2024-06-20T13:48:17Z

@chhagedorn: I'm getting timeouts when trying the new test in JDK 21u:

compiler/loopopts/TestPartialPeelAtUnsignedTestsNegativeLimit.java#Xbatch              Error. Program `/jdk21_opt/images/jdk/bin/java' timed out (timeout set to 480000ms, elapsed time including timeout handling was 480262ms).
compiler/loopopts/TestPartialPeelAtUnsignedTestsNegativeLimit.java#Xcomp-compile-test  Error. Program `/jdk21_opt/images/jdk/bin/java' timed out (timeout set to 480000ms, elapsed time including timeout handling was 480283ms).
compiler/loopopts/TestPartialPeelAtUnsignedTestsNegativeLimit.java#Xcomp-run-inline    Error. Program `/jdk21_opt/images/jdk/bin/java' timed out (timeout set to 480000ms, elapsed time including timeout handling was 480267ms).
compiler/loopopts/TestPartialPeelAtUnsignedTestsNegativeLimit.java#vanilla             Passed. Execution successful

Do you know if there's a bug or a performance problem? The related issues are already integrated.

The tests use the new CompileOnly format which only went into JDK 22 with JDK-8027711. It looks like with JDK 21u, the CompileOnly command is just ignored and none of these test methods get compiled and the tests become really slow when only executing them with the interpreter (we have some iteration-heavy loops in these tests).

I suggest to simply change CompileOnly to separate CompileCommand=compileonly statements.

TheRealMDoerr · 2024-06-20T13:59:54Z

Thanks! That has helped.

chhagedorn · 2024-06-20T14:11:29Z

test/hotspot/jtreg/compiler/loopopts/TestPartialPeelAtUnsignedTestsNegativeLimit.java

+ * @bug 8332920
+ * @summary Tests partial peeling at unsigned tests with limit being negative in exit tests "i >u limit".
+ * @run main/othervm -Xbatch -XX:-TieredCompilation
+ *                   -XX:CompileCommand=compileonly,*TestPartialPeel*::original*,*TestPartialPeel*::test*


You need separate CompileCommand=compileonly statements since they only supports one pattern. The idea of JDK-8027711 was to keep the benefit of defining multiple patterns with CompileOnly but also getting the same syntax as used by CompileCommand=compileonly. This was the reason I used CompileOnly here in mainline and not a compile command.

Thank you! I was not aware of that. Fixed.

You're welcome :-) looks good now!

TheRealMDoerr · 2024-06-21T09:44:39Z

Thanks a lot for your support and your review!

TheRealMDoerr · 2024-06-22T19:52:42Z

/approval request Should get backported for parity with 21.0.5-oracle. Applies cleanly, but the test needs an adaptation which has been reviewed by the original author. Tier 1-4 have passed.

openjdk · 2024-06-22T19:53:32Z

@TheRealMDoerr
8332920: The approval request has been created successfully.

TheRealMDoerr · 2024-06-24T08:58:09Z

/integrate

openjdk · 2024-06-24T09:00:10Z

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

ca23974: 8320655: awt screencast robot spin and sync issues with native libpipewire api
e99ad49: 8331011: [XWayland] TokenStorage fails under Security Manager
46fcb7e: 8334333: MissingResourceCauseTestRun.java fails if run by root
11628ef: 8319197: Exclude hb-subset and hb-style from compilation
3222647: 8307778: com/sun/jdi/cds tests fail with jtreg's Virtual test thread factory
b0607ee: 8299487: Test java/net/httpclient/whitebox/SSLTubeTestDriver.java timed out
717e389: 8316306: Open source and convert manual Swing test
241b054: 8325754: Dead AbstractQueuedSynchronizer$ConditionNodes survive minor garbage collections
57824ed: 8321220: JFR: RecordedClass reports incorrect modifiers
1c19bbd: 8317360: Missing null checks in JfrCheckpointManager and JfrStringPool initialization routines
... and 51 more: https://git.openjdk.org/jdk21u-dev/compare/df030542eb11bb3482e2a8c7c8a477a333a409c8...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-06-24T09:00:16Z

@TheRealMDoerr Pushed as commit 7328df4.

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

Backport ef101f1bf20f2813f855af4bc4eb317565175208

928a2a5

openjdk bot changed the title ~~Backport ef101f1bf20f2813f855af4bc4eb317565175208~~ 8332920: C2: Partial Peeling is wrongly applied for CmpU with negative limit Jun 14, 2024

openjdk bot added backport clean labels Jun 14, 2024

openjdk bot added the rfr Pull request is ready for review label Jun 14, 2024

Replace -XX:CompileOnly

3d13b5a

openjdk bot removed the clean label Jun 20, 2024

chhagedorn reviewed Jun 20, 2024

View reviewed changes

Use separate CompileCommands.

7a33ed3

chhagedorn approved these changes Jun 21, 2024

View reviewed changes

openjdk bot added approval ready Pull request is ready to be integrated and removed approval labels Jun 22, 2024

openjdk bot added the integrated Pull request has been integrated label Jun 24, 2024

openjdk bot closed this Jun 24, 2024

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

8332920: C2: Partial Peeling is wrongly applied for CmpU with negative limit #728

8332920: C2: Partial Peeling is wrongly applied for CmpU with negative limit #728

Uh oh!

Conversation

TheRealMDoerr commented Jun 14, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Jun 14, 2024

Uh oh!

openjdk bot commented Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jun 14, 2024

Uh oh!

openjdk bot commented Jun 14, 2024

Uh oh!

mlbridge bot commented Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

TheRealMDoerr commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chhagedorn commented Jun 20, 2024

Uh oh!

TheRealMDoerr commented Jun 20, 2024

Uh oh!

chhagedorn Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheRealMDoerr Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

chhagedorn Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

TheRealMDoerr commented Jun 21, 2024

Uh oh!

TheRealMDoerr commented Jun 22, 2024

Uh oh!

openjdk bot commented Jun 22, 2024

Uh oh!

TheRealMDoerr commented Jun 24, 2024

Uh oh!

openjdk bot commented Jun 24, 2024

Uh oh!

openjdk bot commented Jun 24, 2024

Uh oh!

Uh oh!

TheRealMDoerr commented Jun 14, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jun 14, 2024 •

edited

Loading

mlbridge bot commented Jun 14, 2024 •

edited

Loading

TheRealMDoerr commented Jun 20, 2024 •

edited

Loading

chhagedorn Jun 20, 2024 •

edited

Loading