Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from

Conversation

chhagedorn
Copy link
Member

@chhagedorn chhagedorn commented Jun 3, 2024

A signed test is wrongly split off an unsigned test during Partial Peeling which results in not entering a loop even though it should.

Idea of Partial Peeling

Partial Peeling rotates a loop with the hope to being able to convert the loop into a counted loop later. It is therefore preferable to use signed tests over unsigned tests because counted loops must use a signed loop exit test (i.e. with a BaseCountedLoopEnd).

Partial Peeling with Unsigned Test

However, if there is only a suitable unsigned test, we can still try to use it for Partial Peeling. The idea is to then split off a signed version of the unsigned test and place it right before the unsigned test which can then be used as a loop exit test and later as BaseCountedLoopEnd:

// Loop:
// <peeled section>
// Signed Loop Exit Condition i < 0 or i >= limit
// <-- CUT HERE -->
// Unsigned Loop Exit Condition i >=u limit
// <rest of unpeeled section>
// goto Loop

Requirements for Using an Unsigned Test

The Signed and Unsigned Loop Exit Test do not need have the same result at runtime. It is sufficient if the Signed Loop Exit Test implies the Unsigned Loop Exit Test:

  • If the Signed Loop Exit Test is false, then the (original) Unsigned Loop Exit Test will make sure to exit the loop if required.
  • If the Signed Loop Exit Test is true, then the (original) Unsigned Loop Exit Test must also be true. Otherwise, we would exit a loop that we should have continued to execute.

The Requirements Are Currently Broken

This strong requirement for splitting off a signed test is currently broken as seen in the test cases (for example, testWhileLTIncr()): We split off the signed loop exit test i >= limit, then partial peel and we get the signed loop exit test i >= limit as entry guard to the loop which is wrong:

// After Partial Peeling, we have:
// if (i >= limit) goto Exit
// Loop:
// if (i >=u limit) goto Exit
// ...
// i++;
// if (i >= limit) goto Exit
// goto Loop
// Exit:
// ...
//
// If init = MAX_VALUE and limit = MIN_VALUE:
// i >= limit
// MAX_VALUE >= MIN_VALUE
// which is true where
// i >=u limit
// MAX_VALUE >=u MIN_VALUE
// MAX_VALUE >=u (uint)(MAX_INT + 1)
// is false and we wrongly never enter the loop even though we should have.
// This results in a wrong execution.

Why Are the Requirements Broken?

The reason is that

i >=u limit

can only be converted into the two signed comparisons

i < 0 || i >= limit

if limit is non-negative (i.e. limit >= 0):

// Note that this does not hold for limit < 0:
// Example with i = -3 and limit = -2:
// i < 0
// -2 < 0
// is true and thus also "i < 0 || i >= limit". But
// i >=u limit
// -3 >=u -2
// is false.

This is currently missing and we wrongly use i < 0 or i >= limit as split off signed test.

Fixing the Broken Requirements

To fix this, I've added a bailout when limit could be negative which is the same as checking if the type of limit contains a negative value:

Node* limit = cmpu->in(2);
const TypeInt* type_limit = _igvn.type(limit)->is_int();
if (type_limit->_lo < 0) {
return nullptr;
}

Thanks,
Christian


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-8332920: C2: Partial Peeling is wrongly applied for CmpU with negative limit (Bug - P2)(⚠️ The fixVersion in this issue is [23] but the fixVersion in .jcheck/conf is 24, a new backport will be created when this pr is integrated.)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19522

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 3, 2024

👋 Welcome back chagedorn! 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
Copy link

openjdk bot commented Jun 3, 2024

@chhagedorn 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: kvn, thartmann, epeter

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

  • 2a37764: 8333743: Change .jcheck/conf branches property to match valid branches
  • 75dc2f8: 8330182: Start of release updates for JDK 24
  • 054362a: 8332550: [macos] Voice Over: java.awt.IllegalComponentStateException: component must be showing on the screen to determine its location
  • 9b436d0: 8333674: Disable CollectorPolicy.young_min_ergo_vm for PPC64
  • 487c477: 8333647: C2 SuperWord: some additional PopulateIndex tests
  • d02cb74: 8333270: HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests fail with "Unexpected reference" if timeoutFactor is less than 1/3
  • 02f2404: 8333560: -Xlint:restricted does not work with --release
  • 606df44: 8332670: C1 clone intrinsic needs memory barriers
  • 33fd6ae: 8333622: ubsan: relocInfo_x86.cpp:101:56: runtime error: pointer index expression with base (-1) overflowed
  • 8de5d20: 8332865: ubsan: os::attempt_reserve_memory_between reports overflow
  • ... and 71 more: https://git.openjdk.org/jdk/compare/91101f0d4fc8e06d0d74e06361db6ac87efeeb8e...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
