From 6082db54f762c3c9e6293251165e1477c6ece07b Mon Sep 17 00:00:00 2001 From: rwestrel Date: Mon, 2 Dec 2024 14:33:25 +0100 Subject: [PATCH 1/3] fix --- src/hotspot/share/opto/loopTransform.cpp | 13 ++++++------- src/hotspot/share/opto/loopnode.hpp | 2 +- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/hotspot/share/opto/loopTransform.cpp b/src/hotspot/share/opto/loopTransform.cpp index f644e26bbe77f..f5661eabc501d 100644 --- a/src/hotspot/share/opto/loopTransform.cpp +++ b/src/hotspot/share/opto/loopTransform.cpp @@ -2463,7 +2463,7 @@ bool PhaseIdealLoop::is_scaled_iv_plus_extra_offset(Node* exp1, Node* offset3, N //------------------------------do_range_check--------------------------------- // Eliminate range-checks and other trip-counter vs loop-invariant tests. -void PhaseIdealLoop::do_range_check(IdealLoopTree *loop, Node_List &old_new) { +void PhaseIdealLoop::do_range_check(IdealLoopTree* loop) { #ifndef PRODUCT if (PrintOpto && VerifyLoopOptimizations) { tty->print("Range Check Elimination "); @@ -2527,7 +2527,7 @@ void PhaseIdealLoop::do_range_check(IdealLoopTree *loop, Node_List &old_new) { // have control above the pre loop, but there's no guarantee that they do. There's no guarantee either that the pre // loop limit has control that's out of loop (a previous round of range check elimination could have set a limit that's // not loop invariant). - Node* new_limit_ctrl = dominated_node(pre_ctrl, pre_limit_ctrl); + Node* new_limit_ctrl = dominated_node(pre_ctrl, pre_limit_ctrl, compute_early_ctrl(main_limit, main_limit_ctrl)); // Ensure the original loop limit is available from the // pre-loop Opaque1 node. @@ -2778,8 +2778,8 @@ void PhaseIdealLoop::do_range_check(IdealLoopTree *loop, Node_List &old_new) { // new pre_limit can push Bool/Cmp/Opaque nodes down (when one of the eliminated condition has parameters that are not // loop invariant in the pre loop. set_ctrl(pre_opaq, new_limit_ctrl); - set_ctrl(pre_end->cmp_node(), new_limit_ctrl); - set_ctrl(pre_end->in(1), new_limit_ctrl); + set_ctrl(pre_end->cmp_node(), pre_end->in(0)); + set_ctrl(pre_end->in(1), pre_end->in(0)); _igvn.replace_input_of(pre_opaq, 1, pre_limit); @@ -2822,8 +2822,7 @@ void PhaseIdealLoop::do_range_check(IdealLoopTree *loop, Node_List &old_new) { // new main_limit can push Bool/Cmp nodes down (when one of the eliminated condition has parameters that are not loop // invariant in the pre loop. set_ctrl(opqzm, new_limit_ctrl); - set_ctrl(iffm->in(1)->in(1), new_limit_ctrl); - set_ctrl(iffm->in(1), new_limit_ctrl); + assert(is_dominator(new_limit_ctrl, get_ctrl(iffm->in(1)->in(1))), "control of cmp should be below control of updated input"); C->print_method(PHASE_AFTER_RANGE_CHECK_ELIMINATION, 4, cl); } @@ -3402,7 +3401,7 @@ bool IdealLoopTree::iteration_split_impl(PhaseIdealLoop *phase, Node_List &old_n // with full checks, but the main-loop with no checks. Remove said checks // from the main body. if (should_rce) { - phase->do_range_check(this, old_new); + phase->do_range_check(this); } // Double loop body for unrolling. Adjust the minimum-trip test (will do diff --git a/src/hotspot/share/opto/loopnode.hpp b/src/hotspot/share/opto/loopnode.hpp index 698e48aadb4ef..07a5e28b23e1a 100644 --- a/src/hotspot/share/opto/loopnode.hpp +++ b/src/hotspot/share/opto/loopnode.hpp @@ -1416,7 +1416,7 @@ class PhaseIdealLoop : public PhaseTransform { } // Eliminate range-checks and other trip-counter vs loop-invariant tests. - void do_range_check(IdealLoopTree *loop, Node_List &old_new); + void do_range_check(IdealLoopTree* loop); // Clone loop with an invariant test (that does not exit) and // insert a clone of the test that selects which version to From e7904f6cb7a58d6bc96757060cffd8076e319c69 Mon Sep 17 00:00:00 2001 From: rwestrel Date: Mon, 2 Dec 2024 14:56:48 +0100 Subject: [PATCH 2/3] comment --- src/hotspot/share/opto/loopTransform.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/hotspot/share/opto/loopTransform.cpp b/src/hotspot/share/opto/loopTransform.cpp index f5661eabc501d..5fd2273c4656c 100644 --- a/src/hotspot/share/opto/loopTransform.cpp +++ b/src/hotspot/share/opto/loopTransform.cpp @@ -2526,7 +2526,8 @@ void PhaseIdealLoop::do_range_check(IdealLoopTree* loop) { // Range check elimination optimizes out conditions whose parameters are loop invariant in the main loop. They usually // have control above the pre loop, but there's no guarantee that they do. There's no guarantee either that the pre // loop limit has control that's out of loop (a previous round of range check elimination could have set a limit that's - // not loop invariant). + // not loop invariant). new_limit_ctrl is used for both the pre and main loops. Early control for the main limit may be + // below the pre loop entry and the pre limit and must be taken into account when initializing new_limit_ctrl. Node* new_limit_ctrl = dominated_node(pre_ctrl, pre_limit_ctrl, compute_early_ctrl(main_limit, main_limit_ctrl)); // Ensure the original loop limit is available from the @@ -2820,7 +2821,7 @@ void PhaseIdealLoop::do_range_check(IdealLoopTree* loop) { assert(opqzm->outcnt() == 1, "cannot hack shared node"); _igvn.replace_input_of(opqzm, 1, main_limit); // new main_limit can push Bool/Cmp nodes down (when one of the eliminated condition has parameters that are not loop - // invariant in the pre loop. + // invariant in the pre loop). set_ctrl(opqzm, new_limit_ctrl); assert(is_dominator(new_limit_ctrl, get_ctrl(iffm->in(1)->in(1))), "control of cmp should be below control of updated input"); From 0b08402d6fd082c84f86a0b21aaf5d462c9c6301 Mon Sep 17 00:00:00 2001 From: rwestrel Date: Wed, 4 Dec 2024 16:15:00 +0100 Subject: [PATCH 3/3] review --- src/hotspot/share/opto/loopTransform.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/hotspot/share/opto/loopTransform.cpp b/src/hotspot/share/opto/loopTransform.cpp index 5fd2273c4656c..6efad72491775 100644 --- a/src/hotspot/share/opto/loopTransform.cpp +++ b/src/hotspot/share/opto/loopTransform.cpp @@ -2779,6 +2779,8 @@ void PhaseIdealLoop::do_range_check(IdealLoopTree* loop) { // new pre_limit can push Bool/Cmp/Opaque nodes down (when one of the eliminated condition has parameters that are not // loop invariant in the pre loop. set_ctrl(pre_opaq, new_limit_ctrl); + // Can't use new_limit_ctrl for Bool/Cmp because it can be out of loop while they are loop variant. Conservatively set + // control to latest possible one. set_ctrl(pre_end->cmp_node(), pre_end->in(0)); set_ctrl(pre_end->in(1), pre_end->in(0)); @@ -2820,9 +2822,11 @@ void PhaseIdealLoop::do_range_check(IdealLoopTree* loop) { // The OpaqueNode is unshared by design assert(opqzm->outcnt() == 1, "cannot hack shared node"); _igvn.replace_input_of(opqzm, 1, main_limit); - // new main_limit can push Bool/Cmp nodes down (when one of the eliminated condition has parameters that are not loop - // invariant in the pre loop). + // new main_limit can push opaque node for zero trip guard down (when one of the eliminated condition has parameters + // that are not loop invariant in the pre loop). set_ctrl(opqzm, new_limit_ctrl); + // Bool/Cmp nodes for zero trip guard should have been assigned control between the main and pre loop (because zero + // trip guard depends on induction variable value out of pre loop) so shouldn't need to be adjusted assert(is_dominator(new_limit_ctrl, get_ctrl(iffm->in(1)->in(1))), "control of cmp should be below control of updated input"); C->print_method(PHASE_AFTER_RANGE_CHECK_ELIMINATION, 4, cl);