This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Recognize SSAT and USAT from SMIN/SMAX
ClosedPublic

Authored by dmgreen on Feb 15 2022, 1:57 AM.

Details

Summary

We have some recognition of SSAT and USAT from SELECT_CC at the moment. This extends the matching to SMIN/SMAX which can help catch more cases, either from min/max being the canonical form in instcombine or from some expanded nodes like fp_to_si_sat.

Diff Detail

Event Timeline

dmgreen created this revision.Feb 15 2022, 1:57 AM
dmgreen requested review of this revision.Feb 15 2022, 1:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2022, 1:57 AM

Any reason why this couldn't be performed generically in DAGCombiner::visitIMINMAX?

Any reason why this couldn't be performed generically in DAGCombiner::visitIMINMAX?

I think ssat/usat are Arm specific things. There is not even an AArch64 equivalent.

Sorry - turns out I was half remembering D52286, which never even landed!

SjoerdMeijer added inline comments.Feb 16 2022, 1:44 AM
llvm/lib/Target/ARM/ARMISelLowering.cpp
1571

I was just curious if we need to set a target combine for SMAX given that SMIN is the root of the node that we are matching; don't know how that works exactly...

17566

Typo: we they

17615

Should we try PerformVQDMULHCombine if this returns SDValue()?

Thanks for taking a look.

llvm/lib/Target/ARM/ARMISelLowering.cpp
1571

The code tries to match min(max(..)) or max(min(..)), providing the bounds are correct. Both seem to come up in practice, from the tests we have. There is some code in PerformMinMaxToSatCombine that swaps the min and max nodes if it needs to.

17566

Oh yeah, will fix.

17615

The rest of this function looks at vectors under MVE. VQDMULH is only an MVE vector operation, so won't do anything for i32.

SjoerdMeijer accepted this revision.Feb 17 2022, 1:10 AM

Ok, thanks, looks like a nice patch to me.

This revision is now accepted and ready to land.Feb 17 2022, 1:10 AM
dmgreen updated this revision to Diff 409529.Feb 17 2022, 1:29 AM

Thanks! Here's an update for that comment, to make sure I don't forget.

This revision was landed with ongoing or failed builds.Feb 23 2022, 12:56 AM
This revision was automatically updated to reflect the committed changes.