Skip to content

8348572: C2 compilation asserts due to unexpected irreducible loop #23363

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.

Sign up for GitHub

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 5 commits into from
Closed

8348572: C2 compilation asserts due to unexpected irreducible loop #23363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9a4b906
JDK-8348572
eme64 Jan 30, 2025
8003c38
add the test
eme64 Jan 30, 2025
d7b2f3b
Merge branch 'master' into JDK-8348572-bailout-for-new-irreducible-loops
eme64 Jan 30, 2025
352ebb9
update for Vladimir
eme64 Jan 31, 2025
209360d
Update test/hotspot/jtreg/compiler/loopopts/TestSplitIfNewIrreducible…
eme64 Feb 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/hotspot/share/opto/c2_globals.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,9 @@
develop(bool, VerifyLoopOptimizations, false, \
"verify major loop optimizations") \
\
develop(bool, VerifyNoNewIrreducibleLoops, false, \
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you plan to add it to our stress testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cannot currently do that:

With the new debug flag -XX:+VerifyNoNewIrreducibleLoops, we still hit the assert, as expected:

And we can do this later:

For now, have the assert behind a Verify flag, so that JDK-8348570 is unblocked. Later, we can remove the Verify flag and alway enable the assert again.

Does that sound ok to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. My question was about your future plan when "everything" is fixed.

"Verify that no new irreducible loops are created after parsing") \
\
product(bool, ProfileDynamicTypes, true, DIAGNOSTIC, \
"do extra type profiling and use it more aggressively") \
\
Expand Down
13 changes: 8 additions & 5 deletions src/hotspot/share/opto/cfgnode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,15 +436,18 @@ bool RegionNode::are_all_nodes_in_infinite_subgraph(Unique_Node_List& worklist)

void RegionNode::set_loop_status(RegionNode::LoopStatus status) {
assert(loop_status() == RegionNode::LoopStatus::NeverIrreducibleEntry, "why set our status again?");
assert(status != RegionNode::LoopStatus::MaybeIrreducibleEntry || !is_Loop(), "LoopNode is never irreducible entry.");
_loop_status = status;
}