Copy link

openjdk bot commented Jun 3, 2024

@chhagedorn 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.

@openjdk openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jun 3, 2024
@chhagedorn chhagedorn marked this pull request as ready for review June 3, 2024 13:03
@openjdk openjdk bot added the rfr Pull request is ready for review label Jun 3, 2024
@mlbridge
Copy link

mlbridge bot commented Jun 3, 2024

Webrevs

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jun 3, 2024
@chhagedorn
Copy link
Member Author

Thanks Vladimir for your review!

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the Signed Loop Exit Test is false, then the (original) Unsigned Loop Exit Test will make sure to exit the loop if required.

Is that comment incorrect then?

// if and it's projections. The original if test is replaced with
// a constant to force the stay-in-loop path.

Copy link
Contributor

@eme64 eme64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More to come later

// If
// limit >= 0 (COND)
// then the unsigned loop exit condition is equivalent to the signed loop exit condition
// i < 0 || i >= limit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we come up with an alternative equation if limit < 0? And maybe even a combined condition? Not sure if that is helpful, but I'd like to think about it. Could also be a follow-up RFE.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not thought about it, yet, but I also have a feeling that we might can do better. Since this is a P2 bug fix, I propose to do that separately in an RFE as you suggested.

// to be true (otherwise, we wrongly exit a loop that should not have been exited). More formally, we need to ensure:
// "Signed Loop Exit Test" implies "Unsigned Loop Exit Test"
// This is trivially given:
// - Stride < 0:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// - Stride < 0:
// - stride < 0:

Stylistic decision, leave this to your

return nullptr;
}

// For stride < 0, we split off the signed loop exit condition
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// For stride < 0, we split off the signed loop exit condition
// For stride < 0, we insert off the signed loop exit condition

Why do you say "split", we are inserting this extra check, right? Or is the idea that the "rotation" puts this new condition as the last check, hence the exit check in the loop body? Being a big more explicit could help here.

// Unsigned Loop Exit Condition i >=u limit
// <rest of unpeeled section>
// <peeled section>
// Signed Loop Exit Condition i < 0 or i >= limit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is not litterally an OR here, right? It's a bit confusing.

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great otherwise. Nice test and proof!

//
// After Partial Peeling, we have the following structure:
// <cloned peeled section>
// Signed Loop Exit Condition i < 0 or i >= limit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Signed Loop Exit Condition i < 0 or i >= limit
// Signed Loop Exit Condition i < 0 (or i >= limit)

// Unsigned Loop Exit Condition i >=u limit
// <rest of unpeeled section>
// <peeled section>
// Signed Loop Exit Condition i < 0 or i >= limit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Signed Loop Exit Condition i < 0 or i >= limit
// Signed Loop Exit Condition i < 0 (or i >= limit)

//
// Loop:
// <peeled section>
// Signed Loop Exit Condition i < 0 or i >= limit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Signed Loop Exit Condition i < 0 or i >= limit
// Signed Loop Exit Condition i < 0 (or i >= limit)

// then the unsigned loop exit condition is equivalent to the signed loop exit condition
// i < 0 || i >= limit
//
// Note that this does not hold for limit < 0:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed, rephrase this as counterexample.

// - Stride < 0:
// i < 0 // Signed Loop Exit Condition
// i >u MAX_INT // all negative values are greater than MAX_INT when converted to unsigned
// i >=u limit // limit <= MAX_INT (trivially) and since limit >= 0 assumption (COND)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed, an intermediate step in the proof would be good here.

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! That looks good to me.

@chhagedorn
Copy link
Member Author

chhagedorn commented Jun 5, 2024

Thanks Tobias for your review!

Comment on lines 3055 to 3057
// dummy-if |
// / | |
// other | |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This dummy-if is created with insert_region_before_proj() such that we can have a region between the exit projection (exit-proj) of the original unsigned loop exit test and the If node while keeping both the If and the exit-proj. Also see:

