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

[Christian Hagedorn]

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

>> Christian Hagedorn has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Move push-inputs/outputs to Node_List.
>>  - Review Emanuel
>
> 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.

Good point. Might it even be better suited inside `Node_List`? It does more sound like a thing a list should know how to do.

How about going with `list.push_non_null_non_cfg_inputs_of(node)`?

FWIW, there are quite some places where we only want to put the non-cfg nodes on a node list. I suggest to file a follow up RFE to replace those with this new method if you agree. Same for the outputs below.

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

Same as above, better inside `Node_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.

I see your point. Due to a lack of having a better naming idea, I went with `...Node`. Let me think some more about 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?

I think that's a left-over from an earlier refactoring. I removed `find_opaque_loop_nodes()` and directly use `is_in_expression()`.

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

It's always a good sign when you point out the names I'm also not quite satisfied to start with - so that's an indicator to change it :-) 

I tried to be concise here but then there are really no good and precise options. Let's go with `is_maybe_in_expression()` and `is_in_expression()` as also suggested above.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1570596089
PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1570596662
PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1570597725
PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1570608887
PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1570601200