8281429: PhiNode::Value() is too conservative for tripcount of CountedLoop #7823

rwestrel · 2022-03-15T16:02:54Z

The type for the iv phi of a counted loop is computed from the types
of the phi on loop entry and the type of the limit from the exit
test. Because the exit test is applied to the iv after increment, the
type of the iv phi is at least one less than the limit (for a positive
stride, one more for a negative stride).

Also, for a stride whose absolute value is not 1 and constant init and
limit values, it's possible to compute accurately the iv phi type.

This change caused a few failures and I had to make a few adjustments
to loop opts code as well.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (1 reviews required, with at least 1 reviewer)

Issue

JDK-8281429: PhiNode::Value() is too conservative for tripcount of CountedLoop

Reviewers

Tobias Hartmann (@TobiHartmann - Reviewer) ⚠️ Review applies to 19b38997
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7823/head:pull/7823
$ git checkout pull/7823

Update a local copy of the PR:
$ git checkout pull/7823
$ git pull https://git.openjdk.java.net/jdk pull/7823/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7823

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7823.diff

bridgekeeper · 2022-03-15T16:03:06Z

👋 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 · 2022-03-15T16:05:19Z

@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 · 2022-03-15T16:09:08Z

Webrevs

TobiHartmann

Looks good to me otherwise.

src/hotspot/share/opto/cfgnode.cpp

TobiHartmann · 2022-03-22T09:45:24Z

src/hotspot/share/opto/loopPredicate.cpp

                inner_head = inner_loop->_head->as_Loop();
-                inner_head->verify_strip_mined(1);


Why did you remove the verification code?

I hit a failure in that code for an inner loop that no longer has the shape of a counted loop. Let me double check if I can still reproduce it.

In the update I just pushed, I reverted that change, added a test that causes the failure that this was supposed to fix. The new fix is the new cast in in PhaseIdealLoop::do_unroll(). Without it, c2 can't remove the back branch of a main loop and the graph contains a counted loop that doesn't have the right shape anymore (the exit test doesn't depend on the iv phi after transformation but doesn't constant fold).

src/hotspot/share/opto/loopnode.cpp

src/hotspot/share/opto/cfgnode.cpp

openjdk · 2022-03-22T10:21:30Z

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

8281429: PhiNode::Value() is too conservative for tripcount of CountedLoop

Reviewed-by: thartmann, kvn

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

5ac7186: 8282559: Allow multiple search terms in javadoc search
7022543: 8286195: ProblemList test/lib-test/jdk/test/lib/TestMutuallyExclusivePlatformPredicates.java
ede06c3: 8282060: RemoteRuntimeImageTest is not actually testing on JDK 8
dce860a: 8285947: Avoid redundant HashMap.containsKey calls in ZoneName
9d2f591: 8285987: executing shell scripts without #! fails on Alpine linux
fd41e65: 8286115: G1: G1RemSetArrayOfCardsEntriesBase off-by-one error
7e88ff8: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary
81d7475: 7132796: [macosx] closed/javax/swing/JComboBox/4517214/bug4517214.java fails on MacOS
4a5e7a1: 8282555: Missing memory edge when spilling MoveF2I, MoveD2L etc
1bb4de2: 8285956: (fs) Excessive default poll interval in PollingWatchService
... and 15 more: https://git.openjdk.java.net/jdk/compare/29c2e54cf6fe472bd75a75fedf4ecf66e204647a...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.

chhagedorn · 2022-03-22T10:41:32Z

src/hotspot/share/opto/cfgnode.cpp

+                  julong ulo = static_cast<julong>(lo->hi_as_long());
+                  julong diff = (uhi - ulo - 1) / (-stride_t->lo_as_long()) * (-stride_t->lo_as_long());
+                  julong ufirst = hi->lo_as_long() - diff;
+                  first = reinterpret_cast<jlong &>(ufirst);


Could we also use a static_cast here and further down instead?

Thanks for looking at this.
I'm not sure. I modeled this code on java_add/java_substract etc.

I thought that just for a numeric conversion, one could use static_cast as reinterpret_cast operates on pointers or references. But I'm not entirely sure what the convention should be. I guess both should work fine.

Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>

TobiHartmann

I executed testing and the "no iterations?" assert triggers. I'll follow up with details on how to reproduce.

