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

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.

LGTM - see inline for a couple of minor points.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2038-2040
+    // `add nsw` + `sadd.with.overflow` with constants should fold.
+    Value *X;
+    const APInt *RHS, *LHS;
----------------
Nit: prefer to give constant values a 'C' name and write a code comment using those names to describe the transform.
So something like:

```
// If we have 2 constant operands whose sum does not overflow:
// saddo (X +nsw C0), C1 --> saddo X, C0+C1
const APInt *C0, *C1;
if (match(...))
```


================
Comment at: llvm/test/Transforms/InstCombine/sadd-with-overflow.ll:36-37
 ;
   %2 = add nsw i8 %0, 127
   %3 = tail call { i8, i1 } @llvm.sadd.with.overflow.i8(i8 %2, i8 127)
   ret { i8, i1 } %3
----------------
Nit: Ideally, the positive test would use constants at the limit, and the negative test would use constants just over that limit. So the 1st test would be something like:

```
  %2 = add nsw i8 %0, 100
  %3 = tail call { i8, i1 } @llvm.sadd.with.overflow.i8(i8 %2, i8 27)
  ret { i8, i1 } %3
```
and this one:

```
  %2 = add nsw i8 %0, 100
  %3 = tail call { i8, i1 } @llvm.sadd.with.overflow.i8(i8 %2, i8 28)
  ret { i8, i1 } %3

```

That gives us just a bit more coverage in case the underlying code breaks somehow, and makes it clearer how the transform is implemented.


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

https://reviews.llvm.org/D58881