Skip to content
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

8307683: Loop Predication should not hoist range checks with trap on success projection by negating their condition #14156

Closed
wants to merge 6 commits into from

Conversation

chhagedorn
Copy link
Member

@chhagedorn chhagedorn commented May 25, 2023

JDK-4809552 allowed Loop Predication to be applied to IfNodes that have a positive value instead of a LoadRangeNode:

if (range->Opcode() != Op_LoadRange) {
const TypeInteger* tinteger = phase->_igvn.type(range)->isa_integer(bt);
if (tinteger == nullptr || tinteger->empty() || tinteger->lo_as_long() < 0) {
// Allow predication on positive values that aren't LoadRanges.
// This allows optimization of loops where the length of the
// array is a known value and doesn't need to be loaded back
// from the array.
return false;
}

This, however, is only correct if we have an actual RangeCheckNode for an array. The reason for that is that if we hoist a real range check and create a Hoisted Predicate for it, we only need to check the lower and upper bound of all array accesses (i.e. the array access of the first and the last loop iteration). All array accesses in between are implicitly covered and do not need to be checked again.

But if we face an IfNode without a LoadRangeNode, we could be comparing anything. We do not have any guarantee that if the first and last loop iteration check succeed that the other loop iteration checks will also succeed. An example of this is shown in the test case test(). We wrongly create a Hoisted Range Check Predicate where the lower and upper bound are always true, but for some values of the loop induction variable, the hoisted check would actually fail. We then crash because an added Assertion Predicate exactly performs this failing check (crash with halt). Without any loop splitting (i.e. no Assertion Predicates), we have a wrong execution due to never executing the branch where we increment iFld2 because we removed it together with the check.

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-8307683: Loop Predication should not hoist range checks with trap on success projection by negating their condition

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14156

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented May 25, 2023

👋 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.

@chhagedorn chhagedorn changed the title Loop Predication is wrongly applied to non-RangeCheckNodes without a LoadRangeNode 8307683: Loop Predication is wrongly applied to non-RangeCheckNodes without a LoadRangeNode May 25, 2023
@openjdk
Copy link

openjdk bot commented May 25, 2023

@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 hotspot-compiler hotspot-compiler-dev@openjdk.org rfr Pull request is ready for review labels May 25, 2023
@mlbridge
Copy link

mlbridge bot commented May 25, 2023

Webrevs

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 good to me. Scary, that we didn't find this earlier.

Comment on lines 102 to 103

flag = !flag;
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
flag = !flag;
flag = !flag;

// fails for i = 0 and we halt. When not splitting this loop (with LoopMaxUnroll=0), we have a wrong execution due
// to never executing iFld2++ (we removed the check and the branch with the trap).
for (int i = -1; i < 1000; i++) {
if (Integer.compareUnsigned(i, 100) < 0) { // Loop Predication creates a Hoisted Range Check Predicate due to trap with Float.isNan().
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment explaining that this is equivalent to i <u 100 <=> i >= 0 && i < 100 and we add a predicate for the else branch, i.e. for i < 0 || i >= 100, and remove the if branch.

@openjdk
Copy link

openjdk bot commented May 26, 2023

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

8307683: Loop Predication should not hoist range checks with trap on success projection by negating their condition

Reviewed-by: thartmann, roland

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

  • 96ed139: 8308766: TLAB initialization may cause div by zero
  • 6c7225f: 8303417: RISC-V: Merge vector instructs with similar match rules
  • a46b5ac: 8308503: AArch64: SIGILL when running with -XX:UseBranchProtection=pac-ret on hardware without PAC feature
  • f9ad7df: 8300865: C2: product reduction in ProdRed_Double is not vectorized
  • 8eda97d: 8305320: DbgStrings and AsmRemarks are leaking
  • 0951474: 8309150: Need to escape " inside attribute values
  • 0119969: 8309171: Test vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/TestDescription.java fails after JDK-8308341
  • f8a924a: 8308975: Fix signed integer overflow in compiler code, part 2
  • 5531f6b: 8308819: add JDWP and JDI virtual thread support for ThreadReference.ForceEarlyReturn
  • e42a4b6: 8309236: ProblemList java/util/concurrent/locks/Lock/OOMEInAQS.java with ZGC and Generational ZGC again
  • ... and 78 more: https://git.openjdk.org/jdk/compare/90e57fd5a96199b01e7c058a4d8a82e467d7f41a...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 openjdk bot added the ready Pull request is ready to be integrated label May 26, 2023
Copy link
Contributor

@rwestrel rwestrel 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 to me. Thanks for the quick fix.
Do we want to file a bug to revisit this in a correct way?

// This allows optimization of loops where the length of the
// array is a known value and doesn't need to be loaded back
// from the array.
if (!iff->is_RangeCheck() || tinteger == nullptr || tinteger->empty() || tinteger->lo_as_long() < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the problem not happen with a LoadRange as second input? Couldn't the LoadRange constant fold and then the predicates could constant fold as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right. It's also a problem with a LoadRange, even if it does not constant fold.

@rwestrel
Copy link
Contributor

Do we want to file a bug to investigate whether this can be done in a correct way?

@chhagedorn
Copy link
Member Author

Thanks Tobias and Roland for the initial reviews.

After thinking about the fix again and discussing it with Roland, I updated the fix:

We disallow this pattern for an IfNode:

if (iv <u limit) {
  trap();
}

Because this is flipped to:

if (iv >=u limit) {

} else {
  trap();
}

to have the trap to the false projection. However, this does not match the range check pattern of a RangeCheckNode:

if (iv <u limit) {

} else {
  trap();
}

We should therefore bailout in these cases, regardless of whether limit is a positive constant or a LoadRangeNode. If we still created a Hoisted Range Check Predicate, it could succeed at runtime (i.e. true for the value of iv in the first loop iteration and true for the value of iv in the last loop iteration) while the check to be hoisted could fail in other loop iterations. An example for this is shown in test():

// Hoisted Range Check Predicate is always true:
// iv_first_iteration >=u limit && iv_last_iteration >=u limit  <=>
// -1 >=u 100 && 999 >= u 100
for (int i = -1; i < 1000; i++) {
    if (Integer.compareUnsigned(i, 100) < 0) {
        iFld2++;
        Float.isNaN(34); // Float class is unloaded with -Xcomp -> inserts trap
    } else {
        iFld++;
    }
}

However, if 0 <= i < 100, then the hoisted check Integer.compareUnsigned(i, 100) < 0 would be false. We then wrongly skip the branch with the Float.isNan(34) trap (was removed when creating the Hoisted Range Check Predicate) and miss to execute iFld2 leading to a wrong execution (or when splitting this loop, we halt because of an initialized Assertion Predicate which will fail).

@chhagedorn chhagedorn changed the title 8307683: Loop Predication is wrongly applied to non-RangeCheckNodes without a LoadRangeNode 8307683: Loop Predication wrongly hoists non-RangeCheckNodes as range checks May 26, 2023
@chhagedorn chhagedorn changed the title 8307683: Loop Predication wrongly hoists non-RangeCheckNodes as range checks 8307683: Loop Predication wrongly hoists IfNodes without a range check pattern as range check May 26, 2023
Copy link
Contributor

@rwestrel rwestrel 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 to me.

@jerboaa
Copy link
Contributor

jerboaa commented May 30, 2023

This fails the compiler/loopopts/TestSkeletonPredicateNegation test with:

#
# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (/home/runner/work/jdk/jdk/src/hotspot/share/opto/loopPredicate.cpp:848), pid=32813, tid=32828
#  assert(!iff->is_RangeCheck()) failed: can only be IfNode because RangeCheckNodes always have trap on false projection
#
# JRE version: OpenJDK Runtime Environment (21.0) (fastdebug build 21-internal-chhagedorn-229583b7613867127e42baca158773bcf9c08c73)
# Java VM: OpenJDK 64-Bit Server VM (fastdebug 21-internal-chhagedorn-229583b7613867127e42baca158773bcf9c08c73, mixed mode, tiered, compressed oops, compressed class ptrs, g1 gc, linux-amd64)
# Problematic frame:
# V  [libjvm.so+0x1298438]  IdealLoopTree::is_range_check_if(IfProjNode*, PhaseIdealLoop*, BasicType, Node*, Node*&, Node*&, long&) const+0x278
#
# CreateCoredumpOnCrash turned off, no core file dumped
#
# An error report file with more information is saved as:
# /home/runner/work/jdk/jdk/build/run-test-prebuilt/test-support/jtreg_test_hotspot_jtreg_tier1_compiler/scratch/hs_err_pid32813.log
#
# Compiler replay data is saved as:
# /home/runner/work/jdk/jdk/build/run-test-prebuilt/test-support/jtreg_test_hotspot_jtreg_tier1_compiler/scratch/replay_pid32813.log
#
# If you would like to submit a bug report, please visit:
#   https://bugreport.java.com/bugreport/crash.jsp
#
result: Error. Agent communication error: java.io.EOFException; check console log for any additional details

which seems related?

@chhagedorn
Copy link
Member Author

Thanks @jerboaa for reporting that. I've seen that in my testing over the weekend as well. I'm looking into it.

@chhagedorn
Copy link
Member Author

chhagedorn commented May 30, 2023

In the failing test case (see newly added testRangeCheckNode()), we have a RangeCheckNode with an unloaded trap on the true/success projection because we are always taking the false/exception path. Loop Predication wrongly detects this pattern as valid range check and tries to create a Hoisted Predicate for that RangeCheckNode by flipping the boolean condition. This is wrong due to the same reasons explained above for the IfNode. The failing assertion should have made sure that we will never have a RangeCheckNode there.

I'm therefore suggesting to remove the entire negation of range checks which seems to be wrong. We should bail out in is_range_check_if() if we have the false projection as success projection. I've pushed an update accordingly.

I will run some testing again.

Copy link
Contributor

@rwestrel rwestrel 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 to me.

@chhagedorn
Copy link
Member Author

Thanks Roland for re-reviewing it again and the offline discussion!

@chhagedorn chhagedorn changed the title 8307683: Loop Predication wrongly hoists IfNodes without a range check pattern as range check 8307683: Loop Predication should not hoist range checks with trap on success projection by negating their condition May 30, 2023
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 analysis. Looks good to me. As we discussed offline, let's run some sanity performance testing (we can already integrate this).

chhagedorn and others added 2 commits June 1, 2023 09:57
…NonRangeCheck.java

Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>
Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>
@chhagedorn
Copy link
Member Author

Thanks Tobias for reviewing the new fix again! That's a good idea. I've run some standard benchmarks and could not find any regressions.

@chhagedorn
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Jun 1, 2023

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

  • 96ed139: 8308766: TLAB initialization may cause div by zero
  • 6c7225f: 8303417: RISC-V: Merge vector instructs with similar match rules
  • a46b5ac: 8308503: AArch64: SIGILL when running with -XX:UseBranchProtection=pac-ret on hardware without PAC feature
  • f9ad7df: 8300865: C2: product reduction in ProdRed_Double is not vectorized
  • 8eda97d: 8305320: DbgStrings and AsmRemarks are leaking
  • 0951474: 8309150: Need to escape " inside attribute values
  • 0119969: 8309171: Test vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/TestDescription.java fails after JDK-8308341
  • f8a924a: 8308975: Fix signed integer overflow in compiler code, part 2
  • 5531f6b: 8308819: add JDWP and JDI virtual thread support for ThreadReference.ForceEarlyReturn
  • e42a4b6: 8309236: ProblemList java/util/concurrent/locks/Lock/OOMEInAQS.java with ZGC and Generational ZGC again
  • ... and 78 more: https://git.openjdk.org/jdk/compare/90e57fd5a96199b01e7c058a4d8a82e467d7f41a...master

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Jun 1, 2023

@chhagedorn Pushed as commit dfd3da3.

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

@hungk20
Copy link

hungk20 commented Jun 12, 2023

May I know if the fix applied to jdk11 as well?

We got the same issue with jdk11.0.19.

@chhagedorn
Copy link
Member Author

@hungk20 The negation of the condition for range checks was introduced in JDK-7173584 which went into JDK 9. Therefore, JDK 11u is also affected. You could either backport this fix or backout JDK-8297951 (see JDK-8308884).

@jerboaa
Copy link
Contributor

jerboaa commented Jun 12, 2023

OpenJDK 11.0.20 in July will have JDK-8297951 backed out. We'll revisit the situation for OpenJDK 11.0.21 (October) where the actual fix (i.e. this bug) will likely get in. See https://bugs.openjdk.org/browse/JDK-8309119

@hungk20
Copy link

hungk20 commented Jun 12, 2023

Thanks @chhagedorn @jerboaa, I will keep an eye on the the new versions.

@chhagedorn chhagedorn deleted the JDK-8307683 branch August 24, 2023 06:17
@KarlHakanNordgren
Copy link

KarlHakanNordgren commented Sep 7, 2023

@jerboaa && @chhagedorn : I'm not familiar with the term "backed out". Does "OpenJDK 11.0.20 in July will have JDK-8297951 backed out." mean that JDK-8297951 has been reverted?

@chhagedorn
Copy link
Member Author

Hi @KarlHakanNordgren, yes, in OpenJDK 11.0.20, JDK-8297951 is backed out/reverted (see JDK-8309121).

There is a REDO task (JDK-8309119) which has not (yet?) made it into OpenJDK 11.0.21 but only in OpenJDK 17.0.9.

@jerboaa
Copy link
Contributor

jerboaa commented Sep 8, 2023

@KarlHakanNordgren What @chhagedorn said. Current JDK 11 release (11.0.20) have JDK-8297951 reverted.

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
6 participants