#ifdef ASSERT
void RegionNode::verify_can_be_irreducible_entry() const {
assert(loop_status() == RegionNode::LoopStatus::MaybeIrreducibleEntry, "must be marked irreducible");
assert(!is_Loop(), "LoopNode cannot be irreducible loop entry");
// A Region can only be an irreducible entry if:
// - It is marked as "maybe irreducible entry". Any other loop status would guarantee
// that it is never an irreducible loop entry.
// - And it is not a LoopNode, those are guaranteed to be reducible loop entries.
bool RegionNode::can_be_irreducible_entry() const {
return loop_status() == RegionNode::LoopStatus::MaybeIrreducibleEntry &&
!is_Loop();
}
#endif //ASSERT

void RegionNode::try_clean_mem_phis(PhaseIterGVN* igvn) {
// Incremental inlining + PhaseStringOpts sometimes produce:
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/opto/cfgnode.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ class RegionNode : public Node {
#endif //ASSERT
LoopStatus loop_status() const { return _loop_status; };
void set_loop_status(LoopStatus status);
DEBUG_ONLY(void verify_can_be_irreducible_entry() const;)
bool can_be_irreducible_entry() const;

virtual int Opcode() const;
virtual uint size_of() const { return sizeof(*this); }
Expand Down
13 changes: 11 additions & 2 deletions src/hotspot/share/opto/loopnode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5631,14 +5631,23 @@ int PhaseIdealLoop::build_loop_tree_impl(Node* n, int pre_order) {
// l is irreducible: we just found a second entry m.
_has_irreducible_loops = true;
RegionNode* secondary_entry = m->as_Region();
DEBUG_ONLY(secondary_entry->verify_can_be_irreducible_entry();)

if (!secondary_entry->can_be_irreducible_entry()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo? Why negation?

Copy link
Contributor Author

@eme64 eme64 Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If a secondary entry can not be a secondary (irreducible) entry, then we have a problem, so we do a bailout.

Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not completely.

The issue is we should avoid creating new irreducible loops.
What if l is already marked as irreducible l->_irreducible = 1 by following code? I don't see check above for such case.
And again come here but secondary_entry can be irreducible (it is Region for example or already marked) and we skip bailout. Why it is okay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is we should avoid creating new irreducible loops.

Absolutely. This is not a fix at all. But since we can catch that the state is wrong here, we should bail out instead of continuing on in production. That is at least a little improvement. The bug-fix would come in a second step, and may be much more complicated as it would have to reconsider what to do about split_if. Do we postpone until after loop-opts for example? Before considering such an involved fix, I would rather want to do this "defensive" patch first. Does that make sense? This also allows us to integrate JDK-8348570, which is currently blocked by the failing assert (which is now disabled, but bailout instead, can be enabled with the flag).

What if l is already marked as irreducible l->_irreducible = 1 by following code? I don't see check above for such case.

If l->_irreducible = 1, then the corresponding node should have been marked as MaybeIrreducibleEntry, and so should also m be marked with MaybeIrreducibleEntry. If that is the case, we are fine, these Region were created at parsing and we currently consider that fine.

But if one of the Region of the now irreducible loop was not marked accordingly already during parsing, then a new irreducible loop appeared during compilation - and that's not good.

And again come here but secondary_entry can be irreducible (it is Region for example or already marked) and we skip bailout. Why it is okay?

If we marked a Region with MaybeIrreducibleEntry, then we treat it differently in some optimizations. For example, when the region loses a control input, we have to check if the loop is now dead, with a global connectivity search. For LoopNode losing the control input means we already know that the loop is dead, since that was the only loop entry. But for irreducible loops, losing one entry means we do not know if there is still a secondary entry or not. For context see JDK-8280126.

Does that answer your question? If not, we may have to talk about it offline ;)

PS:
The whole irreducible loop handling is still broken actually, see JDK-8308675. We plan to eventually also disallow creation of irreducible loops at parsing. But that's a big project, and we were hoping for a student project. But we still also should not introduce new irreducible loops during parsing, and that's what we are dealing with here in this bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

First, I am fine with this "band-aid" change. I understand that it simple replaces assert with bailout which is fine.
But I am trying to understand what it does.

There are few states when we come to this part of code:

  • l is or not marked as irreducible
  • m is or not marked with MaybeIrreducibleEntry (is it set only for not Loop?)
  • m is or not Loop

So we have 8 combinations. I would like to hear reasons in which cases we should bailout and in which not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the existing code is not exactly pretty or straight forward 😅

Let me explain what we know when we get here, with m as secondary_entry:

m: the CFG node we are currently looking at in the DFS.

We know that m is located inside the IdealLoopTree* l.

But now we just found out that l is already postvisited, i.e. that we already walked all CFG nodes in l before, and exited back through its original loop entry node.

That means the traversal has left the l loop structure, and has now found a second way into that loop structure l at m. Hence, we know that m must be a secondary entry to l.

We now know that l must be irreducible, so we set l->_irreducible = 1.

This is the LoopStatus definition:

  enum LoopStatus {
    // No guarantee: the region may be an irreducible loop entry, thus we have to
    // be careful when removing entry control to it.
    MaybeIrreducibleEntry,
    // Limited guarantee: this region may be (nested) inside an irreducible loop,
    // but it will never be an irreducible loop entry.
    NeverIrreducibleEntry,
    // Strong guarantee: this region is not (nested) inside an irreducible loop.
    Reducible,
  };

Here about the combinations:

  • I don't think that it matters if l is already marked irreducible or not. For example if m is marked with NeverIrreducibleEntry, then l could be irreducible, i.e. have multiple entries. But m is not allowed to be one of those.
  • For m we have 3 cases:
    • MaybeIrreducibleEntry: everything is ok, we expect that m could be a secondary entry to a loop.
    • NeverIrreducibleEntry: it would be ok if l is irreducible, but that should not happen through m as a secondary entry. An assumption was violated - the graph state is incoherent.
    • Reducible: Here we do not expect l to be irreducible, and m should not be a secondary entry either. An assumption was violated - the graph state is incoherent.

That is already sufficient to justify the bailout here.

But let me consider if m is a Loop or just Region anyway.

What if m is:

  • NOT LoopNode, just Region: we just found a new edge entering l. This edge was not reachable from inside the loop structure l, and so it must be a secondary entry.
  • LoopNode, it has an entry in(1) and a backedge in(2):
    • Assume we just came via the backedge: that is contradictory, as the backedge should be reachable from inside the loop, and hence we should already have visited that edge before declaring l postvisited.
    • Assume we came via the entry: I suppose that looks ok at first....

But we only turn Region into Loop if we are sure it is not in an irreducible loop, so I think the assumption is that a LoopNode only has reducible CFG inside its loop structure. Look at beautify_loops:

After we know that the Region only has 2 entries, and the l is reducible:

  } else if (!_head->is_Loop() && !_irreducible) {
    // Make a new LoopNode to replace the old loop head
    Node *l = new LoopNode( _head->in(1), _head->in(2) );

Hence, if we found any secondary entry into a LoopNode* m, that would also be a contradiction.
I could be wrong about this, and again, I think it does not matter.
All that matters is that we already have a contradiction if m is not marked with MaybeIrreducibleEntry, as we saw above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vnkozlov Does that help you, or do you have more questions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, thank you for explaining this to me. I think I got it finally.
So default Region state is NeverIrreducibleEntry and it set to MaybeIrreducibleEntry when we find it inside irreducible loop during parsing. And that is the only valid state (MaybeIrreducibleEntry) we allow in this part of code.
Yes, Loop node can't be irreducible and we should not allow it here too.

assert(!VerifyNoNewIrreducibleLoops, "A new irreducible loop was created after parsing.");
C->record_method_not_compilable("A new irreducible loop was created after parsing.");
return pre_order;
}

// Walk up the loop-tree, mark all loops that are already post-visited as irreducible
// Since m is a secondary entry to them all.
while( is_postvisited(l->_head) ) {
l->_irreducible = 1; // = true
RegionNode* head = l->_head->as_Region();
DEBUG_ONLY(head->verify_can_be_irreducible_entry();)
if (!head->can_be_irreducible_entry()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same issue here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same explanation as above :)

assert(!VerifyNoNewIrreducibleLoops, "A new irreducible loop was created after parsing.");
C->record_method_not_compilable("A new irreducible loop was created after parsing.");
Copy link
Member

@TobiHartmann TobiHartmann Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If you haven't done that yet, I would suggest to hardcode these bailouts to "always bail" out and run testing to check if the bailout always works. You'll of course get all kinds of test failures but the VM should not crash/assert (you can filter for these in the test results and ignore anything else).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed offline:
There is another bailout below, for the case of irreducible loops that are also infinite. And that one also triggers regularly, and as far as I remember even with the fuzzer. So I'd say he bailout path is sufficiently covered.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, makes sense. Thanks for checking.

return pre_order;
}
l = l->_parent;
// Check for bad CFG here to prevent crash, and bailout of compile
if (l == nullptr) {
Expand Down
91 changes: 91 additions & 0 deletions test/hotspot/jtreg/compiler/loopopts/TestSplitIfNewIrreducibleLoop.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/*
* Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

/*
* @test
* @bug 8348572
* @summary Tests where new irreducible loop is introduced by split_if.
*
* @run driver compiler.loopopts.TestSplitIfNewIrreducibleLoop
*
* @run main/othervm -Xcomp -XX:PerMethodTrapLimit=0
* -XX:CompileCommand=compileonly,compiler.loopopts.TestSplitIfNewIrreducibleLoop::test
* compiler.loopopts.TestSplitIfNewIrreducibleLoop
*/

package compiler.loopopts;

public class TestSplitIfNewIrreducibleLoop {

static class A {
public A parent;
}

static class B extends A {}

public static void main(String[] args) {
// Instantiate one each: classes are loaded.
A a = new A();
B b = new B();
test(b);
}

static int test(A parent) {
do {
if (parent instanceof B b) { return 1; }
if (parent != null) { parent = parent.parent; }
if (parent == null) { return 0; }
} while (true);
}

// Before split_if it looks like this (the instanceof check has already been partial peeled):
//
// if (parent instanceof B b) { return 1; }
// do {
// if (parent != null) { parent = parent.parent; }
// if (parent == null) { return 0; }
// if (parent instanceof B b) { return 1; }
// } while (true);
//
//
// Now, we want to split_if the first if in the loop body, like this:
//
// if (parent instanceof B b) { return 1; }
// if (parent != null) { goto LOOP2; } else { goto LOOP1; }
// do {
// :LOOP1
// parent = parent.parent;
// :LOOP2
// if (parent == null) { return 0; }
// if (parent instanceof B b) { return 1; }
// if (parent != null) { goto LOOP2; } else { goto LOOP1; }
// } while (true);
//
// As the comment in ifnode.cpp / split_if says: we know that on the backedge
// "parent" cannot be null, and so we would be able to split the if through
// the region, and on the backedge it would constant fold away, and we would
// only have to check the split if in the loop entry.
//
// Problem: we have introduced an irreducible loop!
}