From fc18201bbdac7ac7d78767c780d3efe5352ee77a Mon Sep 17 00:00:00 2001 From: Christian Hagedorn Date: Mon, 8 Apr 2024 10:52:30 +0000 Subject: [PATCH] 8327111: Replace remaining usage of create_bool_from_template_assertion_predicate() which requires additional OpaqueLoop*Nodes transformation strategies Reviewed-by: epeter, kvn --- src/hotspot/share/opto/loopTransform.cpp | 94 +++---------------- src/hotspot/share/opto/loopnode.hpp | 2 - src/hotspot/share/opto/predicates.cpp | 59 ++++++++++++ src/hotspot/share/opto/predicates.hpp | 2 + ...stCloningWithManyDiamondsInExpression.java | 36 ++++++- 5 files changed, 107 insertions(+), 86 deletions(-) diff --git a/src/hotspot/share/opto/loopTransform.cpp b/src/hotspot/share/opto/loopTransform.cpp index 31ef298d3af..5fd98251af5 100644 --- a/src/hotspot/share/opto/loopTransform.cpp +++ b/src/hotspot/share/opto/loopTransform.cpp @@ -1488,96 +1488,26 @@ void PhaseIdealLoop::count_opaque_loop_nodes(Node* n, uint& init, uint& stride) } } -// Create a new Bool node from the provided Template Assertion Predicate. -// Unswitched loop: new_init and new_stride are both null. Clone OpaqueLoopInit and OpaqueLoopStride. -// Otherwise: Replace found OpaqueLoop* nodes with new_init and new_stride, respectively. -Node* PhaseIdealLoop::create_bool_from_template_assertion_predicate(Node* template_assertion_predicate, Node* new_init, - Node* new_stride, Node* control) { - Node_Stack to_clone(2); - Node* opaque4 = template_assertion_predicate->in(1); - assert(opaque4->Opcode() == Op_Opaque4, "must be Opaque4"); - to_clone.push(opaque4, 1); - uint current = C->unique(); - Node* result = nullptr; - bool is_unswitched_loop = new_init == nullptr && new_stride == nullptr; - assert(new_init != nullptr || is_unswitched_loop, "new_init must be set when new_stride is non-null"); - // Look for the opaque node to replace with the new value - // and clone everything in between. We keep the Opaque4 node - // so the duplicated predicates are eliminated once loop - // opts are over: they are here only to keep the IR graph - // consistent. - do { - Node* n = to_clone.node(); - uint i = to_clone.index(); - Node* m = n->in(i); - if (is_part_of_template_assertion_predicate_bool(m)) { - to_clone.push(m, 1); - continue; - } - if (m->is_Opaque1()) { - if (n->_idx < current) { - n = n->clone(); - register_new_node(n, control); - } - int op = m->Opcode(); - if (op == Op_OpaqueLoopInit) { - if (is_unswitched_loop && m->_idx < current && new_init == nullptr) { - new_init = m->clone(); - register_new_node(new_init, control); - } - n->set_req(i, new_init); - } else { - assert(op == Op_OpaqueLoopStride, "unexpected opaque node"); - if (is_unswitched_loop && m->_idx < current && new_stride == nullptr) { - new_stride = m->clone(); - register_new_node(new_stride, control); - } - if (new_stride != nullptr) { - n->set_req(i, new_stride); - } - } - to_clone.set_node(n); - } - while (true) { - Node* cur = to_clone.node(); - uint j = to_clone.index(); - if (j+1 < cur->req()) { - to_clone.set_index(j+1); - break; - } - to_clone.pop(); - if (to_clone.size() == 0) { - result = cur; - break; - } - Node* next = to_clone.node(); - j = to_clone.index(); - if (next->in(j) != cur) { - assert(cur->_idx >= current || next->in(j)->Opcode() == Op_Opaque1, "new node or Opaque1 being replaced"); - if (next->_idx < current) { - next = next->clone(); - register_new_node(next, control); - to_clone.set_node(next); - } - next->set_req(j, cur); - } - } - } while (result == nullptr); - assert(result->_idx >= current, "new node expected"); - assert(!is_unswitched_loop || new_init != nullptr, "new_init must always be found and cloned"); - return result; -} - // Clone an Assertion Predicate for the main loop. new_init and new_stride are set as new inputs. Since the predicates // cannot fail at runtime, Halt nodes are inserted instead of uncommon traps. Node* PhaseIdealLoop::clone_assertion_predicate_and_initialize(Node* iff, Node* new_init, Node* new_stride, Node* predicate, Node* uncommon_proj, Node* control, IdealLoopTree* outer_loop, Node* input_proj) { - Node* result = create_bool_from_template_assertion_predicate(iff, new_init, new_stride, control); + TemplateAssertionPredicateExpression template_assertion_predicate_expression(iff->in(1)->as_Opaque4()); + Opaque4Node* new_opaque4_node; + if (new_stride == nullptr) { + // Only set a new OpaqueLoopInitNode node and clone the existing OpaqueLoopStrideNode without modification. + // This is done when creating a new Template Assertion Predicate for the main loop which requires a new init node. + assert(new_init->is_OpaqueLoopInit(), "only for creating new Template Assertion Predicates"); + new_opaque4_node = template_assertion_predicate_expression.clone_and_replace_init(new_init, control, this); + } else { + new_opaque4_node = template_assertion_predicate_expression.clone_and_replace_init_and_stride(new_init, new_stride, + control, this); + } Node* proj = predicate->clone(); Node* other_proj = uncommon_proj->clone(); Node* new_iff = iff->clone(); - new_iff->set_req(1, result); + new_iff->set_req(1, new_opaque4_node); proj->set_req(0, new_iff); other_proj->set_req(0, new_iff); Node* frame = new ParmNode(C->start(), TypeFunc::FramePtr); diff --git a/src/hotspot/share/opto/loopnode.hpp b/src/hotspot/share/opto/loopnode.hpp index dbd7d2fdeb9..e4654f6538c 100644 --- a/src/hotspot/share/opto/loopnode.hpp +++ b/src/hotspot/share/opto/loopnode.hpp @@ -950,8 +950,6 @@ private: Node* input_proj); static void count_opaque_loop_nodes(Node* n, uint& init, uint& stride); static bool subgraph_has_opaque(Node* n); - Node* create_bool_from_template_assertion_predicate(Node* template_assertion_predicate, Node* new_init, Node* new_stride, - Node* control); static bool assertion_predicate_has_loop_opaque_node(IfNode* iff); static void get_assertion_predicates(Node* predicate, Unique_Node_List& list, bool get_opaque = false); void update_main_loop_assertion_predicates(Node* ctrl, CountedLoopNode* loop_head, Node* init, int stride_con); diff --git a/src/hotspot/share/opto/predicates.cpp b/src/hotspot/share/opto/predicates.cpp index 905254ae382..997a05dc1f2 100644 --- a/src/hotspot/share/opto/predicates.cpp +++ b/src/hotspot/share/opto/predicates.cpp @@ -170,6 +170,49 @@ class CloneStrategy : public TransformStrategyForOpaqueLoopNodes { } }; +// This strategy replaces the OpaqueLoopInitNode with the provided init node and clones the OpaqueLoopStrideNode. +class ReplaceInitAndCloneStrideStrategy : public TransformStrategyForOpaqueLoopNodes { + Node* const _new_init; + Node* const _new_ctrl; + PhaseIdealLoop* const _phase; + + public: + ReplaceInitAndCloneStrideStrategy(Node* new_init, Node* new_ctrl, PhaseIdealLoop* phase) + : _new_init(new_init), + _new_ctrl(new_ctrl), + _phase(phase) {} + NONCOPYABLE(ReplaceInitAndCloneStrideStrategy); + + Node* transform_opaque_init(OpaqueLoopInitNode* opaque_init) const override { + return _new_init; + } + + Node* transform_opaque_stride(OpaqueLoopStrideNode* opaque_stride) const override { + return _phase->clone_and_register(opaque_stride, _new_ctrl)->as_OpaqueLoopStride(); + } +}; + +// This strategy replaces the OpaqueLoopInit and OpaqueLoopStride nodes with the provided init and stride nodes, +// respectively. +class ReplaceInitAndStrideStrategy : public TransformStrategyForOpaqueLoopNodes { + Node* const _new_init; + Node* const _new_stride; + + public: + ReplaceInitAndStrideStrategy(Node* new_init, Node* new_stride) + : _new_init(new_init), + _new_stride(new_stride) {} + NONCOPYABLE(ReplaceInitAndStrideStrategy); + + Node* transform_opaque_init(OpaqueLoopInitNode* opaque_init) const override { + return _new_init; + } + + Node* transform_opaque_stride(OpaqueLoopStrideNode* opaque_stride) const override { + return _new_stride; + } +}; + // Creates an identical clone of this Template Assertion Predicate Expression (i.e.cloning all nodes from the Opaque4Node // to and including the OpaqueLoop* nodes). The cloned nodes are rewired to reflect the same graph structure as found for // this Template Assertion Predicate Expression. The cloned nodes get 'new_ctrl' as ctrl. There is no other update done @@ -179,6 +222,22 @@ Opaque4Node* TemplateAssertionPredicateExpression::clone(Node* new_ctrl, PhaseId return clone(clone_init_and_stride_strategy, new_ctrl, phase); } +// Same as clone() but instead of cloning the OpaqueLoopInitNode, we replace it with the provided 'new_init' node. +Opaque4Node* TemplateAssertionPredicateExpression::clone_and_replace_init(Node* new_init, Node* new_ctrl, + PhaseIdealLoop* phase) { + ReplaceInitAndCloneStrideStrategy replace_init_and_clone_stride_strategy(new_init, new_ctrl, phase); + return clone(replace_init_and_clone_stride_strategy, new_ctrl, phase); +} + +// Same as clone() but instead of cloning the OpaqueLoopInit and OpaqueLoopStride node, we replace them with the provided +// 'new_init' and 'new_stride' nodes, respectively. +Opaque4Node* TemplateAssertionPredicateExpression::clone_and_replace_init_and_stride(Node* new_init, Node* new_stride, + Node* new_ctrl, + PhaseIdealLoop* phase) { + ReplaceInitAndStrideStrategy replace_init_and_stride_strategy(new_init, new_stride); + return clone(replace_init_and_stride_strategy, new_ctrl, phase); +} + // Class to collect data nodes from a source to target nodes by following the inputs of the source node recursively. // The class takes a node filter to decide which input nodes to follow and a target node predicate to start backtracking // from. All nodes found on all paths from source->target(s) are returned in a Unique_Node_List (without duplicates). diff --git a/src/hotspot/share/opto/predicates.hpp b/src/hotspot/share/opto/predicates.hpp index 37c6510c65f..a794e31ac3b 100644 --- a/src/hotspot/share/opto/predicates.hpp +++ b/src/hotspot/share/opto/predicates.hpp @@ -307,6 +307,8 @@ class TemplateAssertionPredicateExpression : public StackObj { } Opaque4Node* clone(Node* new_ctrl, PhaseIdealLoop* phase); + Opaque4Node* clone_and_replace_init(Node* new_init, Node* new_ctrl,PhaseIdealLoop* phase); + Opaque4Node* clone_and_replace_init_and_stride(Node* new_init, Node* new_stride, Node* new_ctrl, PhaseIdealLoop* phase); }; // This class represents a Predicate Block (i.e. either a Loop Predicate Block, a Profiled Loop Predicate Block, diff --git a/test/hotspot/jtreg/compiler/predicates/TestCloningWithManyDiamondsInExpression.java b/test/hotspot/jtreg/compiler/predicates/TestCloningWithManyDiamondsInExpression.java index 06af33f9a8e..9efe3ea85b4 100644 --- a/test/hotspot/jtreg/compiler/predicates/TestCloningWithManyDiamondsInExpression.java +++ b/test/hotspot/jtreg/compiler/predicates/TestCloningWithManyDiamondsInExpression.java @@ -24,7 +24,7 @@ /* * @test - * @bug 8327110 + * @bug 8327110 8327111 * @requires vm.compiler2.enabled * @summary Test that DFS algorithm for cloning Template Assertion Predicate Expression does not endlessly process paths. * @run main/othervm/timeout=30 -Xcomp -XX:LoopMaxUnroll=0 @@ -35,9 +35,23 @@ * -XX:CompileCommand=compileonly,*TestCloningWithManyDiamondsInExpression::test* * -XX:CompileCommand=inline,*TestCloningWithManyDiamondsInExpression::create* * compiler.predicates.TestCloningWithManyDiamondsInExpression - * @run main compiler.predicates.TestCloningWithManyDiamondsInExpression + * @run main/timeout=30 compiler.predicates.TestCloningWithManyDiamondsInExpression */ + /* + * @test + * @bug 8327111 + * @summary Test that DFS algorithm for cloning Template Assertion Predicate Expression does not endlessly process paths. + * @run main/othervm/timeout=30 -Xcomp + * -XX:CompileCommand=compileonly,*TestCloningWithManyDiamondsInExpression::test* + * -XX:CompileCommand=inline,*TestCloningWithManyDiamondsInExpression::create* + * compiler.predicates.TestCloningWithManyDiamondsInExpression + * @run main/othervm/timeout=30 -Xbatch + * -XX:CompileCommand=compileonly,*TestCloningWithManyDiamondsInExpression::test* + * -XX:CompileCommand=inline,*TestCloningWithManyDiamondsInExpression::create* + * compiler.predicates.TestCloningWithManyDiamondsInExpression + */ + package compiler.predicates; public class TestCloningWithManyDiamondsInExpression { @@ -51,6 +65,8 @@ public class TestCloningWithManyDiamondsInExpression { for (int i = 0; i < 10_000; i++) { testSplitIf(i % 2); testLoopUnswitching(i % 2); + testLoopUnrolling(i % 2); + testLoopPeeling(i % 2); } } @@ -98,6 +114,22 @@ public class TestCloningWithManyDiamondsInExpression { } } + static void testLoopUnrolling(int x) { + int[] a = new int[createExpressionWithManyDiamonds(x) + 1000]; + for (int i = 0; i < limit; i++) { + a[i] = i; // Loop Predication hoists this check and creates a Template Assertion Predicate. + } + } + + static void testLoopPeeling(int x) { + int[] a = new int[createExpressionWithManyDiamonds(x) + 1000]; + for (int i = 0; i < limit; i++) { + a[i] = i; // Loop Predication hoists this check and creates a Template Assertion Predicate. + if (x == 0) { // Reason to peel with LoopMaxUnroll=0 + return; + } + } + } // Creates in int expression with many diamonds. This method is forced-inlined. static int createExpressionWithManyDiamonds(int x) {