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

[Christian Hagedorn]

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

src/hotspot/share/opto/loopTransform.cpp line 1399:

> 1397: // Is 'n' a node that can be found on the input chain of a Template Assertion Predicate bool (i.e. between a Template
> 1398: // Assertion Predicate If node and the OpaqueLoop* nodes)?
> 1399: static bool is_part_of_template_assertion_predicate_bool(Node* n) {

Only used by the now dead `subgraph_has_opaque()` method.

src/hotspot/share/opto/loopTransform.cpp line 1440:

> 1438:   for (uint i = 0; i < wq.size(); i++) {
> 1439:     Node* n = wq.at(i);
> 1440:     if (TemplateAssertionPredicateExpressionNode::valid_opcode(n)) {

`valid_opcode()` also includes the `OpaqueLoop*` nodes which was not the  case for `is_part_of_template_assertion_predicate_bool()`. Therefore needed to adapt the code below to handle that.

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

> 324:     return node->is_Opaque1();
> 325:   };
> 326:   DataNodesOnPathsToTargets data_nodes_on_path_to_targets(TemplateAssertionPredicateExpressionNode::valid_opcode,

Moved `TemplateAssertionPredicateExpression::maybe_contains()` to `TemplateAssertionPredicateExpressionNode::valid_opcode()` which suited better.

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

> 293: // The expression itself can belong to no, one, or two Template Assertion Predicates:
> 294: // - None: This node is already dead (i.e. we replaced the Bool condition of the Template Assertion Predicate).
> 295: // - Two: A OpaqueLoopInitNode could be part of two Template Assertion Predicates.

This is a little odd but I did not want to change that. This can only happen when we have not cloned a Template Assertion Predicate Expression, yet (when cloning, we will not common the `OpaqueLoopInitNodes`). Maybe we could create a separate `OpaqueLoopInitNode` for the init and the last value Template Assertion Predicate at some point - but that would just beautify this code here and does not currently offer any other benefit. On top of that (even though rather minor), we have additional nodes that are actually not required to make Template Assertion Predicates work. So, I'm not sure if we really want that.

src/hotspot/share/opto/split_if.cpp line 418:

> 416: // untouched copy that is still recognized by the Template Assertion Predicate matching code.
> 417: void PhaseIdealLoop::clone_template_assertion_predicate_expression_down(Node* node) {
> 418:   if (!TemplateAssertionPredicateExpressionNode::is_valid(node)) {

Covers previous `subgraph_has_opaque()` call to check whether this node is part of a Template Assertion Predicate Expression.

test/hotspot/jtreg/compiler/predicates/assertion/TestSplitIfCloningDown.java line 44:

> 42: package compiler.predicates.assertion;
> 43: 
> 44: public class TestSplitIfCloningDown {

Just a sanity test that exercises the refactored code. There was no assert or anything alike when running the test before this refactoring.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1559425203
PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1559426759
PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1559428056
PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1559461421
PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1559429400
PR Review Comment: https://git.openjdk.org/jdk/pull/18723#discussion_r1559431474