JDK-8272574: C2: assert(false) failed: Bad graph detected in build_loop_late #5142

casparcwang · 2021-08-17T12:02:21Z

Current loop predication will promote nodes(with a dependency on a later control node) to the insertion point which dominates the later control node.

In the following example, loopPrediction will promote node 434 to the outer loop(predicted insert point is right after node 424), and it depends on control node 207. But node 424 dominates node 207, which means after the promotion, the cloned nodes have a control dependency on a later control node, which leads to a bad graph.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8272574: C2: assert(false) failed: Bad graph detected in build_loop_late

Reviewers

Tobias Hartmann (@TobiHartmann - Reviewer)
Christian Hagedorn (@chhagedorn - Reviewer) ⚠️ Review applies to 332d7a1
Vladimir Kozlov (@vnkozlov - Reviewer)

Contributors

Hui Shi <hshi@openjdk.org>
Christian Hagedorn <chagedorn@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5142

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

Using diff file

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

bridgekeeper · 2021-08-17T12:02:30Z

👋 Welcome back casparcwang! 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 · 2021-08-17T12:04:20Z

@casparcwang 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 · 2021-08-17T12:08:41Z

Webrevs

casparcwang · 2021-08-17T12:57:54Z

/contributor add hshi

openjdk · 2021-08-17T12:58:19Z

@casparcwang Could not parse hash as a valid contributor.
Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

/contributor add @openjdk-bot
/contributor add duke
/contributor add J. Duke <duke@openjdk.org>

casparcwang · 2021-08-17T13:02:03Z

/contributor add hshi

openjdk · 2021-08-17T13:02:45Z

@casparcwang
Contributor Hui Shi <hshi@openjdk.org> successfully added.

dean-long · 2021-08-17T23:32:35Z

@casparcwang @hshi, does the test assert in a debug build? I'm just wondering if this is related to other bugs.

casparcwang · 2021-08-18T01:33:29Z

@casparcwang @hshi, does the test assert in a debug build? I'm just wondering if this is related to other bugs.

Thank you for your review. The test asserts in fastdebug & slowdebug build, and give the assert message of assert(false) failed: Bad graph detected in build_loop_late.

casparcwang · 2021-08-18T04:06:44Z

@casparcwang @hshi, does the test assert in a debug build? I'm just wondering if this is related to other bugs.

The test 'TestBufferVectorization.java' in the pre-submit tests failed, but it does not failed on my machine. The jit code of 'compiler.vectorization.TestBufferVectorization$TestBuffer::run' is different after applying the patch: one loop predication is not allowed but another different loop predication is performed. So I agree with you that the patch is not complete, there must exist other latent bugs.

dean-long · 2021-08-18T04:20:10Z

We have two other bugs failing with the same assert: JDK-8272562 and JDK-8271954. Could they be related?

TobiHartmann

Could you please explain in more detail how we ended up with the 400 LoadI having a dependency on 207 IfTrue which is below the predicate insertion point? Is it part of another predicate? I think it would help if you could add comments to the test, explaining what is going on (i.e. where we insert predicates for which range checks).

Why did you add the test to loopstripmining/LoadSplitThruPhi.java? It's unrelated to loop strip mining, right?

TobiHartmann · 2021-08-18T07:39:29Z

test/hotspot/jtreg/compiler/loopstripmining/LoadSplitThruPhi.java

