[PATCH] D54534: [InstCombine] Add support for saturating add/sub

[Nikita Popov via Phabricator via llvm-commits]

nikic marked 4 inline comments as done.
nikic added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:2046-2054
     OverflowCheckFlavor OCF =
         IntrinsicIDToOverflowCheckFlavor(II->getIntrinsicID());
     assert(OCF != OCF_INVALID && "unexpected!");
 
     Value *OperationResult = nullptr;
     Constant *OverflowResult = nullptr;
     if (OptimizeOverflowCheck(OCF, II->getArgOperand(0), II->getArgOperand(1),
----------------
spatel wrote:
> Can we reuse this code for the new intrinsics?
This is generally possible. The reason why I did not do this is that for the always overflow case, we'd end up inserting an unused instruction (https://github.com/llvm-mirror/llvm/blob/7c7c0a2cd982d34de3e49dc0ef7a459e1262f306/lib/Transforms/InstCombine/InstCombineCompares.cpp#L3805).

If creating unnecessary instructions as an intermediate state is fine, I can switch this to use OptimizeOverflowCheck.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:2061-2068
+    if (isa<Constant>(II->getArgOperand(0)) &&
+        !isa<Constant>(II->getArgOperand(1))) {
+      // Canonicalize constants into the RHS.
+      Value *LHS = II->getArgOperand(0);
+      II->setArgOperand(0, II->getArgOperand(1));
+      II->setArgOperand(1, LHS);
+      return II;
----------------
spatel wrote:
> This was already duplicated 3x, so I added a helper:
> rL347604
> 
> I might have missed it, but I don't think the tests exercise this possibility? Also, the baseline tests are not checked into trunk yet?
You're right, a test case for this is missing. I didn't commit the baseline tests yet, in case there are comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:2085-2087
+      if (OR == OverflowResult::AlwaysOverflows)
+        return replaceInstUsesWith(*II,
+                                   ConstantInt::getAllOnesValue(II->getType()));
----------------
spatel wrote:
> Should we add this (and the later usub_sat fold to zero) to InstSimplify? That would make all of these cases more symmetric: 
> if (no_ov) { create regular math op }
> 
> At that point, it might not be worth using this switch-within-a-switch. Just handle each intrinsic in its own case statement.
Adding this to InstSimplify would mean that we have to run the overflow calculation (which requires known bits) twice, once in InstSimplify for the always overflow case and one in InstCombine for the never overflow case. Not sure if that's worthwhile?


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:2109
+    // sat(sat(X - Val2) - Val) -> sat(X - (Val+Val2))
+    // if Val and Val2 have the same sign
+    if (auto *Other = dyn_cast<IntrinsicInst>(Arg0)) {
----------------
One case this code currently does not handle is something like ssub.sat(ssub.add(%foo, -10, 10), i.e. mixes of signed sub and add where the sign ends up being the same if we account for the add/sub.

I think this case could be elegantly handled by canonicalizing ssub.sat(%foo, C) to ssub.add(%foo, -C) (assuming C != MIN), similar to the canonicalization that also happens for normal subs. What do you think about this?


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

https://reviews.llvm.org/D54534