From 4109c73a78c424d409e9fdd96913a772467666c8 Mon Sep 17 00:00:00 2001 From: Marc Chevalier Date: Mon, 3 Mar 2025 09:32:54 +0000 Subject: [PATCH] 8349523: Unused runtime calls to drem/frem should be removed Reviewed-by: thartmann, kvn, chagedorn --- src/hotspot/share/opto/compile.cpp | 3 + src/hotspot/share/opto/divnode.cpp | 40 ++++++--- src/hotspot/share/opto/divnode.hpp | 2 +- src/hotspot/share/opto/node.cpp | 21 +++++ src/hotspot/share/opto/node.hpp | 4 + src/hotspot/share/opto/phaseX.cpp | 2 + .../compiler/c2/irTests/ModDNodeTests.java | 89 ++++++++++++++++++- .../compiler/c2/irTests/ModFNodeTests.java | 89 ++++++++++++++++++- .../compiler/lib/ir_framework/IRNode.java | 20 +++++ 9 files changed, 253 insertions(+), 17 deletions(-) diff --git a/src/hotspot/share/opto/compile.cpp b/src/hotspot/share/opto/compile.cpp index 7a03ac418a8..7c9d8250e5c 100644 --- a/src/hotspot/share/opto/compile.cpp +++ b/src/hotspot/share/opto/compile.cpp @@ -436,6 +436,9 @@ void Compile::disconnect_useless_nodes(Unique_Node_List& useful, Unique_Node_Lis n->raw_del_out(j); --j; --max; + if (child->is_data_proj_of_pure_function(n)) { + worklist.push(n); + } } } if (n->outcnt() == 1 && n->has_special_unique_user()) { diff --git a/src/hotspot/share/opto/divnode.cpp b/src/hotspot/share/opto/divnode.cpp index bb66ad47ed7..90b5d7f9e27 100644 --- a/src/hotspot/share/opto/divnode.cpp +++ b/src/hotspot/share/opto/divnode.cpp @@ -1515,6 +1515,12 @@ Node* ModFNode::Ideal(PhaseGVN* phase, bool can_reshape) { if (!can_reshape) { return nullptr; } + PhaseIterGVN* igvn = phase->is_IterGVN(); + + bool result_is_unused = proj_out_or_null(TypeFunc::Parms) == nullptr; + if (result_is_unused) { + return replace_with_con(igvn, TypeF::make(0.)); + } // Either input is TOP ==> the result is TOP const Type* t1 = phase->type(dividend()); @@ -1535,10 +1541,10 @@ Node* ModFNode::Ideal(PhaseGVN* phase, bool can_reshape) { // If either is a NaN, return an input NaN if (g_isnan(f1)) { - return replace_with_con(phase, t1); + return replace_with_con(igvn, t1); } if (g_isnan(f2)) { - return replace_with_con(phase, t2); + return replace_with_con(igvn, t2); } // If an operand is infinity or the divisor is +/- zero, punt. @@ -1553,13 +1559,19 @@ Node* ModFNode::Ideal(PhaseGVN* phase, bool can_reshape) { xr ^= min_jint; } - return replace_with_con(phase, TypeF::make(jfloat_cast(xr))); + return replace_with_con(igvn, TypeF::make(jfloat_cast(xr))); } Node* ModDNode::Ideal(PhaseGVN* phase, bool can_reshape) { if (!can_reshape) { return nullptr; } + PhaseIterGVN* igvn = phase->is_IterGVN(); + + bool result_is_unused = proj_out_or_null(TypeFunc::Parms) == nullptr; + if (result_is_unused) { + return replace_with_con(igvn, TypeD::make(0.)); + } // Either input is TOP ==> the result is TOP const Type* t1 = phase->type(dividend()); @@ -1580,10 +1592,10 @@ Node* ModDNode::Ideal(PhaseGVN* phase, bool can_reshape) { // If either is a NaN, return an input NaN if (g_isnan(f1)) { - return replace_with_con(phase, t1); + return replace_with_con(igvn, t1); } if (g_isnan(f2)) { - return replace_with_con(phase, t2); + return replace_with_con(igvn, t2); } // If an operand is infinity or the divisor is +/- zero, punt. @@ -1598,33 +1610,33 @@ Node* ModDNode::Ideal(PhaseGVN* phase, bool can_reshape) { xr ^= min_jlong; } - return replace_with_con(phase, TypeD::make(jdouble_cast(xr))); + return replace_with_con(igvn, TypeD::make(jdouble_cast(xr))); } -Node* ModFloatingNode::replace_with_con(PhaseGVN* phase, const Type* con) { +Node* ModFloatingNode::replace_with_con(PhaseIterGVN* phase, const Type* con) { Compile* C = phase->C; Node* con_node = phase->makecon(con); CallProjections projs; extract_projections(&projs, false, false); - C->gvn_replace_by(projs.fallthrough_proj, in(TypeFunc::Control)); + phase->replace_node(projs.fallthrough_proj, in(TypeFunc::Control)); if (projs.fallthrough_catchproj != nullptr) { - C->gvn_replace_by(projs.fallthrough_catchproj, in(TypeFunc::Control)); + phase->replace_node(projs.fallthrough_catchproj, in(TypeFunc::Control)); } if (projs.fallthrough_memproj != nullptr) { - C->gvn_replace_by(projs.fallthrough_memproj, in(TypeFunc::Memory)); + phase->replace_node(projs.fallthrough_memproj, in(TypeFunc::Memory)); } if (projs.catchall_memproj != nullptr) { - C->gvn_replace_by(projs.catchall_memproj, C->top()); + phase->replace_node(projs.catchall_memproj, C->top()); } if (projs.fallthrough_ioproj != nullptr) { - C->gvn_replace_by(projs.fallthrough_ioproj, in(TypeFunc::I_O)); + phase->replace_node(projs.fallthrough_ioproj, in(TypeFunc::I_O)); } assert(projs.catchall_ioproj == nullptr, "no exceptions from floating mod"); assert(projs.catchall_catchproj == nullptr, "no exceptions from floating mod"); if (projs.resproj != nullptr) { - C->gvn_replace_by(projs.resproj, con_node); + phase->replace_node(projs.resproj, con_node); } - C->gvn_replace_by(this, C->top()); + phase->replace_node(this, C->top()); C->remove_macro_node(this); disconnect_inputs(C); return nullptr; diff --git a/src/hotspot/share/opto/divnode.hpp b/src/hotspot/share/opto/divnode.hpp index a926f4ab122..127e2431b0b 100644 --- a/src/hotspot/share/opto/divnode.hpp +++ b/src/hotspot/share/opto/divnode.hpp @@ -158,7 +158,7 @@ public: // Base class for float and double modulus class ModFloatingNode : public CallLeafNode { protected: - Node* replace_with_con(PhaseGVN* phase, const Type* con); + Node* replace_with_con(PhaseIterGVN* phase, const Type* con); public: ModFloatingNode(Compile* C, const TypeFunc* tf, const char *name); diff --git a/src/hotspot/share/opto/node.cpp b/src/hotspot/share/opto/node.cpp index 0e2957ac66e..d1b3d96b76b 100644 --- a/src/hotspot/share/opto/node.cpp +++ b/src/hotspot/share/opto/node.cpp @@ -1452,6 +1452,8 @@ static void kill_dead_code( Node *dead, PhaseIterGVN *igvn ) { // The restriction (outcnt() <= 2) is the same as in set_req_X() // and remove_globally_dead_node(). igvn->add_users_to_worklist( n ); + } else if (dead->is_data_proj_of_pure_function(n)) { + igvn->_worklist.push(n); } else { BarrierSet::barrier_set()->barrier_set_c2()->enqueue_useful_gc_barrier(igvn, n); } @@ -2931,6 +2933,25 @@ bool Node::is_dead_loop_safe() const { bool Node::is_div_or_mod(BasicType bt) const { return Opcode() == Op_Div(bt) || Opcode() == Op_Mod(bt) || Opcode() == Op_UDiv(bt) || Opcode() == Op_UMod(bt); } +bool Node::is_pure_function() const { + switch (Opcode()) { + case Op_ModD: + case Op_ModF: + return true; + default: + return false; + } +} + +// `maybe_pure_function` is assumed to be the input of `this`. This is a bit redundant, +// but we already have and need maybe_pure_function in all the call sites, so +// it makes it obvious that the `maybe_pure_function` is the same node as in the caller, +// while it takes more thinking to realize that a locally computed in(0) must be equal to +// the local in the caller. +bool Node::is_data_proj_of_pure_function(const Node* maybe_pure_function) const { + return Opcode() == Op_Proj && as_Proj()->_con == TypeFunc::Parms && maybe_pure_function->is_pure_function(); +} + //============================================================================= //------------------------------yank------------------------------------------- // Find and remove diff --git a/src/hotspot/share/opto/node.hpp b/src/hotspot/share/opto/node.hpp index 941b816d8e0..e89ec9455a6 100644 --- a/src/hotspot/share/opto/node.hpp +++ b/src/hotspot/share/opto/node.hpp @@ -1284,6 +1284,10 @@ public: bool is_div_or_mod(BasicType bt) const; + bool is_pure_function() const; + + bool is_data_proj_of_pure_function(const Node* maybe_pure_function) const; + //----------------- Printing, etc #ifndef PRODUCT public: diff --git a/src/hotspot/share/opto/phaseX.cpp b/src/hotspot/share/opto/phaseX.cpp index 0dd2acd8664..397b1e52e93 100644 --- a/src/hotspot/share/opto/phaseX.cpp +++ b/src/hotspot/share/opto/phaseX.cpp @@ -1342,6 +1342,8 @@ void PhaseIterGVN::remove_globally_dead_node( Node *dead ) { } assert(!(i < imax), "sanity"); } + } else if (dead->is_data_proj_of_pure_function(in)) { + _worklist.push(in); } else { BarrierSet::barrier_set()->barrier_set_c2()->enqueue_useful_gc_barrier(this, in); } diff --git a/test/hotspot/jtreg/compiler/c2/irTests/ModDNodeTests.java b/test/hotspot/jtreg/compiler/c2/irTests/ModDNodeTests.java index 4cdae04c39c..3c28c936d31 100644 --- a/test/hotspot/jtreg/compiler/c2/irTests/ModDNodeTests.java +++ b/test/hotspot/jtreg/compiler/c2/irTests/ModDNodeTests.java @@ -41,7 +41,13 @@ public class ModDNodeTests { TestFramework.run(); } - @Run(test = {"constant", "notConstant", "veryNotConstant"}) + @Run(test = {"constant", "notConstant", "veryNotConstant", + "unusedResult", + "repeatedlyUnused", + "unusedResultAfterLoopOpt1", + "unusedResultAfterLoopOpt2", + "unusedResultAfterLoopOpt3", + }) public void runMethod() { Asserts.assertEQ(constant(), q % 72.0d % 30.0d); Asserts.assertEQ(alsoConstant(), q % 31.432d); @@ -49,6 +55,11 @@ public class ModDNodeTests { Asserts.assertTrue(Double.isNaN(nanRightConstant())); Asserts.assertEQ(notConstant(37.5d), 37.5d % 32.0d); Asserts.assertEQ(veryNotConstant(531.25d, 14.5d), 531.25d % 32.0d % 14.5d); + unusedResult(1.1d, 2.2d); + repeatedlyUnused(1.1d, 2.2d); + Asserts.assertEQ(unusedResultAfterLoopOpt1(1.1d, 2.2d), 0.d); + Asserts.assertEQ(unusedResultAfterLoopOpt2(1.1d, 2.2d), 0.d); + Asserts.assertEQ(unusedResultAfterLoopOpt3(1.1d, 2.2d), 0.d); } @Test @@ -114,4 +125,80 @@ public class ModDNodeTests { public double veryNotConstant(double x, double y) { return x % 32.0d % y; } + + @Test + @IR(failOn = IRNode.MOD_D, phase = CompilePhase.ITER_GVN1) + @IR(counts = {IRNode.MOD_D, "1"}, phase = CompilePhase.AFTER_PARSING) + public void unusedResult(double x, double y) { + double unused = x % y; + } + + @Test + @IR(failOn = IRNode.MOD_D, phase = CompilePhase.ITER_GVN1) + @IR(counts = {IRNode.MOD_D, "1"}, phase = CompilePhase.AFTER_PARSING) + public void repeatedlyUnused(double x, double y) { + double unused = 1.d; + for (int i = 0; i < 100_000; i++) { + unused = x % y; + } + } + + // The difference between unusedResultAfterLoopOpt1 and unusedResultAfterLoopOpt2 + // is that they exercise a slightly different reason why the node is being removed, + // and thus a different execution path. In unusedResultAfterLoopOpt1 the modulo is + // used in the traps of the parse predicates. In unusedResultAfterLoopOpt2, it is not. + @Test + @IR(counts = {IRNode.MOD_D, "1"}, phase = CompilePhase.ITER_GVN2) + @IR(failOn = IRNode.MOD_D, phase = CompilePhase.BEFORE_MACRO_EXPANSION) + public double unusedResultAfterLoopOpt1(double x, double y) { + double unused = x % y; + + int a = 77; + int b = 0; + do { + a--; + b++; + } while (a > 0); + + if (b == 78) { // dead + return unused; + } + return 0.d; + } + + @Test + @IR(counts = {IRNode.MOD_D, "1"}, phase = CompilePhase.AFTER_CLOOPS) + @IR(failOn = IRNode.MOD_D, phase = CompilePhase.PHASEIDEALLOOP1) + public double unusedResultAfterLoopOpt2(double x, double y) { + int a = 77; + int b = 0; + do { + a--; + b++; + } while (a > 0); + + double unused = x % y; + + if (b == 78) { // dead + return unused; + } + return 0.d; + } + + @Test + @IR(counts = {IRNode.MOD_D, "2"}, phase = CompilePhase.AFTER_CLOOPS) + @IR(failOn = IRNode.MOD_D, phase = CompilePhase.PHASEIDEALLOOP1) + public double unusedResultAfterLoopOpt3(double x, double y) { + double unused = x % y; + + int a = 77; + int b = 0; + do { + a--; + b++; + } while (a > 0); + + int other = (b - 77) * (int)(x % y % 1.d); + return (double)other; + } } diff --git a/test/hotspot/jtreg/compiler/c2/irTests/ModFNodeTests.java b/test/hotspot/jtreg/compiler/c2/irTests/ModFNodeTests.java index f3adf5ca44f..1b5e14b4835 100644 --- a/test/hotspot/jtreg/compiler/c2/irTests/ModFNodeTests.java +++ b/test/hotspot/jtreg/compiler/c2/irTests/ModFNodeTests.java @@ -41,7 +41,13 @@ public class ModFNodeTests { TestFramework.run(); } - @Run(test = {"constant", "notConstant", "veryNotConstant"}) + @Run(test = {"constant", "notConstant", "veryNotConstant", + "unusedResult", + "repeatedlyUnused", + "unusedResultAfterLoopOpt1", + "unusedResultAfterLoopOpt2", + "unusedResultAfterLoopOpt3", + }) public void runMethod() { Asserts.assertEQ(constant(), q % 72.0f % 30.0f); Asserts.assertEQ(alsoConstant(), q % 31.432f); @@ -49,6 +55,11 @@ public class ModFNodeTests { Asserts.assertTrue(Float.isNaN(nanRightConstant())); Asserts.assertEQ(notConstant(37.5f), 37.5f % 32.0f); Asserts.assertEQ(veryNotConstant(531.25f, 14.5f), 531.25f % 32.0f % 14.5f); + unusedResult(1.1f, 2.2f); + repeatedlyUnused(1.1f, 2.2f); + Asserts.assertEQ(unusedResultAfterLoopOpt1(1.1f, 2.2f), 0.f); + Asserts.assertEQ(unusedResultAfterLoopOpt2(1.1f, 2.2f), 0.f); + Asserts.assertEQ(unusedResultAfterLoopOpt3(1.1f, 2.2f), 0.f); } @Test @@ -114,4 +125,80 @@ public class ModFNodeTests { public float veryNotConstant(float x, float y) { return x % 32.0f % y; } + + @Test + @IR(failOn = IRNode.MOD_F, phase = CompilePhase.ITER_GVN1) + @IR(counts = {IRNode.MOD_F, "1"}, phase = CompilePhase.AFTER_PARSING) + public void unusedResult(float x, float y) { + float unused = x % y; + } + + @Test + @IR(failOn = IRNode.MOD_F, phase = CompilePhase.ITER_GVN1) + @IR(counts = {IRNode.MOD_F, "1"}, phase = CompilePhase.AFTER_PARSING) + public void repeatedlyUnused(float x, float y) { + float unused = 1.f; + for (int i = 0; i < 100_000; i++) { + unused = x % y; + } + } + + // The difference between unusedResultAfterLoopOpt1 and unusedResultAfterLoopOpt2 + // is that they exercise a slightly different reason why the node is being removed, + // and thus a different execution path. In unusedResultAfterLoopOpt1 the modulo is + // used in the traps of the parse predicates. In unusedResultAfterLoopOpt2, it is not. + @Test + @IR(counts = {IRNode.MOD_F, "1"}, phase = CompilePhase.ITER_GVN2) + @IR(failOn = IRNode.MOD_F, phase = CompilePhase.BEFORE_MACRO_EXPANSION) + public float unusedResultAfterLoopOpt1(float x, float y) { + float unused = x % y; + + int a = 77; + int b = 0; + do { + a--; + b++; + } while (a > 0); + + if (b == 78) { // dead + return unused; + } + return 0.f; + } + + @Test + @IR(counts = {IRNode.MOD_F, "1"}, phase = CompilePhase.AFTER_CLOOPS) + @IR(failOn = IRNode.MOD_F, phase = CompilePhase.PHASEIDEALLOOP1) + public float unusedResultAfterLoopOpt2(float x, float y) { + int a = 77; + int b = 0; + do { + a--; + b++; + } while (a > 0); + + float unused = x % y; + + if (b == 78) { // dead + return unused; + } + return 0.f; + } + + @Test + @IR(counts = {IRNode.MOD_F, "2"}, phase = CompilePhase.AFTER_CLOOPS) + @IR(failOn = IRNode.MOD_F, phase = CompilePhase.PHASEIDEALLOOP1) + public float unusedResultAfterLoopOpt3(float x, float y) { + float unused = x % y; + + int a = 77; + int b = 0; + do { + a--; + b++; + } while (a > 0); + + int other = (b - 77) * (int)(x % y % 1.f); + return (float)other; + } } diff --git a/test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java b/test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java index 8f28294a986..6e373207d9c 100644 --- a/test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java +++ b/test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java @@ -2531,6 +2531,16 @@ public class IRNode { machOnlyNameRegex(X86_CMOVEL_IMM01UCF, "cmovL_imm_01UCF"); } + public static final String MOD_F = PREFIX + "MOD_F" + POSTFIX; + static { + macroNodes(MOD_F, "ModF"); + } + + public static final String MOD_D = PREFIX + "MOD_D" + POSTFIX; + static { + macroNodes(MOD_D, "ModD"); + } + /* * Utility methods to set up IR_NODE_MAPPINGS. */ @@ -2576,6 +2586,16 @@ public class IRNode { IR_NODE_MAPPINGS.put(irNode, entry); } + /** + * Apply {@code regex} on all ideal graph phases up to and including {@link CompilePhase#BEFORE_MACRO_EXPANSION}. + */ + private static void macroNodes(String irNodePlaceholder, String irNodeRegex) { + String regex = START + irNodeRegex + MID + END; + IR_NODE_MAPPINGS.put(irNodePlaceholder, new SinglePhaseRangeEntry(CompilePhase.BEFORE_MACRO_EXPANSION, regex, + CompilePhase.BEFORE_STRINGOPTS, + CompilePhase.BEFORE_MACRO_EXPANSION)); + } + private static void callOfNodes(String irNodePlaceholder, String callRegex) { String regex = START + callRegex + MID + IS_REPLACED + " " + END; IR_NODE_MAPPINGS.put(irNodePlaceholder, new RegexTypeEntry(RegexType.IDEAL_INDEPENDENT, regex));