@@ -54,6 +67,23 @@ public static void main(String[] args) {
        for (int i = 0; i < 1000000; i++) {
            getPermutations(inputArray, outputArray);
        }
+
+        for (int i = 0; i < 100; ++i) {
+            Thread t = new Thread(new Runnable() {


Why do you need a new thread?

for (int a = 0; a < (int)(a + 16); a++) { only will terminate when a overflows, so I add a thread to execute the test, and use main thread to control the test time. I will refactor the test if the final fix is done.

TobiHartmann · 2021-08-18T07:40:51Z

src/hotspot/share/opto/loopPredicate.cpp

@@ -662,6 +662,10 @@ bool IdealLoopTree::is_range_check_if(IfNode *iff, PhaseIdealLoop *phase, Invari
  if (offset && !invar.is_invariant(offset)) { // offset must be invariant
    return false;
  }
+  if (offset && phase->is_dominator(predicate_proj, phase->get_ctrl(offset))) {
+    // offset must be able to float before predicte projection node


predicte -> predicate

TobiHartmann · 2021-08-18T07:51:28Z

test/hotspot/jtreg/compiler/loopstripmining/LoadSplitThruPhi.java

+    public static void getPermutations1(byte[] inputArray, byte[][] outputArray) {
+        int[] indexes = new int[]{0, 2};
+
+        for (int a = 0; a < (int)(a + 16); a++) {


The int cast seems useless.

chhagedorn · 2021-08-18T08:47:02Z

We have two other bugs failing with the same assert: JDK-8272562 and JDK-8271954. Could they be related?

They are both regression from JDK-8252372 (split if optimization) and unrelated to this bug which is related to loop predication.

casparcwang · 2021-08-18T12:18:29Z

Could you please explain in more detail how we ended up with the 400 LoadI having a dependency on 207 IfTrue which is below the predicate insertion point? Is it part of another predicate? I think it would help if you could add comments to the test, explaining what is going on (i.e. where we insert predicates for which range checks).

Why did you add the test to loopstripmining/LoadSplitThruPhi.java? It's unrelated to loop strip mining, right?

Thank you for your review very much!

I removed one line in the test to simplify the graph, the idx of nodes changes, but the logic is the same.

    public static void getPermutations (byte[] inputArray, byte[][] outputArray) {
        int[] indexes = new int[]{0, 2};
        for (int a = 0; a < (int)(a + 16); a++) {
            int oneIdx = indexes[0]++;
            for (int b = a + 1; b < inputArray.length; b++) {
                int twoIdx = indexes[1]++;
                outputArray[twoIdx][0] = inputArray[a];
            }
        }
    }

The node corresponding relation is:
1, the range check of outputArray[twoIdx] is node 248
2, int oneIdx = indexes[0] is loadi node 398 which have a dependency of node 207
3, loop predicate of b < inputArray.length is node 207
4, loadi node 398 is created through LoadNode::split_through_phi of the original parsed bytecode of int twoIdx = indexes[1].

Loop predication optimization wants to float the range check of outputArray[twoIdx] out of the inner for loop, but it starts to clone from node 432 with the following stack trace:

#1  0x00007ffff622d4d9 in Node::Node (this=0x7fff5809abf0, n0=0x0, n1=0x7fff5809a3d0, n2=0x7fff5809a450) at /data/openjdk/jdk_dev/src/hotspot/share/opto/node.cpp:386
#2  0x00007ffff57c213d in AddNode::AddNode (this=0x7fff5809abf0, in1=0x7fff5809a3d0, in2=0x7fff5809a450) at /data/openjdk/jdk_dev/src/hotspot/share/opto/addnode.hpp:44
#3  0x00007ffff57c21e9 in AddINode::AddINode (this=0x7fff5809abf0, in1=0x7fff5809a3d0, in2=0x7fff5809a450) at /data/openjdk/jdk_dev/src/hotspot/share/opto/addnode.hpp:88
#4  0x00007ffff60c1784 in PhaseIdealLoop::is_scaled_iv_plus_offset (this=0x7fff98123a90, exp=0x7fff5c05fcc8, iv=0x7fff58082ce0, p_scale=0x7fff98122e98, p_offset=0x7fff98122ea0, depth=0) at /data/openjdk/jdk_dev/src/hotspot/share/opto/loopTransform.cpp:2527
#5  0x00007ffff60b2718 in PhaseIdealLoop::loop_predication_impl_helper (this=0x7fff98123a90, loop=0x7fff5807df08, proj=0x7fff58084538, predicate_proj=0x7fff58130bd8, cl=0x7fff5c05daf8, zero=0x7fff581278d0, invar=..., reason=Deoptimization::Reason_predicate)
    at /data/openjdk/jdk_dev/src/hotspot/share/opto/loopPredicate.cpp:1154
#6  0x00007ffff60b3a71 in PhaseIdealLoop::loop_predication_impl (this=0x7fff98123a90, loop=0x7fff5807df08) at /data/openjdk/jdk_dev/src/hotspot/share/opto/loopPredicate.cpp:1414
#7  0x00007ffff60b3eab in IdealLoopTree::loop_predication (this=0x7fff5807df08, phase=0x7fff98123a90) at /data/openjdk/jdk_dev/src/hotspot/share/opto/loopPredicate.cpp:1473
#8  0x00007ffff60b3e58 in IdealLoopTree::loop_predication (this=0x7fff5807dfd0, phase=0x7fff98123a90) at /data/openjdk/jdk_dev/src/hotspot/share/opto/loopPredicate.cpp:1468
#9  0x00007ffff60b3e58 in IdealLoopTree::loop_predication (this=0x7fff5807e098, phase=0x7fff98123a90) at /data/openjdk/jdk_dev/src/hotspot/share/opto/loopPredicate.cpp:1468
#10 0x00007ffff60dccff in PhaseIdealLoop::build_and_optimize (this=0x7fff98123a90, mode=LoopOptsSkipSplitIf) at /data/openjdk/jdk_dev/src/hotspot/share/opto/loopnode.cpp:3994
#11 0x00007ffff5a9dc8c in PhaseIdealLoop::PhaseIdealLoop (this=0x7fff98123a90, igvn=..., mode=LoopOptsSkipSplitIf) at /data/openjdk/jdk_dev/src/hotspot/share/opto/loopnode.hpp:1068
#12 0x00007ffff5a9de7e in PhaseIdealLoop::optimize (igvn=..., mode=LoopOptsSkipSplitIf) at /data/openjdk/jdk_dev/src/hotspot/share/opto/loopnode.hpp:1146
#13 0x00007ffff5a91a75 in Compile::Optimize (this=0x7fff98126cb0) at /data/openjdk/jdk_dev/src/hotspot/share/opto/compile.cpp:2198
#14 0x00007ffff5a8aec2 in Compile::Compile (this=0x7fff98126cb0, ci_env=0x7fff98127a20, target=0x7fff580aaa58, osr_bci=-1, subsume_loads=true, do_escape_analysis=true, eliminate_boxing=true, do_locks_coarsening=true, install_code=true, directive=
    0x7ffff01f2df0) at /data/openjdk/jdk_dev/src/hotspot/share/opto/compile.cpp:781
#15 0x00007ffff5978068 in C2Compiler::compile_method (this=0x7ffff02f24b0, env=0x7fff98127a20, target=0x7fff580aaa58, entry_bci=-1, install_code=true, directive=0x7ffff01f2df0) at /data/openjdk/jdk_dev/src/hotspot/share/opto/c2compiler.cpp:107
#16 0x00007ffff5aa83a2 in CompileBroker::invoke_compiler_on_method (task=0x7ffff04c9760) at /data/openjdk/jdk_dev/src/hotspot/share/compiler/compileBroker.cpp:2291
#17 0x00007ffff5aa6edd in CompileBroker::compiler_thread_loop () at /data/openjdk/jdk_dev/src/hotspot/share/compiler/compileBroker.cpp:1964
#18 0x00007ffff5ac7819 in CompilerThread::thread_entry (thread=0x7ffff02f2ad0, __the_thread__=0x7ffff02f2ad0) at /data/openjdk/jdk_dev/src/hotspot/share/compiler/compilerThread.cpp:59
#19 0x00007ffff653cb85 in JavaThread::thread_main_inner (this=0x7ffff02f2ad0) at /data/openjdk/jdk_dev/src/hotspot/share/runtime/thread.cpp:1270
#20 0x00007ffff653ca1b in JavaThread::run (this=0x7ffff02f2ad0) at /data/openjdk/jdk_dev/src/hotspot/share/runtime/thread.cpp:1253
#21 0x00007ffff653a33e in Thread::call_run (this=0x7ffff02f2ad0) at /data/openjdk/jdk_dev/src/hotspot/share/runtime/thread.cpp:361
#22 0x00007ffff628225d in thread_native_entry (thread=0x7ffff02f2ad0) at /data/openjdk/jdk_dev/src/hotspot/os/linux/os_linux.cpp:720
#23 0x00007ffff79b0e25 in start_thread (arg=0x7fff98128700) at pthread_create.c:308
#24 0x00007ffff74d935d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:113

So the range check of outputArray[twoIdx] floats out of the inner for loop, but have a dependency of node 398 LoadI, whose control input is node 207 (loop predicate of b < inputArray.length), and node 207 is also dominated by the predicate insertion point. In the later stage, the optimization found this cycle dependency makes a bad graph.

The predicate insertion point is calculated from node 207, and skips all the predicates, which leads to node 183.

So, the solution is very straight forward: stop the predication to be performed if it has a control which is dominated by the predication point. But the implementation seems to have some latent problem.

chhagedorn · 2021-08-18T13:47:30Z

So the range check of outputArray[twoIdx] floats out of the inner for loop, but have a dependency of node 398 LoadI, whose control input is node 207 (loop predicate of b < inputArray.length), and node 207 is also dominated by the predicate insertion point. In the later stage, the optimization found this cycle dependency makes a bad graph.

So, the solution is very straight forward: stop the predication to be performed if it has a control which is dominated by the predication point. But the implementation seems to have some latent problem.

I'm not sure if this is too restrictive to not apply loop predication at all. It seems to me that this 248 RangeCheck could indeed be moved out of the loop but this pin of 398 LoadI does not allow it which is unfortunate. Can we fix that somehow? Are we too strict when setting the control input in LoadNode::split_through_phi()?

I'm currently looking at:

jdk/src/hotspot/share/opto/memnode.cpp

Lines 1627 to 1629 in 30b0f82

    
           if (this->in(0) == region) { 
        
             x->set_req(0, in); 
        
           } else {

wondering if we could set the control input of the cloned load to the same control input as the memory input of the phi (if the control input of the old load was the region of the phi through which we split)? Something like this:

      if (this->in(0) == region) {
        if (mem->is_Phi() && (mem->in(0) == region) && mem->in(i)->in(0) != NULL) {
          x->set_req(0, mem->in(i)->in(0)); // Use same control as memory
        } else {
          x->set_req(0, in);
        }
      }

casparcwang · 2021-08-19T02:57:12Z

We have two other bugs failing with the same assert: JDK-8272562 and JDK-8271954. Could they be related?

They are both regression from JDK-8252372 (split if optimization) and unrelated to this bug which is related to loop predication.

Thank you for your review and explanation.

I'm not sure if this is too restrictive to not apply loop predication at all. It seems to me that this 248 RangeCheck could indeed be moved out of the loop but this pin of 398 LoadI does not allow it which is unfortunate. Can we fix that somehow? Are we too strict when setting the control input in LoadNode::split_through_phi()?

I'm currently looking at:

jdk/src/hotspot/share/opto/memnode.cpp

Lines 1627 to 1629 in 30b0f82

if (this->in(0) == region) {

x->set_req(0, in);

} else {

wondering if we could set the control input of the cloned load to the same control input as the memory input of the phi (if the control input of the old load was the region of the phi through which we split)? Something like this:
      if (this->in(0) == region) {
        if (mem->is_Phi() && (mem->in(0) == region) && mem->in(i)->in(0) != NULL) {
          x->set_req(0, mem->in(i)->in(0)); // Use same control as memory
        } else {
          x->set_req(0, in);
        }
      } 

I agree with you and your suggested fix looks more elegant in preventing such pinned load. I will analyze and test it to see whether it has unexpected side effects.

What do you think about if the following guard is added to IdealLoopTree::is_range_check_if, which prevents loop predications float to predication point with dependency on the skipped predications(which is right after the predication point). When your suggested fix is applied, there is no pinned load currently. But I'm not sure whether it will occur again in the future.

  if (offset && phase->has_ctrl(offset)) {
    Node* offset_ctrl = phase->get_ctrl(offset);
    if (phase->get_loop(predicate_proj) == phase->get_loop(offset_ctrl) &&
        phase->is_dominator(predicate_proj, offset_ctrl)) {
      return false;
    }
  }

chhagedorn · 2021-08-19T11:20:23Z

I agree with you and your suggested fix looks more elegant in preventing such pinned load. I will analyze and test it to see whether it has unexpected side effects.

Thanks for having a closer look at my suggestion.

What do you think about if the following guard is added to IdealLoopTree::is_range_check_if, which prevents loop predications float to predication point with dependency on the skipped predications(which is right after the predication point). When your suggested fix is applied, there is no pinned load currently. But I'm not sure whether it will occur again in the future.

Given that we could fix this bug without a bail out of loop predication, either by my suggested fix or something else, I'd say that this check should never be taken. And if it is taken it indicates a missed range check elimination opportunity. But you're right, we cannot really prove that there are currently no other such cases. I would therefore suggest to either turn your bailout code into an assertion check or keep your bailout but add an assert(false) to catch a missed case in the future to further investigate. If we will at some point face a non-fixable case we can still remove the assertion code again. But of course, all of this only applies if we can fix the current bug without bailing out of loop predication.

casparcwang · 2021-08-20T06:07:38Z

/contributor add chhagedorn

openjdk · 2021-08-20T06:08:50Z

@casparcwang Could not parse chhagedorn as a valid contributor.
Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

/contributor add @openjdk-bot
/contributor add duke
/contributor add J. Duke <duke@openjdk.org>

casparcwang · 2021-08-20T06:29:53Z

Given that we could fix this bug without a bail out of loop predication, either by my suggested fix or something else, I'd say that this check should never be taken. And if it is taken it indicates a missed range check elimination opportunity. But you're right, we cannot really prove that there are currently no other such cases. I would therefore suggest to either turn your bailout code into an assertion check or keep your bailout but add an assert(false) to catch a missed case in the future to further investigate. If we will at some point face a non-fixable case we can still remove the assertion code again. But of course, all of this only applies if we can fix the current bug without bailing out of loop predication.

Thank you very much! I have make the bailing out code into an assertion, and include your suggested fix in this patch.

The first thing I worry about is: change the control of load will lead to wrong code sequence of different nodes. But during the tests, I found no problems. For two different loads, if a volatile load l1 is before another normal load l2, the later load l2 will have a memory dependency on the acquire memory bar node of the volatile load l1, so the final code sequence will be guaranteed. For two normal loads, the disorder has no problem. So I can not find a scenario which leads to the wrong code generation of your suggested fix, please remind me if you find something, and I will keep on testing.

I have run tests with your suggested fix, and find no problem.

casparcwang · 2021-08-20T06:31:36Z

/contributor add chagedorn

casparcwang · 2021-08-23T08:29:16Z

Otherwise, looks good! Thanks for making these changes.

I tested your code through tier1-7 over the weekend. Test results looked good!

Thanks for your help and testing. I have also run tests during last weekend, and find no problem currently.

The comment has been modified according to your suggestion.

chhagedorn

Otherwise, looks good! Thanks for making these changes.
I tested your code through tier1-7 over the weekend. Test results looked good!

Thanks for your help and testing. I have also run tests during last weekend, and find no problem currently.

Great! I'm curious what others think about it.

chhagedorn · 2021-08-23T12:26:09Z

src/hotspot/share/opto/loopPredicate.cpp

+      // then it will lead to cyclic dependency.
+      // Previously promoted loop predication is in the same loop of predication
+      // point.
+      // This situation can occur when pinning nodes too conservatively. - can we do better?


Dot can be removed before the -.

casparcwang · 2021-08-30T11:59:06Z

May I have more review of this pr?

vnkozlov

Place the test into compiler/loopopts.

vnkozlov · 2021-09-01T16:28:19Z

src/hotspot/share/opto/loopPredicate.cpp

@@ -662,6 +662,20 @@ bool IdealLoopTree::is_range_check_if(IfNode *iff, PhaseIdealLoop *phase, Invari
  if (offset && !invar.is_invariant(offset)) { // offset must be invariant
    return false;
  }
+#ifndef PRODUCT


Should be #ifdef ASSERT for debug build.

On other hand, consider do this check in PRODUCT too to bailout (return false).

vnkozlov · 2021-09-01T16:30:21Z

src/hotspot/share/opto/loopPredicate.cpp

+      // Previously promoted loop predication is in the same loop of predication
+      // point.
+      // This situation can occur when pinning nodes too conservatively - can we do better?
+      assert(false, "cyclic dependency prevents range check elimination");


Print useful data before assert to help debugging issue without rerunning.
Dump offset node, for example.

vnkozlov · 2021-09-01T16:38:54Z

src/hotspot/share/opto/loopPredicate.cpp

@@ -621,7 +621,7 @@ class Invariance : public StackObj {
 // Returns true if the predicate of iff is in "scale*iv + offset u< load_range(ptr)" format
 // Note: this function is particularly designed for loop predication. We require load_range
 //       and offset to be loop invariant computed on the fly by "invar"
-bool IdealLoopTree::is_range_check_if(IfNode *iff, PhaseIdealLoop *phase, Invariance& invar) const {
+bool IdealLoopTree::is_range_check_if(IfNode *iff, PhaseIdealLoop *phase, Invariance& invar, ProjNode *predicate_proj) const {


predicate_proj is used only for assert. Use DEBUG_ONLY(, ProjNode *predicate_proj).
Unless you want to do the check in PRODUCT.

vnkozlov · 2021-09-01T16:39:22Z

src/hotspot/share/opto/loopPredicate.cpp

@@ -1141,7 +1155,7 @@ bool PhaseIdealLoop::loop_predication_impl_helper(IdealLoopTree *loop, ProjNode*
      loop->dump_head();
    }
 #endif
-  } else if (cl != NULL && loop->is_range_check_if(iff, this, invar)) {
+  } else if (cl != NULL && loop->is_range_check_if(iff, this, invar, predicate_proj)) {


Use DEBUG_ONLY(, predicate_proj).

vnkozlov · 2021-09-01T16:39:30Z

src/hotspot/share/opto/loopnode.hpp

@@ -737,7 +737,7 @@ class IdealLoopTree : public ResourceObj {
  bool policy_range_check( PhaseIdealLoop *phase ) const;

  // Return TRUE if "iff" is a range check.
-  bool is_range_check_if(IfNode *iff, PhaseIdealLoop *phase, Invariance& invar) const;
+  bool is_range_check_if(IfNode *iff, PhaseIdealLoop *phase, Invariance& invar, ProjNode *predicate_proj) const;


Use DEBUG_ONLY(, ProjNode *predicate_proj).

vnkozlov · 2021-09-01T16:51:29Z

src/hotspot/share/opto/memnode.cpp

@@ -1625,7 +1625,13 @@ Node *LoadNode::split_through_phi(PhaseGVN *phase) {
      the_clone = x;            // Remember for possible deletion.
      // Alter data node to use pre-phi inputs
      if (this->in(0) == region) {
-        x->set_req(0, in);
+        if (mem->is_Phi() && (mem->in(0) == region) && mem->in(i)->in(0) != NULL) {


I have concern about this because you can have load's address dependency on a control node below memory's control.
I saw cases when Load's address and memory control were Region through which it splits. And all controls were set to Region's inputs.
Also consider that we did not split memory node yet. We may end up with load's clone placed above its memory.

…ontrol.

casparcwang · 2021-09-02T04:17:23Z

Place the test into compiler/loopopts.

Thank you for your review. The test has moved to loopopt dir.

Should be #ifdef ASSERT for debug build.

The assertion has been changed to debug only, and more debug information are printed.

I have concern about this because you can have load's address dependency on a control node below memory's control.
I saw cases when Load's address and memory control were Region through which it splits. And all controls were set to Region's inputs.

I have the same concern too, so I just add another guard MemNode::all_controls_dominate(address, region).

Also consider that we did not split memory node yet. We may end up with load's clone placed above its memory.

when the control of the load is changed to the control of its memory, memory dependency of the load is also set in the following:

      if (mem->is_Phi() && (mem->in(0) == region)) {
        x->set_req(Memory, mem->in(i)); // Use pre-Phi input for the clone.
      }

vnkozlov

Okay. Let me test latest version.

vnkozlov

Testing passed clean.

casparcwang · 2021-09-03T01:03:25Z

Testing passed clean.

Thank you very much for the testing and review.

casparcwang · 2021-09-07T11:06:52Z

The number of required reviews for this PR is now set to 3 (with at least 1 of role reviewers).

ping, this PR needs 3 reviewers. May I have more review of this PR?

TobiHartmann

Looks reasonable to me.

openjdk · 2021-09-14T06:50:29Z

⚠️ @casparcwang the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout JDK-8272574
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

openjdk · 2021-09-14T06:50:31Z

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

8272574: C2: assert(false) failed: Bad graph detected in build_loop_late

Co-authored-by: Hui Shi <hshi@openjdk.org>
Co-authored-by: Christian Hagedorn <chagedorn@openjdk.org>
Reviewed-by: thartmann, chagedorn, 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 296 new commits pushed to the master branch:

e7ab372: 8273641: (bf) Buffer subclasses documentation contains template strings
22a7191: 8273040: Turning off JpAllowDowngrades (or Upgrades)
394ebc8: 8270553: Tests should not use (real, in-use, routable) 1.1.1.1 as dummy IP value
0f31d0f: 8273373: Zero: Cannot invoke JVM in primordial threads on Zero
fe89dd3: 8271254: javac generates unreachable code when using empty semicolon statement
8974b95: 8273730: WorkGangBarrierSync constructor unused
1d3eb14: 8273635: Attempting to acquire lock StackWatermark_lock/9 out of order with lock tty_lock/3
31667da: 8273491: java.util.spi.LocaleServiceProvider spec contains statement that is too strict
ed7789d: 8256977: Bump minimum GCC from 5.x to 6 for JDK
5bfd043: 8273497: building.md should link to both md and html
... and 286 more: https://git.openjdk.java.net/jdk/compare/0e3fde6c3c2f5c05777b79ff5eb1188014269b0f...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TobiHartmann, @chhagedorn, @vnkozlov) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

casparcwang · 2021-09-14T06:55:56Z

Looks reasonable to me.

Thank you for the review.

casparcwang · 2021-09-14T06:56:06Z

/integrate

DamonFool · 2021-09-14T07:18:18Z

/sponsor

openjdk · 2021-09-14T20:51:44Z

@casparcwang
Your change (at version 90ec471) is now ready to be sponsored by a Committer.

openjdk · 2021-09-14T20:55:25Z

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

e7ab372: 8273641: (bf) Buffer subclasses documentation contains template strings
22a7191: 8273040: Turning off JpAllowDowngrades (or Upgrades)
394ebc8: 8270553: Tests should not use (real, in-use, routable) 1.1.1.1 as dummy IP value
0f31d0f: 8273373: Zero: Cannot invoke JVM in primordial threads on Zero
fe89dd3: 8271254: javac generates unreachable code when using empty semicolon statement
8974b95: 8273730: WorkGangBarrierSync constructor unused
1d3eb14: 8273635: Attempting to acquire lock StackWatermark_lock/9 out of order with lock tty_lock/3
31667da: 8273491: java.util.spi.LocaleServiceProvider spec contains statement that is too strict
ed7789d: 8256977: Bump minimum GCC from 5.x to 6 for JDK
5bfd043: 8273497: building.md should link to both md and html
... and 286 more: https://git.openjdk.java.net/jdk/compare/0e3fde6c3c2f5c05777b79ff5eb1188014269b0f...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-09-14T20:55:42Z

@DamonFool @casparcwang Pushed as commit 16c3ad1.

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

casparcwang added 2 commits August 17, 2021 19:31

fix wrong loop predication

09ed06b

Add test

d06dc09

openjdk bot added the rfr Pull request is ready for review label Aug 17, 2021

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Aug 17, 2021

Fix version

dd41df3

TobiHartmann suggested changes Aug 18, 2021

View reviewed changes

casparcwang added 2 commits August 19, 2021 12:30

Fix test and add a guard

613809f

Add comment & remove useless cast

a83a6e2

casparcwang changed the title ~~JDK-8272574: Crashes in PhaseIdealLoop::build_loop_late_post_work~~ JDK-8272574: C2: assert(false) failed: Bad graph detected in build_loop_late Aug 19, 2021

Prevent pinned loadi, and assert if found wrong loop predication

66bd853

Make the comment more professional

332d7a1

chhagedorn approved these changes Aug 23, 2021

View reviewed changes

Remove un-necessary dot

4ce8b2b

vnkozlov reviewed Sep 1, 2021

View reviewed changes

casparcwang added 2 commits September 2, 2021 11:25

guarrantee that address dependency on a control node above memory's c…

967eeca

…ontrol.

Add more debug infomation

90ec471

vnkozlov reviewed Sep 2, 2021

View reviewed changes

vnkozlov approved these changes Sep 2, 2021

View reviewed changes

TobiHartmann approved these changes Sep 14, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 14, 2021

openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 14, 2021

openjdk bot closed this Sep 14, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Sep 14, 2021

chhagedorn mentioned this pull request Dec 2, 2021

8277529: SIGSEGV in C2 CompilerThread Node::rematerialize() compiling Packet::readUnsignedTrint #6670

Closed

3 tasks

JDK-8272574: C2: assert(false) failed: Bad graph detected in build_loop_late #5142

JDK-8272574: C2: assert(false) failed: Bad graph detected in build_loop_late #5142

Conversation

casparcwang commented Aug 17, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Contributors

Reviewing

bridgekeeper bot commented Aug 17, 2021

openjdk bot commented Aug 17, 2021

mlbridge bot commented Aug 17, 2021 • edited Loading

Webrevs

casparcwang commented Aug 17, 2021 • edited Loading

openjdk bot commented Aug 17, 2021

casparcwang commented Aug 17, 2021

openjdk bot commented Aug 17, 2021

dean-long commented Aug 17, 2021 • edited Loading

casparcwang commented Aug 18, 2021

casparcwang commented Aug 18, 2021

dean-long commented Aug 18, 2021

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

Choose a reason for hiding this comment

chhagedorn commented Aug 18, 2021

casparcwang commented Aug 18, 2021 • edited Loading

chhagedorn commented Aug 18, 2021

casparcwang commented Aug 19, 2021

chhagedorn commented Aug 19, 2021 • edited Loading

casparcwang commented Aug 20, 2021

openjdk bot commented Aug 20, 2021

casparcwang commented Aug 20, 2021

casparcwang commented Aug 20, 2021

casparcwang commented Aug 23, 2021

chhagedorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

casparcwang commented Aug 30, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

casparcwang commented Sep 2, 2021

vnkozlov left a comment

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

casparcwang commented Sep 3, 2021

casparcwang commented Sep 7, 2021

TobiHartmann left a comment

Choose a reason for hiding this comment

openjdk bot commented Sep 14, 2021

openjdk bot commented Sep 14, 2021 • edited Loading

casparcwang commented Sep 14, 2021

casparcwang commented Sep 14, 2021

DamonFool commented Sep 14, 2021

openjdk bot commented Sep 14, 2021

openjdk bot commented Sep 14, 2021

openjdk bot commented Sep 14, 2021

casparcwang commented Aug 17, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 17, 2021 •

edited

Loading

casparcwang commented Aug 17, 2021 •

edited

Loading

dean-long commented Aug 17, 2021 •

edited

Loading

casparcwang commented Aug 18, 2021 •

edited

Loading

chhagedorn commented Aug 19, 2021 •

edited

Loading

openjdk bot commented Sep 14, 2021 •

edited

Loading