RFR: 8330004: Refactor cloning down code in Split If for Template Assertion Predicates [v2]

[Christian Hagedorn]

On Thu, 18 Apr 2024 08:43:51 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> src/hotspot/share/opto/loopTransform.cpp line 1450:
>> 
>>> 1448:           if (m != nullptr) {
>>> 1449:             wq.push(m);
>>> 1450:           }
>> 
>> This code now looks almost identical to `TemplateAssertionPredicateExpressionNode::find_opaque_loop_nodes`, except that there we just are looking for one init/stride, and here we count them. Can we refactor this?
>> 
>> Idea: have a callback/lambda we call on init/stride nodes. That callback can then count, or give a "terminate" return: `enum Action { Continue, Terminate };`.
>> 
>> Or maybe you just use the count method also for checking if there are any init/stride. That is slightly more expensive, but maybe it does not matter. Or you give your count method a parameter that simply terminates early, and a return value true/false if we found any. Lots of options.
>
> Ah wait. That is what we used to do: count to check if there are any. That is what `subgraph_has_opaque` did.

Yes, I eventually want to get rid of `count_opaque_loop_nodes()`. This is kinda an intermediate state.

>> src/hotspot/share/opto/predicates.hpp line 344:
>> 
>>> 342:   // Expression Node belongs to.
>>> 343:   template <class ApplyToTemplateFunction>
>>> 344:   void for_each_template_assertion_predicate(ApplyToTemplateFunction apply_to_template_function) {
>> 
>> Suggestion:
>> 
>>   void for_each_template_assertion_predicate(ApplyToTemplateFunction apply_to_template_function) const {
>> 
>> Would that work?
>
> I think the name `ApplyToTemplateFunction` / `apply_to_template_function` is more noisy than necessary. I would just do `Callback` and `callback`. In the for-each context it is clear what this means.

Fair point, updated.

>> src/hotspot/share/opto/predicates.hpp line 360:
>> 
>>> 358:     }
>>> 359:     assert(template_counter <= 2, "a node cannot be part of more than two templates");
>>> 360:     assert(template_counter <= 1 || _node->is_OpaqueLoopInit(), "only OpaqueLoopInit nodes can be part of two templates");
>> 
>> Can this be true? Maybe there are some implicit assumptions about `_node`. But if it was for example an `AddI`, then this node could be used all over the place, and certainly could be used by many TAPE. Am I wrong?
>
> If these asserts are correct, you probably want to add some comments.

When we follow all inputs of a `TemplateAssertionPredicateExpressionNode`, we eventually end up at an `OpaqueLoop*Node`. These nodes do not common up. Therefore, each `TemplateAssertionPredicateExpressionNode` can only be part of one Template Assertion Predicate Expression. One exception is the `OpaqueLoopInitNode` itself. Due to convenience, the init and last value Template Assertion Predicate share this node. I can add a comment to explain these asserts further.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1570592050
PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1570610236
PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1570616176