[PATCH] D60061: [InstCombine] ssubo X, C -> saddo X, -C

[Dan Robertson via Phabricator via llvm-commits]

dlrobertson marked 5 inline comments as done.
dlrobertson added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2160-2164
+    if (match(Arg1, m_APInt(C)) &&
+        !(C->isMinSignedValue() || C->isMaxSignedValue())) {
+      // Create a copy of the original constant and negate it.
+      APInt NewC = APInt(*C);
+      NewC.negate();
----------------
lebedev.ri wrote:
> Hmm, why not just:
> ```
> APInt NewC;
> if (match(Arg1, m_APInt(C)) &&
>     !subWithOverflow(NewC, 0, *C, /*IsSigned=*/II->getIntrinsicID() == Intrinsic::ssub_with_overflow)) {
> ```
> ?
Thanks for the suggestion, but as mentioned above I think `isNotMinSignedValue` is what I need here. If I am misunderstanding something, please let me know.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2161
+    if (match(Arg1, m_APInt(C)) &&
+        !(C->isMinSignedValue() || C->isMaxSignedValue())) {
+      // Create a copy of the original constant and negate it.
----------------
nikic wrote:
> RKSimon wrote:
> > Is C->isMaxSignedValue() necessary?
> Checking for max signed value is not necessary, as `-SignedMax` doesn't overflow, only `-SignedMin` does.
> 
> For parity with `ssub.sat` canonicalization I'd suggest to use the same implementation here, which will also handle non-splat vector constants:
> 
> ```
> Constant *C;
> if (match(Arg1, m_Constant(C)) && C->isNotMinSignedValue()) {
>     Value *NegVal = ConstantExpr::getNeg(C);
>     // Create ssubo with NegVal here.
> }
> ```
Ah! Right. I updated the diff to look more like `ssub.sat`.


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

https://reviews.llvm.org/D60061