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

[Sanjay Patel via Phabricator via llvm-commits]

spatel 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),
----------------
nikic wrote:
> 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.
No, let's not do that then. Creating a dead instruction could mean that instcombine has to iterate the entire function again just to remove it.


================
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;
----------------
nikic wrote:
> 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.
Feel free to add/commit tests as you'd like with NFC patches. We can always adjust them if they don't provide coverage for the code that gets added later.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:2085-2087
+      if (OR == OverflowResult::AlwaysOverflows)
+        return replaceInstUsesWith(*II,
+                                   ConstantInt::getAllOnesValue(II->getType()));
----------------
nikic wrote:
> 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?
Good point. There's no clear answer for where we draw the line. Computing known bits is known to be expensive, but adding to instsimplify potentially gets the code optimized faster (and improves other passes). 

It comes down to how often we expect to see these intrinsics in real code vs. how often we would expect to succeed at these transforms. My guess is those answers are "rare" and "rarer", but I have no data.

Let's just keep this as-is then.



================
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)) {
----------------
nikic wrote:
> 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?
Yes, if we can convert ssub to sadd, let's do that. The more these are handled like the regular math ops, the better.


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

https://reviews.llvm.org/D54534