Skip to content
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.

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

[DAGCombiner] Turn (neg (max x, (neg x))) into (min x, (neg x)) #120666

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
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
2 changes: 2 additions & 0 deletions llvm/include/llvm/CodeGen/ISDOpcodes.h
Original file line number Diff line number Diff line change
Expand Up @@ -1506,6 +1506,8 @@ inline bool isBitwiseLogicOp(unsigned Opcode) {
return Opcode == ISD::AND || Opcode == ISD::OR || Opcode == ISD::XOR;
}

NodeType getInverseMinMaxOpcode(unsigned MinMaxOpc);
Copy link
Contributor

Choose a reason for hiding this comment

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

Document

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


/// Get underlying scalar opcode for VECREDUCE opcode.
/// For example ISD::AND for ISD::VECREDUCE_AND.
NodeType getVecReduceBaseOpcode(unsigned VecReduceOpcode);
Expand Down
18 changes: 18 additions & 0 deletions llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3949,6 +3949,24 @@ SDValue DAGCombiner::visitSUB(SDNode *N) {
if (SDValue Result = TLI.expandABS(N1.getNode(), DAG, true))
return Result;

// Similar to the previous rule, but this time targeting an expanded abs.
// (sub 0, (max X, (sub 0, X))) --> (min X, (sub 0, X))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could also do the converse:

(sub 0, (min X, (sub 0, X))) --> (max X, (sub 0, X))

but I don't know if there's a real need for it.

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 it's fine to add it in this patch. It's done now.

// as well as
// (sub 0, (min X, (sub 0, X))) --> (max X, (sub 0, X))
// Note that these two are applicable to both signed and unsigned min/max.
SDValue X;
SDValue S0;
auto NegPat = m_AllOf(m_Neg(m_Deferred(X)), m_Value(S0));
if (LegalOperations &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically you should check the max is legal, but I doubt in practice the legality of min is different than max

Copy link
Member Author

@mshockwave mshockwave Dec 23, 2024

Choose a reason for hiding this comment

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

I doubt in practice the legality of min is different than max

This, and even if only max is illegal, I think it'll be expanded (while min is not), which makes this transformation even more profitable.

Copy link
Contributor

Choose a reason for hiding this comment

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

It depends in which combiner phase, eventually only legal operations can be emitted

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, if you're referring to the max generated by this rule, I already checked its legality (through NewOpc) below.

sd_match(N1, m_OneUse(m_AnyOf(m_SMax(m_Value(X), NegPat),
m_UMax(m_Value(X), NegPat),
m_SMin(m_Value(X), NegPat),
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 this can preserve flags

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I simply capture and reuse the SDValue of the first sub. It's done now.

m_UMin(m_Value(X), NegPat))))) {
unsigned NewOpc = ISD::getInverseMinMaxOpcode(N1->getOpcode());
if (hasOperation(NewOpc, VT))
return DAG.getNode(NewOpc, DL, VT, X, S0);
}

// Fold neg(splat(neg(x)) -> splat(x)
if (VT.isVector()) {
SDValue N1S = DAG.getSplatValue(N1, true);
Expand Down
15 changes: 15 additions & 0 deletions llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,21 @@ bool ISD::matchBinaryPredicate(
return true;
}

ISD::NodeType ISD::getInverseMinMaxOpcode(unsigned MinMaxOpc) {
switch (MinMaxOpc) {
default:
llvm_unreachable("unrecognized opcode");
case ISD::UMIN:
return ISD::UMAX;
case ISD::UMAX:
return ISD::UMIN;
case ISD::SMIN:
return ISD::SMAX;
case ISD::SMAX:
return ISD::SMIN;
}
}

ISD::NodeType ISD::getVecReduceBaseOpcode(unsigned VecReduceOpcode) {
switch (VecReduceOpcode) {
default:
Expand Down
Loading
Loading