Skip to content

8367967: C2: "fatal error: Not monotonic" with Mod nodes #27408

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

8367967: C2: "fatal error: Not monotonic" with Mod nodes #27408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d143d8b
test
SirYwell Sep 19, 2025
9ba78d4
move up div by zero check
SirYwell Sep 19, 2025
ade824e
add a second @run
SirYwell Sep 22, 2025
0193749
move test
SirYwell Sep 23, 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
10 changes: 5 additions & 5 deletions src/hotspot/share/opto/divnode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1206,6 +1206,11 @@ static const Type* mod_value(const PhaseGVN* phase, const Node* in1, const Node*
if (t1 == Type::TOP) { return Type::TOP; }
if (t2 == Type::TOP) { return Type::TOP; }

// Mod by zero? Throw exception at runtime!
Copy link
Contributor

@iwanowww iwanowww Sep 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The comment is a bit confusing. It's not the node itself which produces the exception, but a dominating zero check (inserted during parsing). So, if a divisor becomes 0, it means the node is effectively dead and can go away.

Also, the node should go away anyway as part of CFG pruning of dead branches when corresponding guard goes away.

BTW if there are cases when control is not eliminated, it may irrevocably break the IR causing crashes down the road (take a look at JDK-8154831 as an example). So, maybe it's safer to just rely on dead control pruning to eliminate effectively dead ModI/ModL nodes and assert that there are no effectively dead ModI/ModL nodes present after GVN pass is over.

Copy link
Member Author

@SirYwell SirYwell Sep 23, 2025

Choose a reason for hiding this comment

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

The comment comes from the original code before my change in #25254, where that path also returned POS but that wasn't monotonic with my changes anymore.

So, if a divisor becomes 0, it means the node is effectively dead and can go away.

I think this check mostly comes down to CCP. We need to return something for a zero divisor, and that something has to be monotonic with subsequent wider inputs.

If you agree with that observation, I can change the comment to better reflect what's going on, e.g., Mod by zero can be observed in PhaseCCP, return TOP to ensure monotonic results (I'm open for other suggestions).

Copy link
Contributor

@iwanowww iwanowww Sep 25, 2025

Choose a reason for hiding this comment

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

Thanks for the clarifications. I thought about it for some time, but as things work now, I don't see a better alternative except just ignoring 0 divisor case. So, please, proceed with the fix as it is now.

Alternatively, to improve robustness, a dead ModI/ModL can kill dependent control akin to what Roland did for Type nodes with JDK-8349479.

Copy link
Member

@chhagedorn chhagedorn Sep 26, 2025

Choose a reason for hiding this comment

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

I don't see a better alternative except just ignoring 0 divisor case

That probably also works. It seems that for DivI/L, we already ignore this case as well.

The question is: What is better when the zero check is not folded but we observe zero for the divisor: Having top to possibly corrupt the graph or just possibly risking miscompilation/div by zero crashes at runtime when the zero check is really off - but not folding the zero check does not necessarily mean it's wrong at runtime. The former is probably easy to catch when it happens while the latter seems more robost but when the zero check is off, it's probably harder to detect/trace back.

Alternatively, to improve robustness, a dead ModI/ModL can kill dependent control akin to what Roland did for Type nodes with JDK-8349479.

Could be an option. We then should probably also extend it to Div nodes. Might be worth to investigate separately.

if (t2 == TypeInteger::zero(bt)) {
return Type::TOP;
}

// We always generate the dynamic check for 0.
// 0 MOD X is 0
if (t1 == TypeInteger::zero(bt)) { return t1; }
Expand All @@ -1215,11 +1220,6 @@ static const Type* mod_value(const PhaseGVN* phase, const Node* in1, const Node*
return TypeInteger::zero(bt);
}

// Mod by zero? Throw exception at runtime!
if (t2 == TypeInteger::zero(bt)) {
return Type::TOP;
}

const TypeInteger* i1 = t1->is_integer(bt);
const TypeInteger* i2 = t2->is_integer(bt);
if (i1->is_con() && i2->is_con()) {
Expand Down
66 changes: 66 additions & 0 deletions test/hotspot/jtreg/compiler/ccp/TestModValueMonotonic.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
Copy link
Member

@chhagedorn chhagedorn Sep 23, 2025

Choose a reason for hiding this comment

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

Another thing: You could move this test to compiler/ccp which fits better than the generic c2 folder.

Copy link
Member Author

@SirYwell SirYwell Sep 23, 2025

Choose a reason for hiding this comment

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

Done!

* 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 8367967
* @summary Ensure ModI/LNode::Value is monotonic with potential division by 0
* @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:CompileOnly=compiler.ccp.TestModValueMonotonic::test*
* -XX:+StressCCP -XX:RepeatCompilation=100 -Xcomp compiler.ccp.TestModValueMonotonic
* @run main compiler.ccp.TestModValueMonotonic
*/
package compiler.ccp;

public class TestModValueMonotonic {
static int iFld;
static long lFld;
static int limit = 1000;
static boolean flag;

public static void main(String[] args) {
testInt();
testLong();
}

static void testInt() {
int zero = 0;

// Make sure loop is not counted such that it is not removed. Created a more complex graph for CCP.
for (int i = 1; i < limit; i*=4) {
zero = 34;
}
int three = flag ? 0 : 3;
iFld = three % zero; // phi[0..3] % phi[0..34]
}

static void testLong() {
long zero = 0;

// Make sure loop is not counted such that it is not removed. Created a more complex graph for CCP.
for (int i = 1; i < limit; i*=4) {
zero = 34;
}
long three = flag ? 0 : 3;
lFld = three % zero; // phi[0..3] % phi[0..34]
}
}