This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Fold sub(add(x,y),min/max(x,y)) -> max/min(x,y) (PR38280)
ClosedPublic

Authored by RKSimon on Apr 8 2022, 9:10 AM.

Details

Summary

As discussed on Issue #37628, we can flip a min/max node if we're subtracting from the sum of the node's operands

My main query is whether we need any oneuse limits or not - the only case where we don't see any reduction in instructions is if both the add + min/max have other uses, where we then replace a sub with a max/min intrinsic, which is probably going too far - any thoughts?

Diff Detail

Event Timeline

RKSimon created this revision.Apr 8 2022, 9:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2022, 9:10 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
RKSimon requested review of this revision.Apr 8 2022, 9:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2022, 9:10 AM
nikic added a comment.Apr 8 2022, 12:14 PM

LoopVectorize failures looks related.

My main query is whether we need any oneuse limits or not - the only case where we don't see any reduction in instructions is if both the add + min/max have other uses, where we then replace a sub with a max/min intrinsic, which is probably going too far - any thoughts?

I'd probably limit the minmax intrinsic to one use, to avoid creating two of those.

llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
2014

Use MinMaxIntrinsic + getInverseMinMaxIntrinsic() rather than enumerating cases?

RKSimon updated this revision to Diff 421705.Apr 9 2022, 2:44 AM

Require one of the operands to be oneuse and convert to MinMaxIntrinsic/getInverseMinMaxIntrinsic()

nikic added inline comments.Apr 10 2022, 11:46 AM
llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
2017

I believe m_c_MaxOrMin also accepts min/max in select form, which would then assert with the intrinsic cast. Unless we want to adjust the matcher (which we might well want to do now that we canonicalize to intrinsic form, but probably independently of this change) I'd suggest a dyn_cast to MinMaxIntrinsic.

RKSimon updated this revision to Diff 421803.Apr 10 2022, 12:54 PM

Use MinMaxIntrinsic directly

nikic accepted this revision.Apr 11 2022, 3:18 AM

LGTM

This revision is now accepted and ready to land.Apr 11 2022, 3:18 AM
This revision was landed with ongoing or failed builds.Apr 11 2022, 3:33 AM
This revision was automatically updated to reflect the committed changes.