8382054: TestLoopPeelingDisabled.java fails with -DVerifyIR=false#30713
Open
raneashay wants to merge 3 commits intoopenjdk:masterfrom
Open
8382054: TestLoopPeelingDisabled.java fails with -DVerifyIR=false#30713raneashay wants to merge 3 commits intoopenjdk:masterfrom
raneashay wants to merge 3 commits intoopenjdk:masterfrom
Conversation
TestLoopPeelingDisabled.java contains a test that expects an IR violation exception when loop peeling is disabled. Specifically, the test has an `@IR` annotation that checks for the presence of loop peeling phases and we expect this IR check to fail when loop peeling is disabled. But when IR checks are disabled using `-DVerifyIR=false`, the framework skips checking the annotation, resulting in no IR violation exception, causing the test to fail. This patch adds a guard so that only when `VerifyIR` is set that we look for the IR violation exception. Credit to Tobias Hartmann for discovering and reporing the issue.
|
👋 Welcome back raneashay! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@raneashay The following label will be automatically applied to this pull request:
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. |
Webrevs
|
dafedafe
reviewed
Apr 14, 2026
Contributor
dafedafe
left a comment
dafedafe
left a comment
There was a problem hiding this comment.
LGTM. Thanks @raneashay. Just running some testing...
| // Then, run the same test with loop peeling disabled, which should | ||
| // elide the {BEFORE,AFTER}_LOOP_PEELING compilation phases, causing the | ||
| // test to throw an IRViolationException. We then check whether the | ||
| // test to throw an IRViolationException. We then check whether the |
Contributor
There was a problem hiding this comment.
Suggested change
| // test to throw an IRViolationException. We then check whether the | |
| // test to throw an IRViolationException. We then check whether the |
chhagedorn
reviewed
Apr 14, 2026
| // Then, run the same test with loop peeling disabled, which should | ||
| // elide the {BEFORE,AFTER}_LOOP_PEELING compilation phases, causing the | ||
| // test to throw an IRViolationException. We then check whether the | ||
| // test to throw an IRViolationException. We then check whether the |
Member
There was a problem hiding this comment.
Suggested change
| // test to throw an IRViolationException. We then check whether the | |
| // test to throw an IRViolationException. We then check whether the |
| Asserts.fail("Unexpected IR violation: " + info); | ||
| } | ||
| System.out.println("Loop peeling correctly disabled"); | ||
| // phase was not found). If IR verification is disabled, this test will |
Member
There was a problem hiding this comment.
Suggested change
| // phase was not found). If IR verification is disabled, this test will | |
| // phase was not found). If IR verification is disabled, this test will |
| } | ||
| System.out.println("Loop peeling correctly disabled"); | ||
| // phase was not found). If IR verification is disabled, this test will | ||
| // not throw IRViolationException. |
Member
There was a problem hiding this comment.
Suggested change
| // not throw IRViolationException. | |
| // not throw an IRViolationException. |
Comment on lines
56
to
68
| if (Boolean.parseBoolean(System.getProperty("VerifyIR", "true"))) { | ||
| try { | ||
| TestFramework.runWithFlags("-XX:+UnlockDiagnosticVMOptions", | ||
| "-XX:LoopPeeling=0"); | ||
| Asserts.fail("Expected IRViolationException"); | ||
| } catch (IRViolationException e) { | ||
| String info = e.getExceptionInfo(); | ||
| if (!info.contains("NO compilation output found for this phase")) { | ||
| Asserts.fail("Unexpected IR violation: " + info); | ||
| } | ||
| System.out.println("Loop peeling correctly disabled"); | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
This will only execute the test with LoopPeeling=0 if VerifyIR is true. But we should also execute it if VerifyIR is false. A better fix could be:
Suggested change
| if (Boolean.parseBoolean(System.getProperty("VerifyIR", "true"))) { | |
| try { | |
| TestFramework.runWithFlags("-XX:+UnlockDiagnosticVMOptions", | |
| "-XX:LoopPeeling=0"); | |
| Asserts.fail("Expected IRViolationException"); | |
| } catch (IRViolationException e) { | |
| String info = e.getExceptionInfo(); | |
| if (!info.contains("NO compilation output found for this phase")) { | |
| Asserts.fail("Unexpected IR violation: " + info); | |
| } | |
| System.out.println("Loop peeling correctly disabled"); | |
| } | |
| } | |
| try { | |
| TestFramework.runWithFlags("-XX:+UnlockDiagnosticVMOptions", | |
| "-XX:LoopPeeling=0"); | |
| Asserts.assertFalse(Boolean.parseBoolean(System.getProperty("VerifyIR", "true")), | |
| "Expected IRViolationException when performing IR matching"); | |
| } catch (IRViolationException e) { | |
| String info = e.getExceptionInfo(); | |
| if (!info.contains("NO compilation output found for this phase")) { | |
| Asserts.fail("Unexpected IR violation: " + info); | |
| } | |
| System.out.println("Loop peeling correctly disabled"); | |
| } |
The key change is that instead of not running the test at all when verifyIR is false, we now run the test unconditionally and throw an assertion only if verifyIR is false AND if we didn't catch IRViolationException.
Contributor
Author
|
Thanks @dafedafe and @chhagedorn! I've updated the patch to include your suggestions. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TestLoopPeelingDisabled.java contains a test that expects an IR
violation exception when loop peeling is disabled. Specifically, the
test has an
@IRannotation that checks for the presence of looppeeling phases and we expect this IR check to fail when loop peeling is
disabled. But when IR checks are disabled using
-DVerifyIR=false, theframework skips checking the annotation, resulting in no IR violation
exception, causing the test to fail.
This patch adds a guard so that only when
VerifyIRis set that we lookfor the IR violation exception.
Credit to Tobias Hartmann for discovering and reporing the issue.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30713/head:pull/30713$ git checkout pull/30713Update a local copy of the PR:
$ git checkout pull/30713$ git pull https://git.openjdk.org/jdk.git pull/30713/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 30713View PR using the GUI difftool:
$ git pr show -t 30713Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30713.diff
Using Webrev
Link to Webrev Comment