8322490: cleanup CastNode construction #17162

caojoshua · 2023-12-19T20:27:06Z

It is a common pattern to have:

Node* n = new CastNode(...);
n->set_req(control_node);

We can modify the constructor to set the control node. It makes the code a little tidier.

Passes tier1 locally on my Linux machine

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-8322490: cleanup CastNode construction (Enhancement - P5)

Reviewers

Christian Hagedorn (@chhagedorn - Reviewer) ⚠️ Review applies to 574617ef
Paul Hohensee (@phohensee - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17162

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-12-19T20:28:09Z

👋 Welcome back caojoshua! 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 · 2023-12-19T20:29:10Z

@caojoshua 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.

mlbridge · 2023-12-19T20:31:54Z

Webrevs

chhagedorn

I think that's a good idea but you've missed some cases of creating new CastIINodes that could use the control-based constructor.

Example (there are more cases when you search for "new CastIINode"):

jdk/src/hotspot/share/opto/loopTransform.cpp

Lines 3164 to 3166 in f7dc257

    
           main_limit = new CastIINode(main_limit, TypeInt::make(upward ? min_jint : orig_limit_t->_lo, 
        
                                                                 upward ? orig_limit_t->_hi : max_jint, Type::WidenMax)); 
        
           main_limit->init_req(0, pre_ctrl);

chhagedorn · 2023-12-20T07:39:31Z

src/hotspot/share/opto/castnode.cpp

@@ -126,37 +126,20 @@ uint ConstraintCastNode::size_of() const {
 Node* ConstraintCastNode::make_cast(int opcode, Node* c, Node* n, const Type* t, DependencyType dependency,


I'm wondering if this method is still worth keeping as it would simply replace a "new CastXXNode" line which is just as expressive/readable. As far as I can see, it's currently only used with statically known Opcodes. So, we could replace them.

On a separate note and might be worth to do in this change as well, we have ConstraintCastNode::make_cast_for_type() and ConstraintCastNode::make(). The intent of the former is clear but the latter switches on the provided basic type. Maybe we can rename the latter to make_cast_for_basic_type() to better align it with make_cast_for_type().

Agree that make_cast probably is not needed. And your renaming suggestion makes sense.

Can we have this in a separate PR? I generally prefer to keep PRs as small of a unit as possible.

If you change the title of the PR into something like "cleanup CastNode construction" then I guess it's fine to do it all in once. But I leave it up to you to decide if you want to split it or not. Both is fine.

node

chhagedorn · 2023-12-21T07:06:52Z

src/hotspot/share/opto/compile.cpp

    // Make the CastII node dependent on the control input to prevent the narrowed ConvI2L
    // node from floating above the range check during loop optimizations. Otherwise, the
    // ConvI2L node may be eliminated independently of the range check, causing the data path
    // to become TOP while the control path is still there (although it's unreachable).


Might be cleaner to move this comment block above the creation of the cast node

chhagedorn

Otherwise, the cleanup looks good.

openjdk · 2023-12-21T07:11:18Z

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

8322490: cleanup CastNode construction

Reviewed-by: chagedorn, phh

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 67 new commits pushed to the master branch:

93fedc1: 8321802: (zipfs) Add validation of incorrect LOC signature in ZipFileSystem
1230853: 8322163: runtime/Unsafe/InternalErrorTest.java fails on Alpine after JDK-8320886
dce7a57: 8321683: Tests fail with AssertionError in RangeWithPageSize
c53f845: 8322539: Parallel: Remove duplicated methods in PSAdaptiveSizePolicy
84c2379: 8320139: [JVMCI] VmObjectAlloc is not generated by intrinsics methods which allocate objects
3b908c4: 8319795: Static huge pages are not used for CodeCache
1802601: 8293623: Simplify G1ConcurrentRefineThreadControl
6de23bf: 8322418: Problem list gc/TestAllocHumongousFragment.java subtests for 8298781
aff659a: 8322538: remove fatal from JVM_VirtualThread functions for !INCLUDE_JVMTI
05745e3: 8319548: Unexpected internal name for Filler array klass causes error in VisualVM
... and 57 more: https://git.openjdk.org/jdk/compare/c328f9589ddc3a981a2c63801bd991f8e593e69f...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@chhagedorn, @phohensee) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

caojoshua · 2023-12-21T17:31:20Z

/integrate

openjdk · 2023-12-21T17:32:37Z

@caojoshua
Your change (at version 69c796b) is now ready to be sponsored by a Committer.

phohensee · 2023-12-21T23:52:14Z

@chhagedorn, Josh applied your requested changes, please re-review.

caojoshua · 2023-12-22T00:42:20Z

@chhagedorn I changed my mind. Added the make_cast cleanups to this PR as well.

chhagedorn

Otherwise, the update looks good! You can directly integrate/get it sponsored afterward.

chhagedorn · 2023-12-22T09:19:48Z

src/hotspot/share/opto/castnode.cpp

-}
-
-Node* ConstraintCastNode::make(Node* c, Node *n, const Type *t, DependencyType dependency, BasicType bt) {
+Node* ConstraintCastNode::make_cast_for_basic_type(Node* c, Node *n, const Type *t, DependencyType dependency, BasicType bt) {


While at it, you can also fix the asterisk positions:

Suggested change

Node* ConstraintCastNode::make_cast_for_basic_type(Node* c, Node *n, const Type *t, DependencyType dependency, BasicType bt) {

Node* ConstraintCastNode::make_cast_for_basic_type(Node* c, Node* n, const Type* t, DependencyType dependency, BasicType bt) {

chhagedorn · 2023-12-22T09:23:33Z

src/hotspot/share/opto/library_call.cpp

@@ -1140,7 +1139,9 @@ bool LibraryCallKit::inline_preconditions_checkIndex(BasicType bt) {

  // length is now known positive, add a cast node to make this explicit
  jlong upper_bound = _gvn.type(length)->is_integer(bt)->hi_as_long();
-  Node* casted_length = ConstraintCastNode::make(control(), length, TypeInteger::make(0, upper_bound, Type::WidenMax, bt), ConstraintCastNode::RegularDependency, bt);
+  Node *casted_length = ConstraintCastNode::make_cast_for_basic_type(


Suggested change

Node *casted_length = ConstraintCastNode::make_cast_for_basic_type(

Node* casted_length = ConstraintCastNode::make_cast_for_basic_type(

chhagedorn · 2023-12-22T09:23:42Z

src/hotspot/share/opto/library_call.cpp

@@ -1168,7 +1169,9 @@ bool LibraryCallKit::inline_preconditions_checkIndex(BasicType bt) {
  }

  // index is now known to be >= 0 and < length, cast it
-  Node* result = ConstraintCastNode::make(control(), index, TypeInteger::make(0, upper_bound, Type::WidenMax, bt), ConstraintCastNode::RegularDependency, bt);
+  Node *result = ConstraintCastNode::make_cast_for_basic_type(


Suggested change

Node *result = ConstraintCastNode::make_cast_for_basic_type(

Node* result = ConstraintCastNode::make_cast_for_basic_type(

chhagedorn · 2023-12-22T09:24:10Z

src/hotspot/share/opto/loopTransform.cpp

@@ -3422,7 +3420,10 @@ bool IdealLoopTree::do_remove_empty_loop(PhaseIdealLoop *phase) {
  Node* exact_limit = phase->exact_limit(this);

  // We need to pin the exact limit to prevent it from floating above the zero trip guard.
-  Node* cast_ii = ConstraintCastNode::make(cl->in(LoopNode::EntryControl), exact_limit, phase->_igvn.type(exact_limit), ConstraintCastNode::UnconditionalDependency, T_INT);
+  Node *cast_ii = ConstraintCastNode::make_cast_for_basic_type(


Suggested change

Node *cast_ii = ConstraintCastNode::make_cast_for_basic_type(

Node* cast_ii = ConstraintCastNode::make_cast_for_basic_type(

chhagedorn · 2023-12-22T09:25:36Z

src/hotspot/share/opto/castnode.hpp

@@ -68,8 +69,7 @@ class ConstraintCastNode: public TypeNode {
  virtual bool depends_only_on_test() const { return _dependency == RegularDependency; }
  bool carry_dependency() const { return _dependency != RegularDependency; }
  TypeNode* dominating_cast(PhaseGVN* gvn, PhaseTransform* pt) const;
-  static Node* make_cast(int opcode, Node* c, Node* n, const Type* t, DependencyType dependency, const TypeTuple* extra_types);
-  static Node* make(Node* c, Node *n, const Type *t, DependencyType dependency, BasicType bt);
+  static Node* make_cast_for_basic_type(Node* c, Node *n, const Type *t, DependencyType dependency, BasicType bt);


Suggested change

static Node* make_cast_for_basic_type(Node* c, Node *n, const Type *t, DependencyType dependency, BasicType bt);

static Node* make_cast_for_basic_type(Node* c, Node* n, const Type* t, DependencyType dependency, BasicType bt);

caojoshua · 2023-12-22T19:05:10Z

/integrate

openjdk · 2023-12-22T19:06:32Z

@caojoshua
Your change (at version d3bd0b8) is now ready to be sponsored by a Committer.

phohensee · 2023-12-22T21:07:57Z

/sponsor

openjdk · 2023-12-22T21:08:46Z

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

f695ca5: 8321151: JDK-8294427 breaks Windows L&F on all older Windows versions
93fedc1: 8321802: (zipfs) Add validation of incorrect LOC signature in ZipFileSystem
1230853: 8322163: runtime/Unsafe/InternalErrorTest.java fails on Alpine after JDK-8320886
dce7a57: 8321683: Tests fail with AssertionError in RangeWithPageSize
c53f845: 8322539: Parallel: Remove duplicated methods in PSAdaptiveSizePolicy
84c2379: 8320139: [JVMCI] VmObjectAlloc is not generated by intrinsics methods which allocate objects
3b908c4: 8319795: Static huge pages are not used for CodeCache
1802601: 8293623: Simplify G1ConcurrentRefineThreadControl
6de23bf: 8322418: Problem list gc/TestAllocHumongousFragment.java subtests for 8298781
aff659a: 8322538: remove fatal from JVM_VirtualThread functions for !INCLUDE_JVMTI
... and 58 more: https://git.openjdk.org/jdk/compare/c328f9589ddc3a981a2c63801bd991f8e593e69f...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-12-22T21:08:53Z

@phohensee @caojoshua Pushed as commit 7263e25.

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

8322490: CastNode constructors accepts control node as input

3c35568

openjdk bot added the rfr Pull request is ready for review label Dec 19, 2023

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Dec 19, 2023

chhagedorn suggested changes Dec 20, 2023

View reviewed changes

Convert some CastIINode instantiations to use the constructor with ctrl

d9a40fd

node

chhagedorn reviewed Dec 21, 2023

View reviewed changes

chhagedorn approved these changes Dec 21, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 21, 2023

Move comment

69c796b

openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 21, 2023

phohensee approved these changes Dec 21, 2023

View reviewed changes

Cleanup make_cast functions

574617e

caojoshua changed the title ~~8322490: CastNode constructors accepts control node as input~~ 8322490: cleanup CastNode construction Dec 22, 2023

openjdk bot removed the sponsor Pull request is ready to be sponsored label Dec 22, 2023

chhagedorn approved these changes Dec 22, 2023

View reviewed changes

Fix asterisk formatting

d3bd0b8

openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 22, 2023

phohensee approved these changes Dec 22, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Dec 22, 2023

openjdk bot closed this Dec 22, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Dec 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8322490: cleanup CastNode construction #17162

8322490: cleanup CastNode construction #17162

caojoshua commented Dec 19, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 19, 2023

openjdk bot commented Dec 19, 2023

mlbridge bot commented Dec 19, 2023 •

edited

Loading

chhagedorn left a comment

chhagedorn Dec 20, 2023

chhagedorn Dec 20, 2023

caojoshua Dec 21, 2023

chhagedorn Dec 21, 2023

chhagedorn Dec 21, 2023

chhagedorn left a comment

openjdk bot commented Dec 21, 2023 •

edited

Loading

caojoshua commented Dec 21, 2023

openjdk bot commented Dec 21, 2023

phohensee commented Dec 21, 2023

caojoshua commented Dec 22, 2023

chhagedorn left a comment •

edited

Loading

chhagedorn Dec 22, 2023

chhagedorn Dec 22, 2023

chhagedorn Dec 22, 2023

chhagedorn Dec 22, 2023

chhagedorn Dec 22, 2023

caojoshua commented Dec 22, 2023

openjdk bot commented Dec 22, 2023

phohensee commented Dec 22, 2023

openjdk bot commented Dec 22, 2023

openjdk bot commented Dec 22, 2023

	main_limit = new CastIINode(main_limit, TypeInt::make(upward ? min_jint : orig_limit_t->_lo,
	upward ? orig_limit_t->_hi : max_jint, Type::WidenMax));
	main_limit->init_req(0, pre_ctrl);

		@@ -126,37 +126,20 @@ uint ConstraintCastNode::size_of() const {
		Node* ConstraintCastNode::make_cast(int opcode, Node* c, Node* n, const Type* t, DependencyType dependency,

	Node* ConstraintCastNode::make_cast_for_basic_type(Node* c, Node n, const Type t, DependencyType dependency, BasicType bt) {
	Node* ConstraintCastNode::make_cast_for_basic_type(Node* c, Node* n, const Type* t, DependencyType dependency, BasicType bt) {

	Node *casted_length = ConstraintCastNode::make_cast_for_basic_type(
	Node* casted_length = ConstraintCastNode::make_cast_for_basic_type(

	Node *result = ConstraintCastNode::make_cast_for_basic_type(
	Node* result = ConstraintCastNode::make_cast_for_basic_type(

	Node *cast_ii = ConstraintCastNode::make_cast_for_basic_type(
	Node* cast_ii = ConstraintCastNode::make_cast_for_basic_type(

	static Node* make_cast_for_basic_type(Node* c, Node n, const Type t, DependencyType dependency, BasicType bt);
	static Node* make_cast_for_basic_type(Node* c, Node* n, const Type* t, DependencyType dependency, BasicType bt);

8322490: cleanup CastNode construction #17162

8322490: cleanup CastNode construction #17162

Conversation

caojoshua commented Dec 19, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Dec 19, 2023

openjdk bot commented Dec 19, 2023

mlbridge bot commented Dec 19, 2023 • edited Loading

Webrevs

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

chhagedorn left a comment

Choose a reason for hiding this comment

openjdk bot commented Dec 21, 2023 • edited Loading

caojoshua commented Dec 21, 2023

openjdk bot commented Dec 21, 2023

phohensee commented Dec 21, 2023

caojoshua commented Dec 22, 2023

chhagedorn left a comment • edited Loading

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

caojoshua commented Dec 22, 2023

openjdk bot commented Dec 22, 2023

phohensee commented Dec 22, 2023

openjdk bot commented Dec 22, 2023

openjdk bot commented Dec 22, 2023

caojoshua commented Dec 19, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 19, 2023 •

edited

Loading

openjdk bot commented Dec 21, 2023 •

edited

Loading

chhagedorn left a comment •

edited

Loading