[PATCH] D68651: [InstCombine] Signed saturation patterns

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Even though we don't care about `trunc`, in all the tests there is a `trunc` in the end.
Can you either drop it from tests, or at least add a test without trunc?

Looks good to me in principle.

I'm pretty sure there are two more patterns that can be handled - you are using extension
because in original code [signed] overflow is undefined.
But that is only the case in IR if `nsw` is specified.
You can write it without UB, e.g.:

  ----------------------------------------
    %agg = uadd_overflow {i4, i1} %a, %b
    %add = extractvalue {i4, i1} %agg, 0
    %ov = extractvalue {i4, i1} %agg, 1
    %res = select %ov, i4 -1, %add
    ret i4 %res
  =>
    %res = uadd_sat i4 %a, %b
    ret i4 %res
    %agg = uadd_overflow {i4, i1} %a, %b
    %ov = extractvalue {i4, i1} %agg, 1
    %add = extractvalue {i4, i1} %agg, 0
  
  Done: 1
  Optimization is correct!

yet we don't handle it:
https://godbolt.org/z/6VAnoL

That can also be written without `@llvm.uadd.with.overflow()`, and the same for signed.



================
Comment at: llvm/include/llvm/IR/Type.h:388
+  /// whilst keeping the old number of lanes.
+  inline Type *changeBitWidth(unsigned NewBitwidth) const;
+
----------------
Can you use it somewhere in existing code and land this change separately?


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

https://reviews.llvm.org/D68651