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

Thu Apr 18 09:08:15 UTC 2024

On Wed, 10 Apr 2024 13:19:11 GMT, Christian Hagedorn <chagedorn 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

Nice refactoring! I left a few comments/suggestions.

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.

src/hotspot/share/opto/predicates.cpp line 337:

> 335: 
> 336: // Check if this node belongs a Template Assertion Predicate Expression (including OpaqueLoop* nodes).
> 337: bool TemplateAssertionPredicateExpressionNode::find_opaque_loop_nodes(Node* node) {

`find` usually tells me that you are going to return what you "find". You could name it more like what you have in the description: `is_in_expression` or `belongs_to_expression`. Also, looks like the single use of this is in `is_valid`, and that just wraps it. Is that intended?

src/hotspot/share/opto/predicates.cpp line 354:

> 352: }
> 353: 
> 354: void TemplateAssertionPredicateExpressionNode::push_non_null_inputs(Unique_Node_List& list, const Node* node) {

Why not make this a method in Node? `node->push_non_null_inputs(list)`.
If that can be part of the header file, then it would even be efficiently inlined, I assume.
We could then use it all over the place!

Well, you should probably indicate that you are not traversing `in(0)`... not sure what would be an adequate name.

src/hotspot/share/opto/predicates.cpp line 367:

> 365: }
> 366: 
> 367: void TemplateAssertionPredicateExpressionNode::push_outputs(Unique_Node_List& list, const Node* node) {

Why not make this a method in `Node`? `node->push_outs(list)`.

src/hotspot/share/opto/predicates.hpp line 297:

> 295: // - Two: A OpaqueLoopInitNode could be part of two Template Assertion Predicates.
> 296: // - One: In all other cases.
> 297: class TemplateAssertionPredicateExpressionNode : public StackObj {

I have a slight irritation that this has a `...Node` suffix. Indicates that it is a subclass of `Node`, which is not correct. But probably it is still a good name, so you can leave it.

src/hotspot/share/opto/predicates.hpp line 314:

> 312:  public:
> 313:   // Check whether the provided node is part of a Template Assertion Predicate Expression or not.
> 314:   static bool is_valid(Node* node) {

I would rename this too. To keep it similar to `is_maybe_in_template_assertion_predicate_expression`, name it `is_in_template_assertion_predicate_expression`.

Hmm. You could also shorten the names, since we know from the context that we are talking about TAPE:

is_in_expression
is_maybe_in_expression

Also: why is there the `find_opaque_loop_nodes` method? Is it even used anywhere else?

src/hotspot/share/opto/predicates.hpp line 320:

> 318:   // Check if the opcode of node could be found in a Template Assertion Predicate Expression.
> 319:   // This also provides a fast check whether a node is unrelated.
> 320:   static bool valid_opcode(const Node* node) {

I'm not a fan of this name, it implies that nodes could be "valid" or "invalid". For one, I think this should start with `is_...`. This would be very long, but at least more accurate: `is_maybe_in_template_assertion_predicate_expression`.

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?

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?

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18723#pullrequestreview-2008170891
PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1570266730
PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1570252881
PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1570304152
PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1570302342
PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1570317355
PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1570243984
PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1570233668
PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1570289388
PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1570311331