Skip to content

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

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
merged 6 commits into from
Jan 3, 2025
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
abf6f85
Pre-commit test
mshockwave Dec 20, 2024
25c43d8
[DAGCombiner] Turn `(neg (max x, (neg x)))` into `(min x, (neg x))`
mshockwave Dec 20, 2024
1abcdc3
Address review comments
mshockwave Dec 20, 2024
55d7531
Add `sub 0, (min X, (sub 0, X))) --> (max X, (sub 0, X))`
mshockwave Dec 20, 2024
3e85bc4
Extract min<->max conversion into its own helper function
mshockwave Dec 23, 2024
b8b253b
Address review comments
mshockwave Dec 26, 2024
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
34 changes: 34 additions & 0 deletions llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3949,6 +3949,40 @@ 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

@jayfoad jayfoad Dec 20, 2024

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

@mshockwave mshockwave Dec 20, 2024

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

@arsenm arsenm Dec 23, 2024

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 •
edited
Loading

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

@arsenm arsenm Dec 26, 2024

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

@mshockwave mshockwave Dec 26, 2024

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

@arsenm arsenm Dec 20, 2024

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

@mshockwave mshockwave Dec 20, 2024

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 = 0;
switch (N1->getOpcode()) {
case ISD::SMAX:
NewOpc = ISD::SMIN;
break;
case ISD::UMAX:
NewOpc = ISD::UMIN;
break;
case ISD::SMIN:
NewOpc = ISD::SMAX;
break;
case ISD::UMIN:
NewOpc = ISD::UMAX;
break;
default:
llvm_unreachable("unrecognized opcode");
}
Copy link
Contributor

@arsenm arsenm Dec 23, 2024

Choose a reason for hiding this comment

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

Should be helper function instead of switch and break. Do we not have a min<->max helper already? I swear I've written one several times

Copy link
Member Author

@mshockwave mshockwave Dec 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do we not have a min<->max helper already? I swear I've written one several times

That was what I tried to search in the first place too, but I don't think there is any. So I'd extracted them into ISD::getInverseMinMaxOpcode.

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
Loading
Loading