[PATCH] D58881: [Transform] Improve fold of sadd.with.overflow

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1825
+  if (OptimizeOverflowCheck(OCF, II->getArgOperand(0), II->getArgOperand(1),
+                            *II, OperationResult, OverflowResult)) {
+    return CreateOverflowTuple(II, OperationResult, OverflowResult);
----------------
nit: Unnecessary braces.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2046
+    //
+    // FIXME: Add the last case.
+    Value *X = nullptr;
----------------
dlrobertson wrote:
> I was not entirely sure how to solve the last case. `sadd.with.overflow` returns an aggregate, while `add nsw` does not. I was not sure how to construct the aggregate, so I did not address the last case in my first attempt.
You can construct the aggregate with the `CreateOverflowTuple` helper, with overflow set to a false constant.

It's probably better to leave it out in this patch though, as this is quite a different optimization. Generally I'd drop all the sign/range checks here, as the fold is valid in either case -- there's just an even better one possible that's not implemented yet.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2049
+    ConstantInt *RHS = nullptr;
+    ConstantInt *LHS = nullptr;
+    if (match(II->getArgOperand(0), m_NSWAdd(m_Value(X), m_ConstantInt(LHS))) &&
----------------
nit: The variables used in the matchers don't need to be initialized, and it's usually not done.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2051
+    if (match(II->getArgOperand(0), m_NSWAdd(m_Value(X), m_ConstantInt(LHS))) &&
+        match(II->getArgOperand(1), m_ConstantInt(RHS))) {
+      APInt LHSValue = LHS->getValue();
----------------
It is better to use an `m_APInt` matcher for these, because it will also work with (splat) vector constants (and you're working on APInts below anyway).


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2056
+          RHSValue.abs().sgt(LHSValue.abs())) {
+        Constant *NewC = ConstantExpr::getAdd(LHS, RHS);
+        return replaceInstUsesWith(
----------------
Consider something like `%y = add nsw i8 %x, 127; sadd.with.overflow(%y, 127)`. The constants have same sign and you will transform this to `sadd.with.overflow(%x, 127+127)`, where `127+127 = -2`.

This transform can't be applied if the result of the addition overflows. You can use `sadd_ov` on APInt to check for that.


================
Comment at: llvm/test/Transforms/InstCombine/call-add-with-overflow.ll:1
+; RUN: opt < %s -instcombine -S | FileCheck %s
+
----------------
To make future diffs easier, it's preferred to automatically generate checks using the `utils/update_test_checks.py` script.


================
Comment at: llvm/test/Transforms/InstCombine/call-add-with-overflow.ll:22
+  ret { i32, i1 } %3
+}
----------------
As we recently added vector support for these intrinsics, we should also have some vector tests. Good cases to cover are:

 * Splat constant `<2 x i32> <i32 42, i32 42>` (will work out of the box once you switch to m_APInt)
 * Splat constant with undef `<2 x i32> <i32 42, i32 undef>` (will also work out of the box)
 * Non-splat constant `<2 x i32> <i32 42, i32 24>` (will not work, and that's okay)

Additionally, the overflowing case mentioned above should be checked, as a negative example. The inverted case here with `-7` and `13` rather than the other way around, would also be good.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58881