[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: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;
----------------
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?


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:2085-2087
+      if (OR == OverflowResult::AlwaysOverflows)
+        return replaceInstUsesWith(*II,
+                                   ConstantInt::getAllOnesValue(II->getType()));
----------------
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.


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

https://reviews.llvm.org/D54534