Skip to content

[SelectionDAG] Simplify classof of MemSDNode and MemIntrinsicSDNode (NFC) #115720

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

Merged
s-barannikov merged 1 commit into from
Nov 12, 2024
Merged

[SelectionDAG] Simplify classof of MemSDNode and MemIntrinsicSDNode (NFC) #115720

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
17 changes: 3 additions & 14 deletions llvm/include/llvm/CodeGen/SelectionDAGNodes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,15 +708,7 @@ END_TWO_BYTE_PACK()
bool isUndef() const { return NodeType == ISD::UNDEF; }

/// Test if this node is a memory intrinsic (with valid pointer information).
/// INTRINSIC_W_CHAIN and INTRINSIC_VOID nodes are sometimes created for
/// non-memory intrinsics (with chains) that are not really instances of
/// MemSDNode. For such nodes, we need some extra state to determine the
/// proper classof relationship.
bool isMemIntrinsic() const {
return (NodeType == ISD::INTRINSIC_W_CHAIN ||
NodeType == ISD::INTRINSIC_VOID) &&
SDNodeBits.IsMemIntrinsic;
}
bool isMemIntrinsic() const { return SDNodeBits.IsMemIntrinsic; }
Copy link
Collaborator

@topperc topperc Nov 11, 2024

Choose a reason for hiding this comment

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

Isn't this SDNodeBits part of a union? Isn't the opcode check to make sure that part of the union is active?

Copy link
Contributor Author

@s-barannikov s-barannikov Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

structs other than SDNodeBitfields have first NumSDNodeBits bits reserved.
That is, IsMemIntrinsic is shared between all structs in the union.

I think the opcode check is a historical artifact.

Copy link
Contributor Author

@s-barannikov s-barannikov Nov 11, 2024

Choose a reason for hiding this comment

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

Note that this opcode check doesn't really tell us which union member is active because INTRINSIC_W_CHAIN / INTRINSIC_VOID can either be an ordinary SDNode or MemIntrinsicSDNode. If I read the removed comment correctly, this bit was introduced to disambiguate these cases, but introducing it also made the opcode check redundant.

Assuming there is no (pre-existing) undefined behavior, just checking the bit is more robust as the method will now return false for ordinary SDNodes erroneously created with isTargetMemoryOpcode opcodes.

Copy link
Collaborator

@topperc topperc Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the clarification.

Assuming there is no (pre-existing) undefined behavior, just checking the bit is more robust as the method will now return false for ordinary SDNodes erroneously created with isTargetMemoryOpcode opcodes.

I think that would be considered a bug to not use SDMemIntrinsicSDNode for anything that isTargetMemoryOpcode()


/// Test if this node is a strict floating point pseudo-op.
bool isStrictFPOpcode() {
Expand Down Expand Up @@ -1464,7 +1456,6 @@ class MemSDNode : public SDNode {
switch (N->getOpcode()) {
case ISD::LOAD:
case ISD::STORE:
case ISD::PREFETCH:
case ISD::ATOMIC_CMP_SWAP:
case ISD::ATOMIC_CMP_SWAP_WITH_SUCCESS:
case ISD::ATOMIC_SWAP:
Expand Down Expand Up @@ -1504,7 +1495,7 @@ class MemSDNode : public SDNode {
case ISD::EXPERIMENTAL_VECTOR_HISTOGRAM:
return true;
default:
return N->isMemIntrinsic() || N->isTargetMemoryOpcode();
return N->isMemIntrinsic();
}
}
};
Expand Down Expand Up @@ -1596,9 +1587,7 @@ class MemIntrinsicSDNode : public MemSDNode {
static bool classof(const SDNode *N) {
// We lower some target intrinsics to their target opcode
// early a node with a target opcode can be of this class
return N->isMemIntrinsic() ||
N->getOpcode() == ISD::PREFETCH ||
N->isTargetMemoryOpcode();
return N->isMemIntrinsic();
}
};

Expand Down
Loading