//------------------------------ insert_region_before_proj -------------------------------------
// Insert a region before an if projection (* - new node)
//
// before
// if(test)
// / |
// v |
// proj v
// other-proj
//
// after
// if(test)
// / |
// v |
// * proj-clone v
// | other-proj
// v
// * new-region
// |
// v
// * dum_if
// / \
// v \
// * dum-proj v
// proj
//
RegionNode* PhaseIdealLoop::insert_region_before_proj(ProjNode* proj) {

testWhileLTDecr(MIN_VALUE + 2000, -2000);
check(MIN_VALUE + 2001); // MAX_VALUE + 2002 iterations
testWhileLTDecr(MIN_VALUE + 2000, MIN_VALUE + 2001);
check(MIN_VALUE + 2001); // MAX_VALUE + 2002 iterations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add a randomized input test here, that plays close to the boundaries? Just to make sure we would catch things like some off-by one errors.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I've added some random test and computed the number of iteration that I expect. I only did it for the interesting cases.

chhagedorn and others added 3 commits June 5, 2024 15:44
Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>
Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>
Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update is good.

Copy link
Contributor

@eme64 eme64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work with the proofs, good work @chhagedorn !

// v v v |
// exit-region |
// | |
// dummy-if |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, I don't see this dummy-if mentioned in any of the comments. Can you add a comment line about what this is, and where it comes from?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, added a note.

@chhagedorn
Copy link
Member Author

chhagedorn commented Jun 6, 2024

Thanks Vladimir and Emanuel for your reviews!

I've submitted some sanity performance testing since we could now be bailing out more. I'm not sure though how much else we could do if there are some performance regressions.

@chhagedorn
Copy link
Member Author

Performance testing looked good. Thanks again for your careful reviews!

/integrate

@openjdk
Copy link

openjdk bot commented Jun 11, 2024

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

  • 2843745: 8333972: Parallel: Remove unused methods in PSOldGen
  • 93f3918: 8333954: Parallel: Remove unused arguments of type ParCompactionManager*
  • 788b876: 8333917: G1: Refactor G1CollectedHeap::register_old_region_with_region_attr
  • 0e4d4a0: 8320725: AArch64: C2: Add "requires_strict_order" flag for floating-point add and mul reduction
  • badf1cb: 8331675: gtest CollectorPolicy.young_min_ergo_vm fails after 8272364
  • 4d6064a: 8333649: Allow different NativeCall encodings
  • fe9c63c: 8333931: Problemlist serviceability/jvmti/vthread/CarrierThreadEventNotification
  • 41c88bc: 8333756: java/lang/instrument/NativeMethodPrefixApp.java failed due to missing intrinsic
  • 3a01b47: 8330205: Initial troff manpage generation for JDK 24
  • 9691153: 8329141: Obsolete RTM flags and code
  • ... and 121 more: https://git.openjdk.org/jdk/compare/91101f0d4fc8e06d0d74e06361db6ac87efeeb8e...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jun 11, 2024
@openjdk openjdk bot closed this Jun 11, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 11, 2024
@openjdk
Copy link

openjdk bot commented Jun 11, 2024

@chhagedorn Pushed as commit ef101f1.

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

@chhagedorn
Copy link
Member Author

/backport jdk:jdk23

@openjdk
Copy link

openjdk bot commented Jun 11, 2024

@chhagedorn the backport was successfully created on the branch backport-chhagedorn-ef101f1b-jdk23 in my personal fork of openjdk/jdk. To create a pull request with this backport targeting openjdk/jdk:jdk23, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit ef101f1b from the openjdk/jdk repository.

The commit being backported was authored by Christian Hagedorn on 11 Jun 2024 and was reviewed by Vladimir Kozlov, Tobias Hartmann and Emanuel Peter.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk:

$ git fetch https://github.com/openjdk-bots/jdk.git backport-chhagedorn-ef101f1b-jdk23:backport-chhagedorn-ef101f1b-jdk23
$ git checkout backport-chhagedorn-ef101f1b-jdk23
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk.git backport-chhagedorn-ef101f1b-jdk23

⚠️ @chhagedorn You are not yet a collaborator in my fork openjdk-bots/jdk. An invite will be sent out and you need to accept it before you can proceed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

4 participants