[PATCH] D87379: [ARM] Selects SSAT/USAT from LLVM IR of min/max patterns

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

It's probably worth adding something to the commit message about how llvm will canonicalize to different patterns than the old code would select from, and that this is updating it to the new expected form.



================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:5002
 // Similar to isLowerSaturate(), but checks for upper-saturating conditions.
 static bool isUpperSaturate(const SDValue LHS, const SDValue RHS,
                             const SDValue TrueVal, const SDValue FalseVal,
----------------
It looks like this isn't used any more. If not, we can remove it. Unfortunately it appears that isLowerSaturate is still used.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:5013
 //
 // SSAT can replace a set of two conditional selectors that bound a number to an
 // interval of type [k, ~k] when k + 1 is a power of 2. Here are some examples:
----------------
Can you add something to this comment about llvm canonicalizing to min(max(..)) or max(min(..))


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:5029
 static bool isSaturatingConditional(const SDValue &Op, SDValue &V,
                                     uint64_t &K, bool &usat) {
+  SDValue V1 = Op.getOperand(0);
----------------
Can you also change usat to Usat or USat or something like it.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:5085-5086
+      V = V2Tmp;
+      K = (uint64_t)
+          PosVal; // At this point, PosVal is guaranteed to be positive
+
----------------
Can you move the comment to its own line, to make the formatting nicer.

You can also possibly reverse the if conditions above to return early and reduce the need for indenting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87379