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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

The more general transforms (changing the icmp predicate and/or constant operand of the icmp) should be handled under InstCombiner::visitICmpInst().
This is a cheap analysis, so it's probably best to have this part in InstSimplify regardless of what we add to InstCombine.

The logic looks good, but see inline for an improvement related to canonical form.



================
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))) {
----------------
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55735