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
8 changes: 6 additions & 2 deletions src/hotspot/share/opto/callnode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1315,7 +1315,7 @@ bool CallLeafPureNode::is_dead() const {
* 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
* This avoids doing the graph surgery manually, but leaves 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).
Expand All @@ -1339,11 +1339,15 @@ TupleNode* CallLeafPureNode::make_tuple_of_input_state_and_top_return_values(con
}

Node* CallLeafPureNode::Ideal(PhaseGVN* phase, bool can_reshape) {
if (is_dead()) { // dead node
if (is_dead()) {
return nullptr;
}

// We need to wait until IGVN because during parsing, usages might still be missing
// and we would remove the call immediately.
if (can_reshape && is_unused()) {
// The result is not used. We remove the call by replacing it with a tuple, that
// is later disintegrated by the projections.
return make_tuple_of_input_state_and_top_return_values(phase->C);
}

Expand Down
11 changes: 11 additions & 0 deletions src/hotspot/share/opto/callnode.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -913,6 +913,17 @@ class CallLeafNode : public CallRuntimeNode {
#endif
};

/* A pure function call, they are assumed not to be safepoints, not to read or write memory,
* have no exception... They just take parameters, return a value without side effect. It is
* always correct to create some, or remove them, if the result is not used.
*
* They still have control input to allow easy lowering into other kind of calls that require
* a control, but this is more a technical than a moral constraint.
*
* Pure calls must have only control and data input and output: I/O, Memory and so on must be top.
* Nevertheless, pure calls can typically be expensive math operations so care must be taken
* when letting the node float.
*/
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;
Expand Down
24 changes: 10 additions & 14 deletions src/hotspot/share/opto/divnode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1517,19 +1517,17 @@ const Type* UModLNode::Value(PhaseGVN* phase) const {
}

Node* ModFNode::Ideal(PhaseGVN* phase, bool can_reshape) {
if (!can_reshape) {
return nullptr;
Node* super = CallLeafPureNode::Ideal(phase, can_reshape);
if (super != nullptr) {
return super;
Copy link
Member

Choose a reason for hiding this comment

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

Can't we just do return CallLeafPureNode::Ideal(phase, can_reshape); at the end of ModFNode::Ideal instead of return nullptr? That's what we usually do in C2, for example in CallStaticJavaNode::Ideal -> CallNode::Ideal. Feels more natural to me and would avoid the super != nullptr check. Also for the other Ideal methods that you modified.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could but it's not that direct. ModFNode::Ideal has 6 returns (without mine):

  • 3 are return replace_with_con(...); which in their turn return nullptr but after making changes in the graph.
  • 2 are return nullptr;
  • 1 is actually returning a node.
    And especially the final one is
return replace_with_con(igvn, TypeF::make(jfloat_cast(xr)));

If we change replace_with_con to actually return a TupleNode to do the job, we still have 2 places where to call the base class' Ideal. So I'm not sure how much better it would be to duplicate the call. It also adds a maintenance burden: if one adds another case where we don't want to make changes, one needs to add another call to CallLeafPureNode::Ideal. I think it's because of the structure of this function: rather than selecting cases where we want to do something and reaching the end with only the leftover cases, we select the cases we don't want to continue with, and we return early, making more cases where we should call the super method.

I'll try something.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, makes sense. Feel free to leave as-is then.

Copy link
Member Author

Choose a reason for hiding this comment

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

There, you can see the thing I've tried. It changes a bit more code, but overall, I think it makes it clearer and address your comment.

}
if (is_dead()) { // dead node

if (!can_reshape) {
return nullptr;
}

PhaseIterGVN* igvn = phase->is_IterGVN();

if (is_unused()) {
return make_tuple_of_input_state_and_top_return_values(igvn->C);
}

// Either input is TOP ==> the result is TOP
const Type* t1 = phase->type(dividend());
const Type* t2 = phase->type(divisor());
Expand Down Expand Up @@ -1571,19 +1569,17 @@ Node* ModFNode::Ideal(PhaseGVN* phase, bool can_reshape) {
}

Node* ModDNode::Ideal(PhaseGVN* phase, bool can_reshape) {
if (!can_reshape) {
return nullptr;
Node* super = CallLeafPureNode::Ideal(phase, can_reshape);
if (super != nullptr) {
return super;
}
if (is_dead()) { // dead node

if (!can_reshape) {
return nullptr;
}

PhaseIterGVN* igvn = phase->is_IterGVN();

if (is_unused()) {
return make_tuple_of_input_state_and_top_return_values(igvn->C);
}

// Either input is TOP ==> the result is TOP
const Type* t1 = phase->type(dividend());
const Type* t2 = phase->type(divisor());
Expand Down