RFR: 8327110: Refactor create_bool_from_template_assertion_predicate() to separate class and fix identical cloning cases used for Loop Unswitching and Split If [v2]

[Christian Hagedorn]

On Wed, 20 Mar 2024 16:16:02 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Christian Hagedorn has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
>> 
>>  - Change from DFS to 2xBFS
>>  - Review Emanuel first part
>>  - Merge branch 'master' into JDK-8327110
>>  - 8327110: Refactor create_bool_from_template_assertion_predicate() to separate class and fix pure cloning cases used for Loop Unswitching and Split If
>
> src/hotspot/share/opto/loopnode.hpp line 1662:
> 
>> 1660:                                                      ParsePredicateSuccessProj* fast_loop_parse_predicate_proj,
>> 1661:                                                      ParsePredicateSuccessProj* slow_loop_parse_predicate_proj);
>> 1662:   IfProjNode* clone_assertion_predicate_for_unswitched_loops(Node* template_assertion_predicate, IfProjNode* predicate,
> 
> Suggestion:
> 
>   IfProjNode* clone_assertion_predicate_for_unswitched_loops(IfNode* template_assertion_predicate, IfProjNode* predicate,
> 
> Could we improve the type? I think all uses have `IfNode*`.

Sure, the code will eventually be removed but I guess it does not hurt to still improve the type.

> src/hotspot/share/opto/loopnode.hpp line 1937:
> 
>> 1935:   }
>> 1936: 
>> 1937:   // Create a copy of the provided data node collection by doing the following:
> 
> By "collection" you mean the `_data_nodes`, right? maybe some renaming could be helpful?

Updated the comment to make it more clear. The caller of this method should not need to know about the internal `_data_nodes` field.

> src/hotspot/share/opto/loopopts.cpp line 4542:
> 
>> 4540: 
>> 4541: void DataNodeGraph::transform_opaque_node(TransformStrategyForOpaqueLoopNodes& transform_strategy, Node* node) {
>> 4542:   const uint next_idx = _phase->C->unique();
> 
> Does this have any use?

Good catch, that's an unused leftover - removed.

> src/hotspot/share/opto/predicates.cpp line 189:
> 
>> 187: 
>> 188:   NodeCheck _node_filter; // Node filter function to decide if we should process a node or not while searching for targets.
>> 189:   NodeCheck _is_target_node; // Function to decide if a node is a target node (i.e. where we should start backtracking).
> 
> There should be some remark that all target nodes must pass the filter.

Good point, updated the comment

> src/hotspot/share/opto/predicates.cpp line 306:
> 
>> 304: Opaque4Node* TemplateAssertionPredicateExpression::clone(TransformStrategyForOpaqueLoopNodes& transform_strategy,
>> 305:                                                          Node* new_ctrl, PhaseIdealLoop* phase) {
>> 306:   ResourceMark rm;
> 
> The ResourceMark makes me a bit nervous, in combination of a non-constant `transform_strategy`.
> Could the `transform_strategy` be a constant reference, maybe by making its functions also const?

Done.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18293#discussion_r1537635240
PR Review Comment: https://git.openjdk.org/jdk/pull/18293#discussion_r1537659968
PR Review Comment: https://git.openjdk.org/jdk/pull/18293#discussion_r1537662060
PR Review Comment: https://git.openjdk.org/jdk/pull/18293#discussion_r1537664444
PR Review Comment: https://git.openjdk.org/jdk/pull/18293#discussion_r1537674109