8346774: Use Predicate classes instead of Node classes #23234

chhagedorn · 2025-01-22T13:10:37Z

This small cleanup PR replaces a lot of usages of Node pointers, to pass around either the head (i.e. IfNode) or the tail (i.e. a success projection) of predicates, with actual Predicate classes. This simplifies the usages, readability and the logical flow, and enables more simplifications in the future, especially once we replace Template Assertion Predicates with a dedicated node.

I've also included some minor refactorings like adding const or fixing typos.

There are no semantic changes involved. The return value optimization should take care to avoid a lot of copies when returning new objects from methods.

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-8346774: Use Predicate classes instead of Node classes (Sub-task - P4)

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer) Review applies to a44280ba
Emanuel Peter (@eme64 - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23234

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-01-22T13:11:07Z

👋 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 · 2025-01-22T13:11:59Z

@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:

8346774: Use Predicate classes instead of Node classes

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 1 new commit pushed to the master branch:

6f4fc82: 8348675: TrayIcon tests fail in Ubuntu 24.10 Wayland

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 · 2025-01-22T13:12:34Z

@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 · 2025-01-22T13:12:10Z

src/hotspot/share/opto/predicates.cpp

+  OpaqueLoopInitNode* new_opaque_init = new OpaqueLoopInitNode(phase->C, new_opaque_input);
+  phase->register_new_node(new_opaque_init, new_control);


Moved here from the caller of clone_and_replace_init(). He now provides the input to the opaque node instead of creating the opaque node and passing the created node to this method.

chhagedorn · 2025-01-22T13:13:17Z

src/hotspot/share/opto/predicates.cpp

-// of the newly created Initialized Assertion Predicate.
-IfTrueNode* TemplateAssertionPredicate::initialize(PhaseIdealLoop* phase, Node* new_control) const {
+// Create a new Initialized Assertion Predicate from this template at the template success projection.
+InitializedAssertionPredicate TemplateAssertionPredicate::initialize(PhaseIdealLoop* phase) const {


Removed new_control. All callers created the Initialized Assertion Predicate below the template. Eventually, this will always be the case since we want to keep Template Assertion Predicates around in the future.

chhagedorn · 2025-01-22T13:13:50Z

src/hotspot/share/opto/predicates.cpp

+InitializedAssertionPredicate InitializedAssertionPredicateCreator::create_from_template(
+    IfNode* template_assertion_predicate, Node* new_control, Node* new_init, Node* new_stride) const {


Also made const and split long line.

chhagedorn · 2025-01-22T13:15:33Z

src/hotspot/share/opto/predicates.cpp

+InitializedAssertionPredicate InitializedAssertionPredicateCreator::create_from_template_and_insert_below(
+    const TemplateAssertionPredicate& template_assertion_predicate) const {


Same as above: Removed new_control.
Also made const.

chhagedorn · 2025-01-22T13:16:09Z

src/hotspot/share/opto/predicates.cpp

+  IfNode* template_assertion_predicate_if = template_assertion_predicate.head();
+  AssertionPredicateType assertion_predicate_type = template_assertion_predicate_if->assertion_predicate_type();
+  int if_opcode = template_assertion_predicate_if->Opcode();


Need to fetch the information from the Template Assertion Predicate now instead of directly from the If.

chhagedorn · 2025-01-22T13:17:02Z

src/hotspot/share/opto/predicates.cpp

-  OpaqueLoopInitNode* opaque_init = new OpaqueLoopInitNode(_phase->C, _init);
-  _phase->register_new_node(opaque_init, _old_target_loop_entry);


See earlier comment about moving opaque creation from caller -> callee.

chhagedorn · 2025-01-22T13:18:00Z

src/hotspot/share/opto/predicates.cpp

+  assert(predicate.head()->_idx >= _node_index_before_cloning, "must be a newly cloned predicate");
+  assert(predicate.tail()->_idx >= _node_index_before_cloning, "must be a newly cloned predicate");


Can now verify both the head and tail.

mlbridge · 2025-01-23T07:38:20Z

Webrevs

01: Full - Incremental (531763da)
00: Full (a44280ba)

vnkozlov

You use copy constructors to return *Predicate* objects (I assume because base class is StackObj). How it affects performance of C2? Unless all these methods are inlined.

chhagedorn · 2025-01-24T12:15:05Z

I assumed that RVO will make sure that we do not create an extra copy regardless of whether a method is inlined or not. Maybe @kimbarrett can comment on that.

Nevertheless, let me run some performance testing.

chhagedorn · 2025-01-27T20:40:37Z

Performance testing looked good.

chhagedorn · 2025-01-29T14:42:28Z

Thanks Vladimir for your review!

kimbarrett · 2025-01-29T17:59:27Z

I assumed that RVO will make sure that we do not create an extra copy regardless of whether a method is inlined or not. Maybe @kimbarrett can comment on that.

Not a review, just a response to the question about RVO.

The use of NRVO and RVO here looks fine to me.

C++17 makes RVO mandatory. We're not using C++17 yet, but part of the
rationale for making it mandatory is that compilers were already doing it (and
had been for some time), at least with optimizing settings. You just couldn't
rely on it, and had to provide the appropriate ctors even if RVO was always
performed. (For example, a copy-ctor is needed even if always using a factory
function that returns a directly constructed object.)

While NRVO was not made mandatory, that may have been more a matter of concern
over specification difficulty for corner cases. In my experience, obvious
cases like those in this PR have been getting NRVO from optimizing compilers
for a long time.

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0135r0.html
https://en.cppreference.com/w/cpp/language/copy_elision

Support for guaranteed copy elision
https://en.cppreference.com/w/cpp/compiler_support/17
gcc: 7, clang: 4, msvc: 19.13 (VS 2017 15.6)
These versions are for the C++17 "guaranteed in all circumstances" feature.

chhagedorn · 2025-01-30T06:50:41Z

Thanks a lot Kim for your detailed answer and the links! That's great to hear and gives us more confidence about these changes.

eme64

Looks reasonable, I have a few minor suggestions.

eme64 · 2025-02-03T09:30:42Z

src/hotspot/share/opto/predicates.cpp

-// Clone this Template Assertion Predicate and replace the OpaqueLoopInitNode with the provided 'new_opaque_init' node.
-IfTrueNode* TemplateAssertionPredicate::clone_and_replace_init(Node* new_control, OpaqueLoopInitNode* new_opaque_init,
-                                                               PhaseIdealLoop* phase) const {
+// Clone this Template Assertion Predicate and replace the input of the OpaqueLoopInitNode with 'new_opaque_input'.


Looks like you now also create a new OpaqueLoopInitNode, so that is slightly inaccurate, right?

eme64 · 2025-02-03T09:35:06Z

src/hotspot/share/opto/predicates.cpp

+   IfTrueNode* success_proj = create_control_nodes(new_control, template_assertion_predicate->Opcode(),
+                                                   assertion_expression,
+                                                   template_assertion_predicate->assertion_predicate_type());
+  return InitializedAssertionPredicate(success_proj);


Suggested change

IfTrueNode* success_proj = create_control_nodes(new_control, template_assertion_predicate->Opcode(),

assertion_expression,

template_assertion_predicate->assertion_predicate_type());

return InitializedAssertionPredicate(success_proj);

IfTrueNode* success_proj = create_control_nodes(new_control,

template_assertion_predicate->Opcode(),

assertion_expression,

template_assertion_predicate->assertion_predicate_type());

return InitializedAssertionPredicate(success_proj);

Indentation, and: If you split the args over lines, I would at least split all of them consistently.

I usually just make sure that long lines split but don't enforce a single arg per line. But I don't mind adapting to that here :-)

eme64

Looks good now!

chhagedorn · 2025-02-03T11:38:53Z

Thanks Emanuel for your review! I'm running some testing again with latest master and will then integrate it.

chhagedorn · 2025-02-04T07:33:09Z

/integrate

openjdk · 2025-02-04T07:33:35Z

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

7ea176d: 8349193: compiler/intrinsics/TestContinuationPinningAndEA.java missing @requires vm.continuations
9b49597: 8334320: Replace vmTestbase/metaspace/share/TriggerUnloadingWithWhiteBox.java with ClassUnloadCommon from testlibrary
43979fb: 8347428: Avoid using secret-key in specifications
618c5eb: 8349183: [BACKOUT] Optimization for StringBuilder append boolean & null
bb837d2: 8342775: [Graal] java/util/concurrent/locks/Lock/OOMEInAQS.java fails OOME thrown from the UncaughtExceptionHandler
a57c9b1: 8349184: [JMH] jdk.incubator.vector.ColumnFilterBenchmark.filterDoubleColumn fails on linux-aarch64
d330421: 8337548: Parallel class loading can pass is_superclass true for interfaces
3f1d9b5: 8348575: SpinLockT is typedef'ed but unused
6f4fc82: 8348675: TrayIcon tests fail in Ubuntu 24.10 Wayland

Your commit was automatically rebased without conflicts.

openjdk · 2025-02-04T07:33:41Z

@chhagedorn Pushed as commit c545a3e.

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

8346774: Use Predicate classes instead of Node classes

e724b69

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jan 22, 2025

chhagedorn added 2 commits January 22, 2025 14:17

more cleanups

a5ac535

more cleanups

a44280b

chhagedorn commented Jan 22, 2025

View reviewed changes

chhagedorn marked this pull request as ready for review January 23, 2025 07:33

openjdk bot added the rfr Pull request is ready for review label Jan 23, 2025

vnkozlov reviewed Jan 23, 2025

View reviewed changes

vnkozlov approved these changes Jan 28, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 28, 2025

eme64 reviewed Feb 3, 2025

View reviewed changes

chhagedorn added 2 commits February 3, 2025 11:27

Merge branch 'master' into JDK-8346774

9b6a64e

Review Emanuel

531763d

openjdk bot removed the ready Pull request is ready to be integrated label Feb 3, 2025

eme64 approved these changes Feb 3, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 3, 2025

openjdk bot added the integrated Pull request has been integrated label Feb 4, 2025

openjdk bot closed this Feb 4, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 4, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8346774: Use Predicate classes instead of Node classes #23234

8346774: Use Predicate classes instead of Node classes #23234

chhagedorn commented Jan 22, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 22, 2025

openjdk bot commented Jan 22, 2025 •

edited

Loading

openjdk bot commented Jan 22, 2025

chhagedorn Jan 22, 2025

chhagedorn Jan 22, 2025

chhagedorn Jan 22, 2025

chhagedorn Jan 22, 2025

chhagedorn Jan 22, 2025

chhagedorn Jan 22, 2025

chhagedorn Jan 22, 2025

mlbridge bot commented Jan 23, 2025 •

edited

Loading

vnkozlov left a comment

chhagedorn commented Jan 24, 2025

chhagedorn commented Jan 27, 2025

chhagedorn commented Jan 29, 2025

kimbarrett commented Jan 29, 2025

chhagedorn commented Jan 30, 2025

eme64 left a comment

eme64 Feb 3, 2025

eme64 Feb 3, 2025

eme64 Feb 3, 2025

chhagedorn Feb 3, 2025

eme64 left a comment

chhagedorn commented Feb 3, 2025

chhagedorn commented Feb 4, 2025

openjdk bot commented Feb 4, 2025

openjdk bot commented Feb 4, 2025

		OpaqueLoopInitNode* new_opaque_init = new OpaqueLoopInitNode(phase->C, new_opaque_input);
		phase->register_new_node(new_opaque_init, new_control);

		InitializedAssertionPredicate InitializedAssertionPredicateCreator::create_from_template(
		IfNode* template_assertion_predicate, Node* new_control, Node* new_init, Node* new_stride) const {

		InitializedAssertionPredicate InitializedAssertionPredicateCreator::create_from_template_and_insert_below(
		const TemplateAssertionPredicate& template_assertion_predicate) const {

		OpaqueLoopInitNode* opaque_init = new OpaqueLoopInitNode(_phase->C, _init);
		_phase->register_new_node(opaque_init, _old_target_loop_entry);

		assert(predicate.head()->_idx >= _node_index_before_cloning, "must be a newly cloned predicate");
		assert(predicate.tail()->_idx >= _node_index_before_cloning, "must be a newly cloned predicate");

8346774: Use Predicate classes instead of Node classes #23234

8346774: Use Predicate classes instead of Node classes #23234

Conversation

chhagedorn commented Jan 22, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 22, 2025

openjdk bot commented Jan 22, 2025 • edited Loading

openjdk bot commented Jan 22, 2025

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

mlbridge bot commented Jan 23, 2025 • edited Loading

Webrevs

vnkozlov left a comment

Choose a reason for hiding this comment

chhagedorn commented Jan 24, 2025

chhagedorn commented Jan 27, 2025

chhagedorn commented Jan 29, 2025

kimbarrett commented Jan 29, 2025

chhagedorn commented Jan 30, 2025

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

eme64 left a comment

Choose a reason for hiding this comment

chhagedorn commented Feb 3, 2025

chhagedorn commented Feb 4, 2025

openjdk bot commented Feb 4, 2025

openjdk bot commented Feb 4, 2025

chhagedorn commented Jan 22, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jan 22, 2025 •

edited

Loading

mlbridge bot commented Jan 23, 2025 •

edited

Loading