Skip to content

8347481: C2: Remove the control input of some nodes #23055

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

8347481: C2: Remove the control input of some nodes #23055

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/hotspot/share/opto/addnode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1997, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1997, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -1270,7 +1270,7 @@ Node* MaxNode::build_min_max(Node* a, Node* b, bool is_max, bool is_unsigned, co
cmp = gvn.transform(CmpNode::make(b, a, bt, is_unsigned));
}
Node* bol = gvn.transform(new BoolNode(cmp, BoolTest::lt));
res = gvn.transform(CMoveNode::make(nullptr, bol, a, b, t));
res = gvn.transform(CMoveNode::make(bol, a, b, t));
}
if (hook != nullptr) {
hook->destruct(&gvn);
Expand Down Expand Up @@ -1299,7 +1299,7 @@ Node* MaxNode::build_min_max_diff_with_zero(Node* a, Node* b, bool is_max, const
}
Node* sub = gvn.transform(SubNode::make(a, b, bt));
Node* bol = gvn.transform(new BoolNode(cmp, BoolTest::lt));
Node* res = gvn.transform(CMoveNode::make(nullptr, bol, sub, zero, t));
Node* res = gvn.transform(CMoveNode::make(bol, sub, zero, t));
if (hook != nullptr) {
hook->destruct(&gvn);
}
Expand Down
30 changes: 15 additions & 15 deletions src/hotspot/share/opto/library_call.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1999, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1999, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -2140,18 +2140,18 @@ bool LibraryCallKit::inline_number_methods(vmIntrinsics::ID id) {
Node* arg = argument(0);
Node* n = nullptr;
switch (id) {
case vmIntrinsics::_numberOfLeadingZeros_i: n = new CountLeadingZerosINode( arg); break;
case vmIntrinsics::_numberOfLeadingZeros_l: n = new CountLeadingZerosLNode( arg); break;
case vmIntrinsics::_numberOfTrailingZeros_i: n = new CountTrailingZerosINode(arg); break;
case vmIntrinsics::_numberOfTrailingZeros_l: n = new CountTrailingZerosLNode(arg); break;
case vmIntrinsics::_bitCount_i: n = new PopCountINode( arg); break;
case vmIntrinsics::_bitCount_l: n = new PopCountLNode( arg); break;
case vmIntrinsics::_reverseBytes_c: n = new ReverseBytesUSNode(nullptr, arg); break;
case vmIntrinsics::_reverseBytes_s: n = new ReverseBytesSNode( nullptr, arg); break;
case vmIntrinsics::_reverseBytes_i: n = new ReverseBytesINode( nullptr, arg); break;
case vmIntrinsics::_reverseBytes_l: n = new ReverseBytesLNode( nullptr, arg); break;
case vmIntrinsics::_reverse_i: n = new ReverseINode(nullptr, arg); break;
case vmIntrinsics::_reverse_l: n = new ReverseLNode(nullptr, arg); break;
case vmIntrinsics::_numberOfLeadingZeros_i: n = new CountLeadingZerosINode( arg); break;
case vmIntrinsics::_numberOfLeadingZeros_l: n = new CountLeadingZerosLNode( arg); break;
case vmIntrinsics::_numberOfTrailingZeros_i: n = new CountTrailingZerosINode(arg); break;
case vmIntrinsics::_numberOfTrailingZeros_l: n = new CountTrailingZerosLNode(arg); break;
case vmIntrinsics::_bitCount_i: n = new PopCountINode( arg); break;
case vmIntrinsics::_bitCount_l: n = new PopCountLNode( arg); break;
case vmIntrinsics::_reverseBytes_c: n = new ReverseBytesUSNode( arg); break;
case vmIntrinsics::_reverseBytes_s: n = new ReverseBytesSNode( arg); break;
case vmIntrinsics::_reverseBytes_i: n = new ReverseBytesINode( arg); break;
case vmIntrinsics::_reverseBytes_l: n = new ReverseBytesLNode( arg); break;
case vmIntrinsics::_reverse_i: n = new ReverseINode( arg); break;
case vmIntrinsics::_reverse_l: n = new ReverseLNode( arg); break;
default: fatal_unexpected_iid(id); break;
}
set_result(_gvn.transform(n));
Expand Down Expand Up @@ -8264,14 +8264,14 @@ bool LibraryCallKit::inline_fma(vmIntrinsics::ID id) {
a = round_double_node(argument(0));
b = round_double_node(argument(2));
c = round_double_node(argument(4));
result = _gvn.transform(new FmaDNode(control(), a, b, c));
result = _gvn.transform(new FmaDNode(a, b, c));
break;
case vmIntrinsics::_fmaF:
assert(callee()->signature()->size() == 3, "fma has 3 parameters of size 1 each.");
a = argument(0);
b = argument(1);
c = argument(2);
result = _gvn.transform(new FmaFNode(control(), a, b, c));
result = _gvn.transform(new FmaFNode(a, b, c));
break;
default:
fatal_unexpected_iid(id); break;
Expand Down
8 changes: 4 additions & 4 deletions src/hotspot/share/opto/loopopts.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1999, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1999, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -857,9 +857,9 @@ Node *PhaseIdealLoop::conditional_move( Node *region ) {
}
}
}
Node *cmov = CMoveNode::make(cmov_ctrl, iff->in(1), phi->in(1+flip), phi->in(2-flip), _igvn.type(phi));
register_new_node( cmov, cmov_ctrl );
_igvn.replace_node( phi, cmov );
Node* cmov = CMoveNode::make(iff->in(1), phi->in(1+flip), phi->in(2-flip), _igvn.type(phi));
register_new_node(cmov, cmov_ctrl);
_igvn.replace_node(phi, cmov);
#ifndef PRODUCT
if (TraceLoopOpts) {
tty->print("CMOV ");
Expand Down
22 changes: 11 additions & 11 deletions src/hotspot/share/opto/movenode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2014, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2014, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -102,7 +102,7 @@ Node *CMoveNode::Ideal(PhaseGVN *phase, bool can_reshape) {
// Canonicalize the node by moving constants to the right input.
if (in(Condition)->is_Bool() && phase->type(in(IfFalse))->singleton() && !phase->type(in(IfTrue))->singleton()) {
BoolNode* b = in(Condition)->as_Bool()->negate(phase);
return make(in(Control), phase->transform(b), in(IfTrue), in(IfFalse), _type);
return make(phase->transform(b), in(IfTrue), in(IfFalse), _type);
}

return nullptr;
Expand Down Expand Up @@ -186,15 +186,15 @@ const Type* CMoveNode::Value(PhaseGVN* phase) const {
//------------------------------make-------------------------------------------
// Make a correctly-flavored CMove. Since _type is directly determined
// from the inputs we do not need to specify it here.
CMoveNode *CMoveNode::make(Node *c, Node *bol, Node *left, Node *right, const Type *t) {
CMoveNode* CMoveNode::make(Node* bol, Node* left, Node* right, const Type* t) {
switch( t->basic_type() ) {
case T_INT: return new CMoveINode( bol, left, right, t->is_int() );
case T_FLOAT: return new CMoveFNode( bol, left, right, t );
case T_DOUBLE: return new CMoveDNode( bol, left, right, t );
case T_LONG: return new CMoveLNode( bol, left, right, t->is_long() );
case T_OBJECT: return new CMovePNode( c, bol, left, right, t->is_oopptr() );
case T_ADDRESS: return new CMovePNode( c, bol, left, right, t->is_ptr() );
case T_NARROWOOP: return new CMoveNNode( c, bol, left, right, t );
case T_INT: return new CMoveINode(bol, left, right, t->is_int());
case T_FLOAT: return new CMoveFNode(bol, left, right, t);
case T_DOUBLE: return new CMoveDNode(bol, left, right, t);
case T_LONG: return new CMoveLNode(bol, left, right, t->is_long());
case T_OBJECT: return new CMovePNode(bol, left, right, t->is_oopptr());
case T_ADDRESS: return new CMovePNode(bol, left, right, t->is_ptr());
case T_NARROWOOP: return new CMoveNNode(bol, left, right, t);
default:
ShouldNotReachHere();
return nullptr;
Expand Down Expand Up @@ -278,7 +278,7 @@ Node *CMoveINode::Ideal(PhaseGVN *phase, bool can_reshape) {
if( in(Condition)->is_Bool() ) {
BoolNode* b = in(Condition)->as_Bool();
BoolNode* b2 = b->negate(phase);
return make(in(Control), phase->transform(b2), in(IfTrue), in(IfFalse), _type);
return make(phase->transform(b2), in(IfTrue), in(IfFalse), _type);
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/hotspot/share/opto/movenode.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2014, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2014, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -47,7 +47,7 @@ class CMoveNode : public TypeNode {
virtual Node *Ideal(PhaseGVN *phase, bool can_reshape);
virtual const Type* Value(PhaseGVN* phase) const;
virtual Node* Identity(PhaseGVN* phase);
static CMoveNode *make(Node *c, Node *bol, Node *left, Node *right, const Type *t);
static CMoveNode* make(Node* bol, Node* left, Node* right, const Type* t);
// Helper function to spot cmove graph shapes
static Node* is_cmove_id(PhaseTransform* phase, Node* cmp, Node* t, Node* f, BoolNode* b);
static Node* Ideal_minmax(PhaseGVN* phase, CMoveNode* cmov);
Expand Down Expand Up @@ -87,14 +87,14 @@ class CMoveLNode : public CMoveNode {
//------------------------------CMovePNode-------------------------------------
class CMovePNode : public CMoveNode {
public:
CMovePNode( Node *c, Node *bol, Node *left, Node *right, const TypePtr* t ) : CMoveNode(bol,left,right,t) { init_req(Control,c); }
CMovePNode(Node* bol, Node* left, Node* right, const TypePtr* t) : CMoveNode(bol, left, right, t) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked on history and this control setting was added at the very beginning.
May be it was done to avoid bypassing safepoints where oops could be modified (objects moved) and result of CMoveP before and after safepoint will be different.
I would be careful about changing it and CMoveN.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have thought about it and can't think of a reason for the control input.
Firstly, the oop map is created after scheduling and should take care of this seemlessly.
Secondly, the control input does not prevent a CMoveP from moving down past a safepoint. And since these nodes have depends_only_on_test == true, they can be moved from a test to an equivalent dominating test, skipping any safepoint in between, which means these nodes can be moved up past a safepoint, too.
As a result, I don't think the control input of CMoveP and CMoveN is necessary and can be safely removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any undesireable performance effects if CMove node floats up beyond its (original) control? It may have changed on modern hardware, but my recollection is cmov instructions were quite expensive to execute.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iwanowww A cmov is as cheap as an add so I don't think there will be any performance effect. I believe what you mean by cmov being expensive is that it has 1 cycle of latency on both of its inputs, while a well-predicted if diamond has 0 cycle of latency on the chosen input only.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, my memory failed me. Indeed I was referring to relative slowness of cmov instructions compared to well-predicted conditional jumps which is not relevant here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a result, I don't think the control input of CMoveP and CMoveN is necessary and can be safely removed.

Okay. Let test it in JDK 25 and see how it goes. We have time to fix it if we find something wrong.
@dafedafe ran tier1-5 our testing and I don't see any new failures.

virtual int Opcode() const;
};

//------------------------------CMoveNNode-------------------------------------
class CMoveNNode : public CMoveNode {
public:
CMoveNNode( Node *c, Node *bol, Node *left, Node *right, const Type* t ) : CMoveNode(bol,left,right,t) { init_req(Control,c); }
CMoveNNode(Node* bol, Node* left, Node* right, const Type* t ) : CMoveNode(bol, left, right, t) {}
virtual int Opcode() const;
};

Expand Down
8 changes: 4 additions & 4 deletions src/hotspot/share/opto/mulnode.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1997, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1997, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -388,7 +388,7 @@ inline Node* make_urshift<TypeInt>(Node* a, Node* b) {
// fused-multiply-add
class FmaNode : public Node {
public:
FmaNode(Node* c, Node* in1, Node* in2, Node* in3) : Node(c, in1, in2, in3) {
FmaNode(Node* in1, Node* in2, Node* in3) : Node(nullptr, in1, in2, in3) {
assert(UseFMA, "Needs FMA instructions support.");
}
virtual Node* Ideal(PhaseGVN* phase, bool can_reshape);
Expand All @@ -398,7 +398,7 @@ class FmaNode : public Node {
// fused-multiply-add double
class FmaDNode : public FmaNode {
public:
FmaDNode(Node* c, Node* in1, Node* in2, Node* in3) : FmaNode(c, in1, in2, in3) {}
FmaDNode(Node* in1, Node* in2, Node* in3) : FmaNode(in1, in2, in3) {}
virtual int Opcode() const;
const Type* bottom_type() const { return Type::DOUBLE; }
virtual uint ideal_reg() const { return Op_RegD; }
Expand All @@ -409,7 +409,7 @@ class FmaDNode : public FmaNode {
// fused-multiply-add float
class FmaFNode : public FmaNode {
public:
FmaFNode(Node* c, Node* in1, Node* in2, Node* in3) : FmaNode(c, in1, in2, in3) {}
FmaFNode(Node* in1, Node* in2, Node* in3) : FmaNode(in1, in2, in3) {}
virtual int Opcode() const;
const Type* bottom_type() const { return Type::FLOAT; }
virtual uint ideal_reg() const { return Op_RegF; }
Expand Down
6 changes: 2 additions & 4 deletions src/hotspot/share/opto/subnode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1997, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1997, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -1416,9 +1416,7 @@ Node* BoolNode::make_predicate(Node* test_value, PhaseGVN* phase) {
//--------------------------------as_int_value---------------------------------
Node* BoolNode::as_int_value(PhaseGVN* phase) {
// Inverse to make_predicate. The CMove probably boils down to a Conv2B.
Node* cmov = CMoveNode::make(nullptr, this,
phase->intcon(0), phase->intcon(1),
TypeInt::BOOL);
Node* cmov = CMoveNode::make(this, phase->intcon(0), phase->intcon(1), TypeInt::BOOL);
return phase->transform(cmov);
}

Expand Down
26 changes: 13 additions & 13 deletions src/hotspot/share/opto/subnode.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1997, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1997, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -532,49 +532,49 @@ class SqrtFNode : public Node {
// reverse bytes of an integer
class ReverseBytesINode : public Node {
public:
ReverseBytesINode(Node *c, Node *in1) : Node(c, in1) {}
ReverseBytesINode(Node* in) : Node(nullptr, in) {}
virtual int Opcode() const;
const Type *bottom_type() const { return TypeInt::INT; }
const Type* bottom_type() const { return TypeInt::INT; }
virtual uint ideal_reg() const { return Op_RegI; }
};

//-------------------------------ReverseBytesLNode--------------------------------
// reverse bytes of a long
class ReverseBytesLNode : public Node {
public:
ReverseBytesLNode(Node *c, Node *in1) : Node(c, in1) {}
ReverseBytesLNode(Node* in) : Node(nullptr, in) {}
virtual int Opcode() const;
const Type *bottom_type() const { return TypeLong::LONG; }
const Type* bottom_type() const { return TypeLong::LONG; }
virtual uint ideal_reg() const { return Op_RegL; }
};

//-------------------------------ReverseBytesUSNode--------------------------------
// reverse bytes of an unsigned short / char
class ReverseBytesUSNode : public Node {
public:
ReverseBytesUSNode(Node *c, Node *in1) : Node(c, in1) {}
ReverseBytesUSNode(Node* in1) : Node(nullptr, in1) {}
virtual int Opcode() const;
const Type *bottom_type() const { return TypeInt::CHAR; }
const Type* bottom_type() const { return TypeInt::CHAR; }
virtual uint ideal_reg() const { return Op_RegI; }
};

//-------------------------------ReverseBytesSNode--------------------------------
// reverse bytes of a short
class ReverseBytesSNode : public Node {
public:
ReverseBytesSNode(Node *c, Node *in1) : Node(c, in1) {}
ReverseBytesSNode(Node* in) : Node(nullptr, in) {}
virtual int Opcode() const;
const Type *bottom_type() const { return TypeInt::SHORT; }
const Type* bottom_type() const { return TypeInt::SHORT; }
virtual uint ideal_reg() const { return Op_RegI; }
};

//-------------------------------ReverseINode--------------------------------
// reverse bits of an int
class ReverseINode : public Node {
public:
ReverseINode(Node *c, Node *in1) : Node(c, in1) {}
ReverseINode(Node* in) : Node(nullptr, in) {}
virtual int Opcode() const;
const Type *bottom_type() const { return TypeInt::INT; }
const Type* bottom_type() const { return TypeInt::INT; }
virtual uint ideal_reg() const { return Op_RegI; }
virtual Node* Identity(PhaseGVN* phase);
virtual const Type* Value(PhaseGVN* phase) const;
Expand All @@ -584,9 +584,9 @@ class ReverseINode : public Node {
// reverse bits of a long
class ReverseLNode : public Node {
public:
ReverseLNode(Node *c, Node *in1) : Node(c, in1) {}
ReverseLNode(Node* in) : Node(nullptr, in) {}
virtual int Opcode() const;
const Type *bottom_type() const { return TypeLong::LONG; }
const Type* bottom_type() const { return TypeLong::LONG; }
virtual uint ideal_reg() const { return Op_RegL; }
virtual Node* Identity(PhaseGVN* phase);
virtual const Type* Value(PhaseGVN* phase) const;
Expand Down