rwestrel · 2022-03-22T13:03:38Z

Looks good to me otherwise.

Thanks for reviewing this.

TobiHartmann · 2022-03-23T07:50:19Z

The following test triggers assert(hi->hi_as_long() > lo->lo_as_long()) failed: no iterations? when executed with -XX:-TieredCompilation -Xbatch:

public class Test {
    public static void main(String[] args) {
        for (long l = (Long.MAX_VALUE - 1); l != (Long.MIN_VALUE + 100_000); l++) {
            if (l == 0) {
                throw new RuntimeException("Test failed");
            }
        }
    }
}

Please add it as regression test.

rwestrel · 2022-03-28T07:32:40Z

Please add it as regression test.

Done in the updated change.

chhagedorn · 2022-03-23T07:14:54Z

src/hotspot/share/opto/cfgnode.cpp

+                  julong ulo = static_cast<julong>(lo->hi_as_long());
+                  julong diff = (uhi - ulo - 1) / (-stride_t->lo_as_long()) * (-stride_t->lo_as_long());
+                  julong ufirst = hi->lo_as_long() - diff;
+                  first = reinterpret_cast<jlong &>(ufirst);


I thought that just for a numeric conversion, one could use static_cast as reinterpret_cast operates on pointers or references. But I'm not entirely sure what the convention should be. I guess both should work fine.

chhagedorn · 2022-03-29T15:11:01Z

test/hotspot/jtreg/compiler/c2/irTests/TestSkeletonPredicates.java

+ * @bug 8282592
+ * @summary C2: assert(false) failed: graph should be schedulable


Are the changes to this file intended? They seem to be unrelated.

They are unrelated but I noticed, while working on this change, that that test has the wrong bug/summary. So yes unrelated but also intended (not sure a separate change is required?)

chhagedorn · 2022-03-29T15:17:04Z

src/hotspot/share/opto/loopnode.cpp

@@ -844,9 +844,18 @@ bool PhaseIdealLoop::create_loop_nest(IdealLoopTree* loop, Node_List &old_new) {
  }

  // May not have gone thru igvn yet so don't use _igvn.type(phi) (PhaseIdealLoop::is_counted_loop() sets the iv phi's type)


Comment seems to be outdated as we are not querying the type of the phi anymore. Can you update it?

Right. I updated it.

TobiHartmann

Looks good to me. All tests passed.

rwestrel · 2022-04-25T09:24:58Z

Thanks @TobiHartmann
I updated the change again: I removed a change from loopTransform.cpp that's no longer necessary after 8273115.

TobiHartmann · 2022-04-28T12:22:43Z

Still looks good to me. Another review would be good.

vnkozlov

There should be correctness tests for MAX_INT,MIN_INT,MAX_LONG,MIN_LONG boundaries, positive and negative strides and abs(stride) != 1. All combinations.

vnkozlov · 2022-04-28T16:24:21Z

src/hotspot/share/opto/cfgnode.cpp

