Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
17 changes: 15 additions & 2 deletions src/hotspot/share/opto/callnode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1306,7 +1306,20 @@ void CallLeafVectorNode::calling_convention( BasicType* sig_bt, VMRegPair *parm_
bool CallLeafPureNode::is_unused() const {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a quick comment why this check implies that the node is not used, i.e. what that means?

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 think i'll need you to explain to me what is unclear at the moment. When I read the function, I see:
"A CallLeafPure is unused iff there is no output result projection."

I don't see what else to add that is not covered by "if we don't use the result, the pure call is unused", which is exactly the code. Is there any untold hypothesis lurking somewhere that I don't see? It seems to me it uses just very common concepts of C2.

Copy link
Contributor

Choose a reason for hiding this comment

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

The call could have other uses for other projections. Why does this projection make it unused?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose I was not aware that TypeFunc::Parms stands for result projection.... the name does not make it immediately apparent.

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 see. I think I'd be better to comment on the declaration of the class then. Something saying that CallLeafPureNode represents calls that are pure: they only have data input and output data (and control for practical reasons for now), no exceptions, no memory, no safepoint... They can be freely be moved around, duplicated or, if the result isn't used, removed. Then that explains... a lot of what we are doing, not just is_unused.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I was really just confused about the Parms. I thought that means parameters .. and not results 🤣

Copy link
Member Author

Choose a reason for hiding this comment

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

It's actually both. For functions, the parameters are starting at Parms, and the results too. Before that, it's all the special stuff: control, memory, io...

return proj_out_or_null(TypeFunc::Parms) == nullptr;
}
// We make a tuple of the global input state + TOP for the output values.

bool CallLeafPureNode::is_dead() const {
return proj_out_or_null(TypeFunc::Control) == nullptr;
}

/* We make a tuple of the global input state + TOP for the output values.
* We use this to delete a pure function that is not used: by replacing the call with
* such a tuple, we let output Proj's idealization pick the corresponding input of the
* pure call, so jumping over it, and effectively, removing the call from the graph.
* This avoids doing the graph surgery manually, but leave that to IGVN
* that is specialized for doing that right. We need also tuple components for output
* values of the function to respect the return arity, and in case there is a projection
* that would pick an output (which shouldn't happen at the moment).
*/
TupleNode* CallLeafPureNode::make_tuple_of_input_state_and_top_return_values(const Compile* C) const {
// Transparently propagate input state but parameters
TupleNode* tuple = TupleNode::make(
Expand All @@ -1326,7 +1339,7 @@ TupleNode* CallLeafPureNode::make_tuple_of_input_state_and_top_return_values(con
}

Node* CallLeafPureNode::Ideal(PhaseGVN* phase, bool can_reshape) {
if (proj_out_or_null(TypeFunc::Control) == nullptr) { // dead node
if (is_dead()) { // dead node
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (is_dead()) { // dead node
if (is_dead()) {

The comment seemed redundant. You could say who else is responsible of cleaning up the dead node though.

What would happen if the CallLeafPureNode loses its control projection, but not the other uses? I don't even know if that is possible. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

IGVN takes care of removing a node without output.

This was motivated by https://bugs.openjdk.org/browse/JDK-8353341 I think @TobiHartmann told me it's common not to touch dead nodes during idealization.

I think it is possible to lose control projection but not data because data was already found dead, but data wasn't yet (but should happen shortly after). As soon as the data projection disappear, the node should be removed. remove_dead_node (used in IGVN) is aggressively removing a dead node and all the usages that become recursively dead. It's the classic constraint of having to find that both data and control are top when one is.

return nullptr;
}

Expand Down
1 change: 1 addition & 0 deletions src/hotspot/share/opto/callnode.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -916,6 +916,7 @@ class CallLeafNode : public CallRuntimeNode {
class CallLeafPureNode : public CallLeafNode {
Copy link
Contributor

Choose a reason for hiding this comment

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

You need a short description about what this node is for. What are the assumptions about it?

protected:
bool is_unused() const;
bool is_dead() const;
TupleNode* make_tuple_of_input_state_and_top_return_values(const Compile* C) const;

public:
Expand Down
5 changes: 3 additions & 2 deletions src/hotspot/share/opto/compile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3299,6 +3299,7 @@ void Compile::final_graph_reshaping_main_switch(Node* n, Final_Reshape_Counts& f
n->subsume_by(n->in(1), this);
break;
case Op_CallLeafPure: {
// If the pure call is not supported, then lower to a CallLeaf.
if (!Matcher::match_rule_supported(Op_CallLeafPure)) {
CallNode* call = n->as_Call();
CallNode* new_call = new CallLeafNode(call->tf(), call->entry_point(),
Expand All @@ -3308,8 +3309,8 @@ void Compile::final_graph_reshaping_main_switch(Node* n, Final_Reshape_Counts& f
new_call->init_req(TypeFunc::Memory, C->top());
new_call->init_req(TypeFunc::ReturnAdr, C->top());
new_call->init_req(TypeFunc::FramePtr, C->top());
for (unsigned int i = 0; i < call->tf()->domain()->cnt() - TypeFunc::Parms; i++) {
new_call->init_req(TypeFunc::Parms + i, call->in(TypeFunc::Parms + i));
for (unsigned int i = TypeFunc::Parms; i < call->tf()->domain()->cnt(); i++) {
new_call->init_req(i, call->in(i));
}
n->subsume_by(new_call, this);
}
Expand Down
4 changes: 2 additions & 2 deletions src/hotspot/share/opto/divnode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1520,7 +1520,7 @@ Node* ModFNode::Ideal(PhaseGVN* phase, bool can_reshape) {
if (!can_reshape) {
return nullptr;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this prevent us from doing the make_tuple_of_input_state_and_top_return_values trick? Because it seems to me that we do not need to reshape the node for that, right? Maybe you should reorder things for that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, you should probably call CallLeafPureNode::Ideal instead of duplicating its logic here and in other subclasses.

Copy link
Member Author

Choose a reason for hiding this comment

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

In an earlier issue (JDK-8349523), I tried to remove these nodes during parsing. It didn't work well. The problem is that it's transformed by GVN before setting any output projection, so of course, the node is removing itself before having the opportunity of being used. We wait until usages have been set before we try to remove the node (or replace it with a tuple).

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds like a bug at the use-site... don't you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed offline, it's relatively normal. Adding a comment to explain.

if (proj_out_or_null(TypeFunc::Control) == nullptr) { // dead node
if (is_dead()) { // dead node
return nullptr;
}

Expand Down Expand Up @@ -1574,7 +1574,7 @@ Node* ModDNode::Ideal(PhaseGVN* phase, bool can_reshape) {
if (!can_reshape) {
return nullptr;
}
if (proj_out_or_null(TypeFunc::Control) == nullptr) { // dead node
if (is_dead()) { // dead node
return nullptr;
}

Expand Down
2 changes: 2 additions & 0 deletions src/hotspot/share/opto/graphKit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1912,6 +1912,8 @@ void GraphKit::set_predefined_output_for_runtime_call(Node* call,
// no i/o
set_control(_gvn.transform( new ProjNode(call,TypeFunc::Control) ));
if (call->is_CallLeafPure()) {
// Pure function have only control (for now) and data output, in particular
// the don't touch the memory, so we don't want a memory proj that is set after.
return;
}
if (keep_mem) {
Expand Down
21 changes: 20 additions & 1 deletion src/hotspot/share/opto/multnode.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,22 @@ class ProjNode : public Node {
ProjNode* other_if_proj() const;
};

//------------------------------TupleNode---------------------------------------
/* Tuples are used to avoid manual graph surgery. When a node with Proj outputs (such as a call)
* must be removed and its ouputs replaced by its input, or some other value, we can make its
* ::Ideal return a tuple of what we want for each output: the ::Identity of output Proj will
* take care to jump over the Tuple and directly pick up the right input of the Tuple.
*
* For instance, if a function call is proven to have no side effect and return the constant 0,
* we can replace it with the 6-tuple:
* (control input, IO input, memory input, frame ptr input, return addr input, Con:0)
* all the output projections will pick up the input of the now gone call, except for the result
* projection that is replaced by 0.
*
* Using TupleNode avoid manual graph surgery and leave that to our expert surgeon: IGVN.
* Since the user of a Tuple are expected to be Proj, when creating a tuple during idealization,
* the output Proj should be enqueued for IGVN immediately after, and the tuple should not survive
* after the current IGVN.
*/
class TupleNode : public MultiNode {
const TypeTuple* _tf;

Expand All @@ -124,6 +139,10 @@ class TupleNode : public MultiNode {
int Opcode() const override;
const Type* bottom_type() const override { return _tf; }

/* Give as many `Node*` as you want in the `nn` pack:
* TupleNode::make(tf, input1)
* TupleNode::make(tf, input1, input2, input3, input4)
*/
template <typename... NN>
static TupleNode* make(const TypeTuple* tf, NN... nn) {
TupleNode* tn = new TupleNode(tf);
Expand Down