[PATCH] D55735: [InstSimplify] Simplify saturating add/sub + icmp

Sun Dec 16 10:53:53 PST 2018

nikic marked 2 inline comments as done.
nikic added inline comments.

================
Comment at: lib/Analysis/InstructionSimplify.cpp:2668-2669
+  case Intrinsic::ssub_sat:
+    // ssub.sat(x, C) is canonicalized to sadd.sat(x, -C) by instcombine, so
+    // no need to check for it here.
+    if (match(II.getOperand(0), m_APInt(C))) {
----------------
spatel wrote:
> Assuming canonical IR isn't a valid assumption for InstSimplify because InstSimplify is used as an analysis independently of InstCombine. Even when InstSimplify is called from within InstCombine, it would theoretically be more efficient to handle non-canonical simplifications before doing other transforms in InstCombine.
> 
> Do we "internally canonicalize" a constant operand for uadd/sadd to operand 1 here in InstSimplify? If not, we might want to do that too. Also if the existing code for binops doesn't do that, it's probably worth a TODO comment.
> 
> In all cases, it's worth varying at least some of the regression tests to prove that we have those non-canonical patterns covered.
I've implemented handling for the non-canonical forms. Regarding doing canonicalization in InstSimplify, I think it only does this if the simplify operation itself takes the instruction in unpacked form (such as SimplifyBinOp which has Opcode+LHS+RHS), not when working on explicit instructions. I'm assuming that swapping operands of actual instruction is not allowed inside InstSimplify.

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

https://reviews.llvm.org/D55735