RFR: 8342692: C2: long counted loop/long range checks: don't create loop-nest for short running loops [v24]

[Christian Hagedorn]

On Wed, 21 May 2025 12:05:06 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> src/hotspot/share/opto/loopnode.cpp line 1156:
>> 
>>> 1154: 
>>> 1155:   void visit(const ParsePredicate& parse_predicate) override {
>>> 1156:     _clone_predicate_to_loop.clone_parse_predicate(parse_predicate, true);
>> 
>> `clone_parse_predicate()` has its second parameter named `is_false_path_loop`. I think that no longer makes sense because we are now reusing the method outside Loop Unswitching.. Maybe we should just rename the parameter to `rewire_uncommon_proj_phi_inputs` which is eventually the name in `create_new_if_for_predicate()`. Additionally, we should rename `ParsePredicate::clone_to_unswitched_loop()` to `clone_to_loop()`. What do you think?
>
> Sounds good. New commit has this renaming. Question now is what we do with `ParsePredicate::trace_cloned_parse_predicate()` that wouldn't always print a message that makes sense.

Good catch. That is now off as well. Additionally, it should probably be `TraceLoopUnswitching` and not `TraceLoopPredicate`.

 We could return the `ParsePredicate` from `clone_parse_predicate()` which is called from `CloneUnswitchedLoopPredicatesVisitor::visit()` and then call it from there. Maybe something like below?

<details>
  <summary>Patch Suggestion (untested)</summary>


Index: src/hotspot/share/opto/predicates.hpp
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/hotspot/share/opto/predicates.hpp b/src/hotspot/share/opto/predicates.hpp
--- a/src/hotspot/share/opto/predicates.hpp	(revision a0cdf36bdfeca9cd8b669859700d63d5ee627458)
+++ b/src/hotspot/share/opto/predicates.hpp	(date 1747831252516)
@@ -288,8 +288,6 @@
   }
 
   static ParsePredicateNode* init_parse_predicate(const Node* parse_predicate_proj, Deoptimization::DeoptReason deopt_reason);
-  NOT_PRODUCT(static void trace_cloned_parse_predicate(bool is_false_path_loop,
-                                                       const ParsePredicateSuccessProj* success_proj);)
 
  public:
   ParsePredicate(Node* parse_predicate_proj, Deoptimization::DeoptReason deopt_reason)
@@ -320,8 +318,8 @@
     return _success_proj;
   }
 
-  ParsePredicate clone_to_unswitched_loop(Node* new_control, bool is_false_path_loop,
-                                          PhaseIdealLoop* phase) const;
+  ParsePredicate clone_to_unswitched_loop(Node* new_control, bool is_false_path_loop, PhaseIdealLoop* phase) const;
+  NOT_PRODUCT(void trace_cloned_parse_predicate(bool is_false_path_loop) const;)
 
   void kill(PhaseIterGVN& igvn) const;
 };
@@ -1158,10 +1156,11 @@
   ClonePredicateToTargetLoop(LoopNode* target_loop_head, const NodeInLoopBody& node_in_loop_body, PhaseIdealLoop* phase);
 
   // Clones the provided Parse Predicate to the head of the current predicate chain at the target loop.
-  void clone_parse_predicate(const ParsePredicate& parse_predicate, bool is_false_path_loop) {
+  ParsePredicate clone_parse_predicate(const ParsePredicate& parse_predicate, bool is_false_path_loop) {
     ParsePredicate cloned_parse_predicate = parse_predicate.clone_to_unswitched_loop(_old_target_loop_entry,
                                                                                      is_false_path_loop, _phase);
     _target_loop_predicate_chain.insert_predicate(cloned_parse_predicate);
+    return cloned_parse_predicate;
   }
 
   void clone_template_assertion_predicate(const TemplateAssertionPredicate& template_assertion_predicate);
@@ -1189,6 +1188,8 @@
   using PredicateVisitor::visit;
 
   void visit(const ParsePredicate& parse_predicate) override;
+  void CloneUnswitchedLoopPredicatesVisitor::clone_parse_predicate(const ParsePredicate& parse_predicate,
+                                                                   bool is_false_path_loop);
   void visit(const TemplateAssertionPredicate& template_assertion_predicate) override;
 };
 
Index: src/hotspot/share/opto/predicates.cpp
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/hotspot/share/opto/predicates.cpp b/src/hotspot/share/opto/predicates.cpp
--- a/src/hotspot/share/opto/predicates.cpp	(revision a0cdf36bdfeca9cd8b669859700d63d5ee627458)
+++ b/src/hotspot/share/opto/predicates.cpp	(date 1747831362294)
@@ -87,7 +87,6 @@
   ParsePredicateSuccessProj* success_proj = phase->create_new_if_for_predicate(_success_proj, new_control,
                                                                                _parse_predicate_node->deopt_reason(),
                                                                                Op_ParsePredicate, is_false_path_loop);
-  NOT_PRODUCT(trace_cloned_parse_predicate(is_false_path_loop, success_proj));
   return ParsePredicate(success_proj, _parse_predicate_node->deopt_reason());
 }
 
@@ -97,11 +96,10 @@
 }
 
 #ifndef PRODUCT
-void ParsePredicate::trace_cloned_parse_predicate(const bool is_false_path_loop,
-                                                  const ParsePredicateSuccessProj* success_proj) {
-  if (TraceLoopPredicate) {
+void ParsePredicate::trace_cloned_parse_predicate(const bool is_false_path_loop) const {
+  if (TraceLoopUnswitching) {
     tty->print("Parse Predicate cloned to %s path loop: ", is_false_path_loop ? "false" : "true");
-    success_proj->in(0)->dump();
+    head()->dump();
   }
 }
 #endif // NOT PRODUCT
@@ -1108,11 +1106,18 @@
   if (_is_counted_loop && deopt_reason == Deoptimization::Reason_loop_limit_check) {
     return;
   }
-  _clone_predicate_to_true_path_loop.clone_parse_predicate(parse_predicate, false);
-  _clone_predicate_to_false_path_loop.clone_parse_predicate(parse_predicate, true);
+  clone_parse_predicate(parse_predicate, false);
+  clone_parse_predicate(parse_predicate, true);
   parse_predicate.kill(_phase->igvn());
 }
 
+void CloneUnswitchedLoopPredicatesVisitor::clone_parse_predicate(const ParsePredicate& parse_predicate,
+                                                                 const bool is_false_path_loop) {
+  const ParsePredicate cloned_parse_predicate =
+      _clone_predicate_to_true_path_loop.clone_parse_predicate(parse_predicate, false);
+  NOT_PRODUCT(cloned_parse_predicate.trace_cloned_parse_predicate(is_false_path_loop);)
+}
+
 // Clone the Template Assertion Predicate, which is currently found before the newly added unswitched loop selector,
 // to the true path and false path loop.
 void CloneUnswitchedLoopPredicatesVisitor::visit(const TemplateAssertionPredicate& template_assertion_predicate) {


</details>

The only downside is that we actually would not need the return value in product. But I guess that's negligible.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21630#discussion_r2100204407