[PATCH] D119819: [ARM] Recognize SSAT and USAT from SMIN/SMAX

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Thanks for taking a look.



================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:1567
+    setTargetDAGCombine(ISD::SMIN);
+    setTargetDAGCombine(ISD::SMAX);
+  }
----------------
SjoerdMeijer wrote:
> 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...
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.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:17561
 
+// Lower smin(smax(x, C1), C2) to ssat or usat, if we they have saturating
+// constant bounds.
----------------
SjoerdMeijer wrote:
> Typo: we they
Oh yeah, will fix.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:17610
+  if (VT == MVT::i32)
+    return PerformMinMaxToSatCombine(SDValue(N, 0), DAG, ST);
+
----------------
SjoerdMeijer wrote:
> Should we try `PerformVQDMULHCombine` if this returns `SDValue()`?
The rest of this function looks at vectors under MVE. VQDMULH is only an MVE vector operation, so won't do anything for i32.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119819/new/

https://reviews.llvm.org/D119819