[PATCH] D54237: Constant folding and instcombine for saturating adds

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

I'm no expert on how things are divided between InstructionSimplify and InstCombine, but shouldn't the simple folds be in InstructionSimplify?
For the record, we were planning to upstream something like this in simplifyBinaryIntrinsic in InstructionSimplify.cpp:

  case Intrinsic::sadd_sat:
    // X + 0 -> X
    if (match(Op1, m_Zero()))
      return Op0;
  
    // 0 + X -> X
    if (match(Op0, m_Zero()))
      return Op1;
  
    // X + undef -> undef
    // undef + X -> undef
    if (match(Op1, m_Undef()) || match(Op0, m_Undef()))
      return UndefValue::get(ReturnType);
  
    break;
  case Intrinsic::ssub_sat:
    // X - 0 -> X
    if (match(Op1, m_Zero()))
      return Op0;
  
    // X - undef -> undef
    // undef - X -> undef
    if (match(Op1, m_Undef()) || match(Op0, m_Undef()))
      return UndefValue::get(ReturnType);
  
    break;

The above also fold expressions involving undef.

nit: In our dowstream repo we currently have implemented folds for sadd_sat and ssub_sat. So even if doing add/sub in different patches, it would be really nice to have a patch for ssub_sat and usub_sat ready and land both patches at the same time. That way we can replace things in one merge, without the need to support the old folds of ssub_sat and the new folds for sadd_sat during a transition period.



================
Comment at: lib/Support/APInt.cpp:1953
+  APInt Res = sadd_ov(RHS, Overflow);
+  if (Overflow) {
+    if (isNegative()) {
----------------
Coding style: skip all braces here.


================
Comment at: lib/Support/APInt.cpp:1966
+  APInt Res = uadd_ov(RHS, Overflow);
+  if (Overflow) {
+    Res = APInt::getMaxValue(Res.getBitWidth());
----------------
Coding style: skip all braces here.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:2027
+    if (isa<Constant>(Arg0) && !isa<Constant>(Arg1)) {
+      // Canonicalize constants into the RHS
+      II->setArgOperand(0, Arg1);
----------------
full stop/period missing


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:2045
+
+      // Make use of known overflow information
+      if (IsUnsigned) {
----------------
full stop/period missing


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:2077
+              // Both adds together may add more than SignedMaxValue
+              // without saturing the final result
+              break;
----------------
full stop/period missing


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:2081
+          } else {
+            // Cannot fold saturated addition with different signs
+            break;
----------------
full stop/period missing


Repository:
  rL LLVM

https://reviews.llvm.org/D54237