Review Emanuel first part

src/hotspot/share/opto/loopPredicate.cpp

@@ -365,7 +365,7 @@ void PhaseIdealLoop::get_assertion_predicates(Node* predicate, Unique_Node_List&
 // Clone an Assertion Predicate for an unswitched loop. OpaqueLoopInit and OpaqueLoopStride nodes are cloned and uncommon
 // traps are kept for the predicate (a Halt node is used later when creating pre/main/post loops and copying this cloned
 // predicate again).
-IfProjNode* PhaseIdealLoop::clone_assertion_predicate_for_unswitched_loops(Node* template_assertion_predicate,
+IfProjNode* PhaseIdealLoop::clone_assertion_predicate_for_unswitched_loops(IfNode* template_assertion_predicate,
                                                                            IfProjNode* predicate,
                                                                            Deoptimization::DeoptReason reason,
                                                                            ParsePredicateSuccessProj* parse_predicate_proj) {

src/hotspot/share/opto/loopnode.hpp

@@ -28,6 +28,7 @@
 #include "opto/cfgnode.hpp"
 #include "opto/multnode.hpp"
 #include "opto/phaseX.hpp"
+#include "opto/predicates.hpp"
 #include "opto/subnode.hpp"
 #include "opto/type.hpp"
 #include "utilities/checkedCast.hpp"
@@ -1659,7 +1660,7 @@ class PhaseIdealLoop : public PhaseTransform {
                                                      Deoptimization::DeoptReason reason, IfProjNode* old_predicate_proj,
                                                      ParsePredicateSuccessProj* fast_loop_parse_predicate_proj,
                                                      ParsePredicateSuccessProj* slow_loop_parse_predicate_proj);
-  IfProjNode* clone_assertion_predicate_for_unswitched_loops(Node* template_assertion_predicate, IfProjNode* predicate,
+  IfProjNode* clone_assertion_predicate_for_unswitched_loops(IfNode* template_assertion_predicate, IfProjNode* predicate,
                                                              Deoptimization::DeoptReason reason,
                                                              ParsePredicateSuccessProj* parse_predicate_proj);
   static void check_cloned_parse_predicate_for_unswitching(const Node* new_entry, bool is_fast_loop) PRODUCT_RETURN;
@@ -1886,13 +1887,6 @@ class PathFrequency {
   float to(Node* n);
 };
 
-// Interface to transform OpaqueLoopInit and OpaqueLoopStride nodes of a Template Assertion Predicate Expression.
-class TransformStrategyForOpaqueLoopNodes : public StackObj {
- public:
-  virtual Node* transform_opaque_init(OpaqueLoopInitNode* opaque_init) = 0;
-  virtual Node* transform_opaque_stride(OpaqueLoopStrideNode* opaque_stride) = 0;
-};
-
 // Class to clone a data node graph by taking a list of data nodes. This is done in 2 steps:
 //   1. Clone the data nodes
 //   2. Fix the cloned data inputs pointing to the old nodes to the cloned inputs by using an old->new mapping.
@@ -1919,10 +1913,10 @@ class DataNodeGraph : public StackObj {
  private:
   void clone(Node* node, Node* new_ctrl);
   void clone_data_nodes(Node* new_ctrl);
-  void clone_data_nodes_and_transform_opaque_loop_nodes(TransformStrategyForOpaqueLoopNodes& transform_strategy,
+  void clone_data_nodes_and_transform_opaque_loop_nodes(const TransformStrategyForOpaqueLoopNodes& transform_strategy,
                                                         Node* new_ctrl);
   void rewire_clones_to_cloned_inputs();
-  void transform_opaque_node(TransformStrategyForOpaqueLoopNodes& transform_strategy, Node* node);
+  void transform_opaque_node(const TransformStrategyForOpaqueLoopNodes& transform_strategy, Node* node);
 
  public:
   // Clone the provided data node collection and rewire the clones in such a way to create an identical graph copy.
@@ -1934,13 +1928,14 @@ class DataNodeGraph : public StackObj {
     return _orig_to_new;
   }
 
-  // Create a copy of the provided data node collection by doing the following:
+  // Create a copy of the data nodes provided to the constructor by doing the following:
   // Clone all non-OpaqueLoop* nodes and rewire them to create an identical subgraph copy. For the OpaqueLoop* nodes,
   // apply the provided transformation strategy and include the transformed node into the subgraph copy to get a complete
   // "cloned-and-transformed" graph copy. For all newly cloned nodes (which could also be new OpaqueLoop* nodes), set
   // `new_ctrl` as ctrl.
-  const OrigToNewHashtable& clone_with_opaque_loop_transform_strategy(TransformStrategyForOpaqueLoopNodes& transform_strategy,
-                                                                      Node* new_ctrl) {
+  const OrigToNewHashtable& clone_with_opaque_loop_transform_strategy(
+      const TransformStrategyForOpaqueLoopNodes& transform_strategy,
+      Node* new_ctrl) {
     clone_data_nodes_and_transform_opaque_loop_nodes(transform_strategy, new_ctrl);
     rewire_clones_to_cloned_inputs();
     return _orig_to_new;

src/hotspot/share/opto/loopopts.cpp

@@ -4526,8 +4526,9 @@ void DataNodeGraph::rewire_clones_to_cloned_inputs() {
 
 // Clone all non-OpaqueLoop* nodes and apply the provided transformation strategy for OpaqueLoop* nodes.
 // Set 'new_ctrl' as ctrl for all cloned non-OpaqueLoop* nodes.
-void DataNodeGraph::clone_data_nodes_and_transform_opaque_loop_nodes(TransformStrategyForOpaqueLoopNodes& transform_strategy,
-                                                                     Node* new_ctrl) {
+void DataNodeGraph::clone_data_nodes_and_transform_opaque_loop_nodes(
+    const TransformStrategyForOpaqueLoopNodes& transform_strategy,
+    Node* new_ctrl) {
   for (uint i = 0; i < _data_nodes.size(); i++) {
     Node* data_node = _data_nodes[i];
     if (data_node->is_Opaque1()) {
@@ -4538,8 +4539,7 @@ void DataNodeGraph::clone_data_nodes_and_transform_opaque_loop_nodes(TransformSt
   }
 }
 
-void DataNodeGraph::transform_opaque_node(TransformStrategyForOpaqueLoopNodes& transform_strategy, Node* node) {
-  const uint next_idx = _phase->C->unique();
+void DataNodeGraph::transform_opaque_node(const TransformStrategyForOpaqueLoopNodes& transform_strategy, Node* node) {
   Node* transformed_node;
   if (node->is_OpaqueLoopInit()) {
     transformed_node = transform_strategy.transform_opaque_init(node->as_OpaqueLoopInit());

src/hotspot/share/opto/predicates.cpp

@@ -161,11 +161,11 @@ class CloneStrategy : public TransformStrategyForOpaqueLoopNodes {
         _new_ctrl(new_ctrl) {}
   NONCOPYABLE(CloneStrategy);
 
-  Node* transform_opaque_init(OpaqueLoopInitNode* opaque_init) override {
+  Node* transform_opaque_init(OpaqueLoopInitNode* opaque_init) const override {
     return _phase->clone_and_register(opaque_init, _new_ctrl)->as_OpaqueLoopInit();
   }
 
-  Node* transform_opaque_stride(OpaqueLoopStrideNode* opaque_stride) override {
+  Node* transform_opaque_stride(OpaqueLoopStrideNode* opaque_stride) const override {
     return _phase->clone_and_register(opaque_stride, _new_ctrl)->as_OpaqueLoopStride();
   }
 };
@@ -182,21 +182,24 @@ Opaque4Node* TemplateAssertionPredicateExpression::clone(Node* new_ctrl, PhaseId
 // 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) returned in a Unique_Node_List (without duplicates).
-class DataNodesOnPathToTargets : public StackObj {
+class DataNodesOnPathsToTargets : public StackObj {
   typedef bool (*NodeCheck)(const Node*);
 
-  NodeCheck _node_filter; // Node filter function to decide if we should process a node or not while searching for targets.
-  NodeCheck _is_target_node; // Function to decide if a node is a target node (i.e. where we should start backtracking).
+  // Node filter function to decide if we should process a node or not while searching for targets.
+  NodeCheck _node_filter;
+  // Function to decide if a node is a target node (i.e. where we should start backtracking). This check should also
+  // trivially pass the _node_filter.
+  NodeCheck _is_target_node;
   Node_Stack _stack; // Stack that stores entries of the form: [Node* node, int next_unvisited_input_index_of_node].
   VectorSet _visited; // Ensure that we are not visiting a node twice in the DFS walk which could happen with diamonds.
   Unique_Node_List _collected_nodes; // The resulting node collection of all nodes on paths from source->target(s).
 
  public:
-  DataNodesOnPathToTargets(NodeCheck node_filter, NodeCheck is_target_node)
+  DataNodesOnPathsToTargets(NodeCheck node_filter, NodeCheck is_target_node)
       : _node_filter(node_filter),
         _is_target_node(is_target_node),
         _stack(2) {}
-  NONCOPYABLE(DataNodesOnPathToTargets);
+  NONCOPYABLE(DataNodesOnPathsToTargets);
 
   // Collect all input nodes from 'start_node'->target(s) by applying the node filter to discover new input nodes and
   // the target node predicate to stop discovering more inputs and start backtracking. The implementation is done
@@ -301,14 +304,14 @@ class DataNodesOnPathToTargets : public StackObj {
 };
 
 // Clones this Template Assertion Predicate Expression and applies the given strategy to transform the OpaqueLoop* nodes.
-Opaque4Node* TemplateAssertionPredicateExpression::clone(TransformStrategyForOpaqueLoopNodes& transform_strategy,
+Opaque4Node* TemplateAssertionPredicateExpression::clone(const TransformStrategyForOpaqueLoopNodes& transform_strategy,
                                                          Node* new_ctrl, PhaseIdealLoop* phase) {
   ResourceMark rm;
   auto is_opaque_loop_node = [](const Node* node) {
     return node->is_Opaque1();
   };
-  DataNodesOnPathToTargets data_nodes_on_path_to_targets(TemplateAssertionPredicateExpression::maybe_contains,
-                                                         is_opaque_loop_node);
+  DataNodesOnPathsToTargets data_nodes_on_path_to_targets(TemplateAssertionPredicateExpression::maybe_contains,
+                                                          is_opaque_loop_node);
   const Unique_Node_List& collected_nodes = data_nodes_on_path_to_targets.collect(_opaque4_node);
   DataNodeGraph data_node_graph(collected_nodes, phase);
   const OrigToNewHashtable& orig_to_new = data_node_graph.clone_with_opaque_loop_transform_strategy(transform_strategy, new_ctrl);

src/hotspot/share/opto/predicates.hpp

@@ -264,6 +264,13 @@ class RuntimePredicate : public StackObj {
   static bool is_success_proj(Node* node, Deoptimization::DeoptReason deopt_reason);
 };
 
+// Interface to transform OpaqueLoopInit and OpaqueLoopStride nodes of a Template Assertion Predicate Expression.
+class TransformStrategyForOpaqueLoopNodes : public StackObj {
+ public:
+  virtual Node* transform_opaque_init(OpaqueLoopInitNode* opaque_init) const = 0;
+  virtual Node* transform_opaque_stride(OpaqueLoopStrideNode* opaque_stride) const = 0;
+};
+
 // A Template Assertion Predicate Expression represents the Opaque4Node for the initial value or the last value of a
 // Template Assertion Predicate and all the nodes up to and including the OpaqueLoop* nodes.
 class TemplateAssertionPredicateExpression : public StackObj {
@@ -273,7 +280,7 @@ class TemplateAssertionPredicateExpression : public StackObj {
   explicit TemplateAssertionPredicateExpression(Opaque4Node* opaque4_node) : _opaque4_node(opaque4_node) {}
 
  private:
-  Opaque4Node* clone(TransformStrategyForOpaqueLoopNodes& transform_strategy, Node* new_ctrl, PhaseIdealLoop* phase);
+  Opaque4Node* clone(const TransformStrategyForOpaqueLoopNodes& transform_strategy, Node* new_ctrl, PhaseIdealLoop* phase);
 
  public:
   // Is 'n' a node that could be part of a Template Assertion Predicate Expression (i.e. could be found on the input

test/hotspot/jtreg/compiler/predicates/TestCloningWithManyDiamondsInExpression.java

@@ -35,6 +35,7 @@
  *                              -XX:CompileCommand=compileonly,*TestCloningWithManyDiamondsInExpression::test*
  *                              -XX:CompileCommand=inline,*TestCloningWithManyDiamondsInExpression::create*
  *                              compiler.predicates.TestCloningWithManyDiamondsInExpression
+ * @run main compiler.predicates.TestCloningWithManyDiamondsInExpression
  */
 
 package compiler.predicates;