[PATCH] D152068: [InstCombine] add overflow checking on AddSub `C-(X+C2) --> (C-C2)-X`

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:2002
+      BinaryOperator *Res = BinaryOperator::CreateSub(
+          ConstantExpr::getSub(C, C2, /*HasNUW=*/false, HasNSW), X);
+      auto *OBO1 = cast<OverflowingBinaryOperator>(Op1);
----------------
given that C-C2 is a constexpr do the flag matter at all? If not can you drop them from the constexpr, they just seem noisey.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:2004
+      auto *OBO1 = cast<OverflowingBinaryOperator>(Op1);
+      if (I.hasNoSignedWrap() && OBO1->hasNoSignedWrap() && HasNSW)
+        Res->setHasNoSignedWrap(true);
----------------
Nit:
`Res->setHasNoSignedWrap(I.hasNoSignedWrap() && OBO1->hasNoSignedWrap() && HasNSW)`


================
Comment at: llvm/test/Transforms/InstCombine/addsub-constant-folding.ll:640
 }
+
+define i8 @add_sub_nsw(i8 %x, i8 %y) {
----------------
Can you precommit the tests? Also can you add a failure test s.t one of the 3 conditions aren't met for each condition.


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

https://reviews.llvm.org/D152068