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

chhagedorn · 2024-04-10T13:19:11Z

This is another patch split off #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

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8330004: Refactor cloning down code in Split If for Template Assertion Predicates (Sub-task - P4)

Reviewers

Emanuel Peter (@eme64 - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18723/head:pull/18723
$ git checkout pull/18723

Update a local copy of the PR:
$ git checkout pull/18723
$ git pull https://git.openjdk.org/jdk.git pull/18723/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18723

View PR using the GUI difftool:
$ git pr show -t 18723

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18723.diff

Webrev

Link to Webrev Comment

…n Predicates

bridgekeeper · 2024-04-10T13:19:54Z

👋 Welcome back chagedorn! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-04-10T13:20:37Z

@chhagedorn This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: epeter, kvn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 165 new commits pushed to the master branch:

f713766: 8329322: Convert PageFormat/Orient.java to use PassFailJFrame
ec180d4: 8324950: IGV: save the state to a file
571e6bc: 8330419: Unused code in ConnectionGraph::specialize_castpp
5853a2c: 8330533: JFR: LocalDateTime should not use milliseconds since epoch
b648ed0: 8329997: Add provisions for checking memory segment alignment constraints
60b65e6: 8325872: Make GuaranteedSafepointInterval default 0
b049609: 8330094: RISC-V: Save and restore FRM in the call stub
4b55fe5: 8330520: linux clang build fails in os_linux.cpp with static_assert with no message is a C++17 extension
5eb2c59: 8330475: Remove unused default value for ModRefBarrierSet::write_ref_array_pre
003e86f: 8324755: Enable parallelism in vmTestbase/gc/gctests/LargeObjects tests
... and 155 more: https://git.openjdk.org/jdk/compare/fc18201bbdac7ac7d78767c780d3efe5352ee77a...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-04-10T13:21:05Z

@chhagedorn The following label will be automatically applied to this pull request:

hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

chhagedorn · 2024-04-10T13:19:41Z

src/hotspot/share/opto/loopTransform.cpp

@@ -1394,40 +1394,6 @@ void PhaseIdealLoop::copy_assertion_predicates_to_main_loop_helper(const Predica
  }
 }

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


Only used by the now dead subgraph_has_opaque() method.

chhagedorn · 2024-04-10T13:20:46Z

src/hotspot/share/opto/loopTransform.cpp