@@ -1117,9 +1117,35 @@ const Type* PhiNode::Value(PhaseGVN* phase) const {
          if (bt != BoolTest::ne) {
            if (stride_t->hi_as_long() < 0) {          // Down-counter loop
              swap(lo, hi);
-              return TypeInteger::make(MIN2(lo->lo_as_long(), hi->lo_as_long()), hi->hi_as_long(), 3, l->bt())->filter_speculative(_type);
+              jlong first = lo->lo_as_long();


first is misleading/confusing name here. I assume first is init but it is limit in this case. I would prefer to have corresponding name to low limit of range.

vnkozlov · 2022-04-28T16:34:41Z

src/hotspot/share/opto/cfgnode.cpp

@@ -1117,9 +1117,35 @@ const Type* PhiNode::Value(PhaseGVN* phase) const {
          if (bt != BoolTest::ne) {
            if (stride_t->hi_as_long() < 0) {          // Down-counter loop
              swap(lo, hi);
-              return TypeInteger::make(MIN2(lo->lo_as_long(), hi->lo_as_long()), hi->hi_as_long(), 3, l->bt())->filter_speculative(_type);
+              jlong first = lo->lo_as_long();
+              if (first < max_signed_integer(l->bt())) {


As I understand this condition is to avoid overflow in next statement. And I thought it should take stride into account:

if (first < (max_signed_integer(l->bt()) + stride_t->hi_as_long() + 1)) {

But since we don't know (in general) what final iv value would be with abs(stride) != 1 using value 1 is conservative and correct here.

In short, this condition and following statement needs comment to explain why 1 is used.

vnkozlov · 2022-04-28T16:49:58Z

src/hotspot/share/opto/cfgnode.cpp

+              jlong first = lo->lo_as_long();
+              if (first < max_signed_integer(l->bt())) {
+                first += 1; // lo is after decrement
+                // When bounds are constant and ABS(stride) greater than 1, exact bounds for the phi can be computed


The comment is confusing - it sounds like we can calculate it only when stride is not 1. I think you mean:

// Exact bounds for the phi can be computed with ABS(stride) greater than 1 when bounds are constant.

vnkozlov · 2022-04-28T17:06:26Z

src/hotspot/share/opto/cfgnode.cpp

+              if (first < max_signed_integer(l->bt())) {
+                first += 1; // lo is after decrement
+                // When bounds are constant and ABS(stride) greater than 1, exact bounds for the phi can be computed
+                if (lo->is_con() && hi->is_con() && hi->lo_as_long() > lo->hi_as_long() && stride_t->lo_as_long() != -1) {


stride is constant. May be used the value instead of calling stride_t->lo_as_long() 3 times in this code.

rwestrel · 2022-04-29T14:05:07Z

Hi Vladimir. Thanks for reviewing this.

There should be correctness tests for MAX_INT,MIN_INT,MAX_LONG,MIN_LONG boundaries, positive and negative strides and abs(stride) != 1. All combinations.

That's reasonable but what kind of tests? Executing a simple counted loop that iterates from MIN_INT to MAX_INT is unlikely to lead to an incorrect result even if the iv type is wrong.

vnkozlov · 2022-04-29T23:55:06Z

Hi Vladimir. Thanks for reviewing this.

There should be correctness tests for MAX_INT,MIN_INT,MAX_LONG,MIN_LONG boundaries, positive and negative strides and abs(stride) != 1. All combinations.

That's reasonable but what kind of tests? Executing a simple counted loop that iterates from MIN_INT to MAX_INT is unlikely to lead to an incorrect result even if the iv type is wrong.

I am concern about unsigned arithmetic to calculate new limit for long indexing case. The test could simple fill up an array and you then check that values in it are correct (and no out of bounds references). You can choose big stride to run test fast.

rwestrel · 2022-05-02T14:53:54Z

I am concern about unsigned arithmetic to calculate new limit for long indexing case. The test could simple fill up an array and you then check that values in it are correct (and no out of bounds references). You can choose big stride to run test fast.

The problem is this code:
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/loopnode.cpp#L1762

It makes it impossible to construct a counted loop with limit Integer.MAX_VALUE with a big stride.

vnkozlov · 2022-05-02T16:06:42Z

I am fine with testing range [MIN_VALUE + stride, MAX_VALUE - stride] to exercise unsigned arithmetic. Whatever maximum loopopts allows.

rwestrel · 2022-05-05T08:27:41Z

@vnkozlov the new commits should address your comments. Let me know if the new tests cover what you asked for.

vnkozlov

This looks good. Thank you.

rwestrel · 2022-05-06T08:23:29Z

@vnkozlov thanks for the review

rwestrel · 2022-05-06T08:23:36Z

/integrate

openjdk · 2022-05-06T08:24:48Z

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

dd06cc6: 8283807: Handle CompileThreshold the same as other thresholds when scaled with -XX:CompileThresholdScaling
015cfda: 8262004: Classpath separator: Man page says semicolon; should be colon on Linux
9425ab2: 8286153: Remove redundant casts and other cleanup
7ebc4bc: 8286066: assert(k != __null) failed: klass not loaded caused by FillerObject_klass
6a1b145: 8286029: Add classpath exemption to globals_vectorApiSupport_***.S.inc
59ef76a: 8285497: Add system property for Java SE specification maintenance version
6d7e446: 8283306: re-resolving indirect call to non-entrant nmethod can crash
4957bc7: 8286056: AArch64: clarify uses of MacroAssembler::far_call/MacroAssembler::far_jump
e7adc28: 8284675: "jpackage.exe" creates application launcher without Windows Application Manfiest
9644a31: 8285616: [macos] Incorrect path for launcher-as-service.txt in .cfg file
... and 28 more: https://git.openjdk.java.net/jdk/compare/29c2e54cf6fe472bd75a75fedf4ecf66e204647a...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-05-06T08:25:06Z

@rwestrel Pushed as commit fa1ca98.

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

fix & test

a8722db

openjdk bot added the rfr Pull request is ready for review label Mar 15, 2022

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Mar 15, 2022

TobiHartmann approved these changes Mar 22, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 22, 2022

chhagedorn reviewed Mar 22, 2022

View reviewed changes

rwestrel and others added 5 commits March 22, 2022 13:47

Update src/hotspot/share/opto/loopnode.cpp

6861729

Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>

Update src/hotspot/share/opto/loopnode.cpp

dab28f0

Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>

Update src/hotspot/share/opto/cfgnode.cpp

aaa19f3

Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>

Update src/hotspot/share/opto/cfgnode.cpp

1532675

Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>

Merge branch 'master' into JDK-8281429

03625e0

TobiHartmann suggested changes Mar 22, 2022

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Mar 22, 2022

review

ed91dd8

exp

97b10aa

rwestrel added 5 commits March 23, 2022 11:29

Merge branch 'master' into JDK-8281429

5b217f5

other fix

2e7ba9c

test & fix

bc27e47

more test

2ec58ba

test fix

3a087b2

chhagedorn reviewed Mar 29, 2022

View reviewed changes

rwestrel added 2 commits April 6, 2022 15:50

undo

a184f43

Merge branch 'master' into JDK-8281429

bd1b69a

TobiHartmann approved these changes Apr 20, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 20, 2022

rwestrel added 2 commits April 25, 2022 10:36

Merge branch 'master' into JDK-8281429

d565c51

undo unneeded change

19b3899

TobiHartmann approved these changes Apr 28, 2022

View reviewed changes

vnkozlov reviewed Apr 28, 2022

View reviewed changes

review

80104fe

rwestrel added 2 commits May 4, 2022 16:06

Merge branch 'master' into JDK-8281429

3aa9684

more review

d4765a9

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 5, 2022

whitespaces

6d14559

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 5, 2022

vnkozlov approved these changes May 5, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label May 6, 2022

openjdk bot closed this May 6, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 6, 2022

		inner_head = inner_loop->_head->as_Loop();
		inner_head->verify_strip_mined(1);

		* @bug 8282592
		* @summary C2: assert(false) failed: graph should be schedulable

		@@ -844,9 +844,18 @@ bool PhaseIdealLoop::create_loop_nest(IdealLoopTree* loop, Node_List &old_new) {
		}

		// May not have gone thru igvn yet so don't use _igvn.type(phi) (PhaseIdealLoop::is_counted_loop() sets the iv phi's type)

8281429: PhiNode::Value() is too conservative for tripcount of CountedLoop #7823

8281429: PhiNode::Value() is too conservative for tripcount of CountedLoop #7823

Conversation

rwestrel commented Mar 15, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 15, 2022

openjdk bot commented Mar 15, 2022

mlbridge bot commented Mar 15, 2022 • edited

Webrevs

TobiHartmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Mar 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TobiHartmann left a comment

Choose a reason for hiding this comment

rwestrel commented Mar 22, 2022

TobiHartmann commented Mar 23, 2022

rwestrel commented Mar 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chhagedorn Mar 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TobiHartmann left a comment

Choose a reason for hiding this comment

rwestrel commented Apr 25, 2022

TobiHartmann commented Apr 28, 2022

vnkozlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwestrel commented Apr 29, 2022

vnkozlov commented Apr 29, 2022

rwestrel commented May 2, 2022

vnkozlov commented May 2, 2022

rwestrel commented May 5, 2022

vnkozlov left a comment

Choose a reason for hiding this comment

rwestrel commented May 6, 2022

rwestrel commented May 6, 2022

openjdk bot commented May 6, 2022

openjdk bot commented May 6, 2022

rwestrel commented Mar 15, 2022 •

edited by openjdk bot

mlbridge bot commented Mar 15, 2022 •

edited

openjdk bot commented Mar 22, 2022 •

edited

chhagedorn Mar 29, 2022 •

edited