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

[Roland Westrelin]

On Wed, 21 May 2025 12:47:21 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> 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);
> +    ...

Thanks for the patch. I applied it and did some smoke testing. I think there's a mistake at the end:

-  _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);

and

+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);)
+}

lines added only use `_clone_predicate_to_true_path_loop` and not `_clone_predicate_to_false_path_loop`. Commit I pushed should fix that.

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

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