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

[Christian Hagedorn]

On Thu, 22 May 2025 14:05:09 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> 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, _p...
>
> 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.

Good catch, thanks!

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

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