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

[Emanuel Peter]

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

>> This is another patch split off https://github.com/openjdk/jdk/pull/16877. It refactors the "cloning down" code for Split If with Template Assertion Predicates. This mainly includes the replacement of `subgraph_has_opaque()` with a new class `TemplateAssertionPredicateExpressionNode`. More details can be found as PR comments.
>> 
>> #### Background
>> 
>> The cloning down code is required in Split If when trying to split any node up that belongs to a Template Assertion Predicate Expression (TAPE) (including the `OpaqueLoop*` nodes). We need to prevent that to avoid having any phi nodes in the TAPE which could result in failures when trying to later match and clone Template Assertion Predicates. Instead of cloning such a TAPE node up, we clone ("down") the entire TAPE.
>> 
>> Thanks,
>> Christian
>
> 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.

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

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

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1570281819
PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1570298607
PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1570312418