-        Node* m = n->in(j);
-        if (m != nullptr) {
-          wq.push(m);
+    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.

chhagedorn · 2024-04-10T13:21:41Z

src/hotspot/share/opto/predicates.cpp

@@ -323,7 +323,7 @@ Opaque4Node* TemplateAssertionPredicateExpression::clone(const TransformStrategy
  auto is_opaque_loop_node = [](const Node* node) {
    return node->is_Opaque1();
  };
-  DataNodesOnPathsToTargets data_nodes_on_path_to_targets(TemplateAssertionPredicateExpression::maybe_contains,
+  DataNodesOnPathsToTargets data_nodes_on_path_to_targets(TemplateAssertionPredicateExpressionNode::valid_opcode,


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

chhagedorn · 2024-04-10T13:22:39Z

src/hotspot/share/opto/split_if.cpp

+// we completely clone the entire Template Assertion Predicate Expression "down". This ensures that we have an
+// untouched copy that is still recognized by the Template Assertion Predicate matching code.
+void PhaseIdealLoop::clone_template_assertion_predicate_expression_down(Node* node) {
+  if (!TemplateAssertionPredicateExpressionNode::is_valid(node)) {


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

chhagedorn · 2024-04-10T13:23:59Z

test/hotspot/jtreg/compiler/predicates/assertion/TestSplitIfCloningDown.java

+
+package compiler.predicates.assertion;
+
+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.

chhagedorn · 2024-04-10T13:41:14Z

src/hotspot/share/opto/predicates.hpp

+//
+// The expression itself can belong to no, one, or two Template Assertion Predicates:
+// - None: This node is already dead (i.e. we replaced the Bool condition of the Template Assertion Predicate).
+// - 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.

mlbridge · 2024-04-11T06:32:52Z

Webrevs

eme64

Nice refactoring! I left a few comments/suggestions.

eme64 · 2024-04-18T08:16:02Z

src/hotspot/share/opto/predicates.hpp

+
+  // Check if the opcode of node could be found in a Template Assertion Predicate Expression.
+  // This also provides a fast check whether a node is unrelated.
+  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.

eme64 · 2024-04-18T08:22:28Z

src/hotspot/share/opto/predicates.hpp

+
+ public:
+  // Check whether the provided node is part of a Template Assertion Predicate Expression or not.
+  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().

eme64 · 2024-04-18T08:27:39Z

src/hotspot/share/opto/predicates.cpp

@@ -332,3 +332,41 @@ Opaque4Node* TemplateAssertionPredicateExpression::clone(const TransformStrategy
  Node* opaque4_clone = *orig_to_new.get(_opaque4_node);
  return opaque4_clone->as_Opaque4();
 }
+
+// Check if this node belongs a Template Assertion Predicate Expression (including OpaqueLoop* nodes).
+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?

eme64 · 2024-04-18T08:36:08Z

src/hotspot/share/opto/loopTransform.cpp

+          Node* m = n->in(j);
+          if (m != nullptr) {
+            wq.push(m);
+          }


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.

eme64 · 2024-04-18T08:47:03Z

src/hotspot/share/opto/predicates.hpp

+  // Apply the given function to all Template Assertion Predicates (if any) to which this Template Assertion Predicate
+  // Expression Node belongs to.
+  template <class ApplyToTemplateFunction>
+  void for_each_template_assertion_predicate(ApplyToTemplateFunction apply_to_template_function) {


Suggested change

void for_each_template_assertion_predicate(ApplyToTemplateFunction apply_to_template_function) {

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.

eme64 · 2024-04-18T08:54:28Z

src/hotspot/share/opto/predicates.cpp

+  return node->is_If() && node->in(1)->is_Opaque4();
+}
+
+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?

eme64 · 2024-04-18T08:55:32Z

src/hotspot/share/opto/predicates.cpp

+  return false;
+}
+
+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.

eme64 · 2024-04-18T08:59:52Z

src/hotspot/share/opto/predicates.hpp

+      }
+    }
+    assert(template_counter <= 2, "a node cannot be part of more than two templates");
+    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.

eme64 · 2024-04-18T09:03:40Z

src/hotspot/share/opto/predicates.hpp

+// - None: This node is already dead (i.e. we replaced the Bool condition of the Template Assertion Predicate).
+// - Two: A OpaqueLoopInitNode could be part of two Template Assertion Predicates.
+// - One: In all other cases.
+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.

chhagedorn

Thanks a lot for your review!

I've addressed all your comments and pushed an update. I already incorporated the idea of moving the push methods to Node_List.

chhagedorn · 2024-04-18T12:02:49Z

src/hotspot/share/opto/loopTransform.cpp

+          Node* m = n->in(j);
+          if (m != nullptr) {
+            wq.push(m);
+          }


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

chhagedorn · 2024-04-18T12:05:40Z

src/hotspot/share/opto/predicates.cpp

+  return false;
+}
+
+void TemplateAssertionPredicateExpressionNode::push_non_null_inputs(Unique_Node_List& list, const Node* node) {


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.

chhagedorn · 2024-04-18T12:06:03Z

src/hotspot/share/opto/predicates.cpp

+  return node->is_If() && node->in(1)->is_Opaque4();
+}
+
+void TemplateAssertionPredicateExpressionNode::push_outputs(Unique_Node_List& list, const Node* node) {


Same as above, better inside Node_List?

chhagedorn · 2024-04-18T12:06:50Z

src/hotspot/share/opto/predicates.hpp

+// - None: This node is already dead (i.e. we replaced the Bool condition of the Template Assertion Predicate).
+// - Two: A OpaqueLoopInitNode could be part of two Template Assertion Predicates.
+// - One: In all other cases.
+class TemplateAssertionPredicateExpressionNode : public StackObj {


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.

chhagedorn · 2024-04-18T12:09:24Z

src/hotspot/share/opto/predicates.hpp

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


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.

chhagedorn · 2024-04-18T12:14:37Z

src/hotspot/share/opto/predicates.hpp

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


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

chhagedorn · 2024-04-18T12:15:30Z

src/hotspot/share/opto/predicates.hpp

+  // Apply the given function to all Template Assertion Predicates (if any) to which this Template Assertion Predicate
+  // Expression Node belongs to.
+  template <class ApplyToTemplateFunction>
+  void for_each_template_assertion_predicate(ApplyToTemplateFunction apply_to_template_function) {


Fair point, updated.

chhagedorn · 2024-04-18T12:19:32Z

src/hotspot/share/opto/predicates.hpp

+      }
+    }
+    assert(template_counter <= 2, "a node cannot be part of more than two templates");
+    assert(template_counter <= 1 || _node->is_OpaqueLoopInit(), "only OpaqueLoopInit nodes can be part of two templates");


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.

eme64

Looks good to me now.

chhagedorn · 2024-04-18T14:29:00Z

Thanks Emanuel for your review and good catch with Unique_Node_List::push() not overriding Node_List::push()! We should clean up these classes and some point.

vnkozlov

Looks good.

chhagedorn · 2024-04-19T07:27:07Z

Thanks Vladimir for your review!

I'll submit some more testing with the latest changes before integration.

chhagedorn · 2024-04-22T06:06:43Z

/integrate

openjdk · 2024-04-22T06:07:09Z

Going to push as commit 20546c1.
Since your change was applied there have been 185 commits pushed to the master branch:

bd67ac6: 8329331: Intrinsify Unsafe::setMemory
185e711: 8318650: Optimized subword gather for x86 targets.
6d56996: 8330540: Rename the enum type CompileCommand to CompileCommandEnum
f6feeb0: 8330703: Improve link syntax in javax.lang.model.util
df04358: 8330179: Clean up non-standard use of /** comments in jdk.compiler
c1dd82b: 8329644: Discuss expected visitor evolution patterns in javax.lang.model.util
b704e91: 8329433: Reduce nmethod header size
8da175d: 8330008: [s390x] Test bit "in-memory" in case of DiagnoseSyncOnValueBasedClasses
3c1d1d9: 8320522: Remove code related to RegisterFinalizersAtInit
650ca65: 8329330: NoClassDefFoundError: Could not initialize class jdk.jfr.internal.MirrorEvents
... and 175 more: https://git.openjdk.org/jdk/compare/fc18201bbdac7ac7d78767c780d3efe5352ee77a...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-22T06:07:14Z

@chhagedorn Pushed as commit 20546c1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8330004: Refactor cloning down code in Split If for Template Assertio…

3f40bd5

…n Predicates

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Apr 10, 2024

chhagedorn commented Apr 10, 2024

View reviewed changes

chhagedorn marked this pull request as ready for review April 11, 2024 06:29

openjdk bot added the rfr Pull request is ready for review label Apr 11, 2024

eme64 suggested changes Apr 18, 2024

View reviewed changes

chhagedorn added 2 commits April 18, 2024 14:27

Review Emanuel

ae4b1f4

Move push-inputs/outputs to Node_List.

f072bae

chhagedorn commented Apr 18, 2024

View reviewed changes

Update

00fe1bb

eme64 approved these changes Apr 18, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 18, 2024

vnkozlov approved these changes Apr 18, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 22, 2024

openjdk bot closed this Apr 22, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 22, 2024

chhagedorn mentioned this pull request Apr 22, 2024

8305638: Renaming and small clean-ups around predicates #16877

Closed

3 tasks


		package compiler.predicates.assertion;

		public class TestSplitIfCloningDown {

	void for_each_template_assertion_predicate(ApplyToTemplateFunction apply_to_template_function) {
	void for_each_template_assertion_predicate(ApplyToTemplateFunction apply_to_template_function) const {

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

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

Conversation

chhagedorn commented Apr 10, 2024 • edited by openjdk bot Loading

Background

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 10, 2024

openjdk bot commented Apr 10, 2024 • edited Loading

openjdk bot commented Apr 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chhagedorn Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

mlbridge bot commented Apr 11, 2024 • edited Loading

Webrevs

eme64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chhagedorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eme64 left a comment

Choose a reason for hiding this comment

chhagedorn commented Apr 18, 2024 • edited Loading

vnkozlov left a comment

Choose a reason for hiding this comment

chhagedorn commented Apr 19, 2024

chhagedorn commented Apr 22, 2024

openjdk bot commented Apr 22, 2024

openjdk bot commented Apr 22, 2024

chhagedorn commented Apr 10, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Apr 10, 2024 •

edited

Loading

chhagedorn Apr 10, 2024 •

edited

Loading

mlbridge bot commented Apr 11, 2024 •

edited

Loading

chhagedorn commented Apr 18, 2024 •

edited

Loading