[PATCH] D53053: [Intrinsic] Signed Saturation Addition Intirnsic

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:540
+SDValue DAGTypeLegalizer::PromoteIntRes_SATSADD(SDNode *N) {
+  SDValue LHS = SExtPromotedInteger(N->getOperand(0));
+  SDValue RHS = SExtPromotedInteger(N->getOperand(1));
----------------
This doesn't seem right. Let's say the input was an i8 SATSADD that should have returned [-128,127] with no wrap around. Let's say the type to promote to i16.  This becomes two sign_extend_inreg from i8 to i16 and an an i16 SATSADD, but that will only saturate when the math overflows 16 bits. So it's not longer saturating to [-128,127]. You get can [-256,254] now.


================
Comment at: lib/CodeGen/TargetLoweringBase.cpp:1874
+         "Expected both operands to be the same in SATSADD");
+  unsigned BitWidth = LHS.getValueSizeInBits();
+
----------------
What about vector types?


================
Comment at: lib/CodeGen/TargetLoweringBase.cpp:1886
+  EVT BoolVT =
+      getSetCCResultType(DAG.getDataLayout(), *DAG.getContext(), MVT::i1);
+
----------------
getSetCCResultType takes the type of the input to the setcc and returns the type that should be used for its output. So MVT::i1 here should LHS.getValueType()


Repository:
  rL LLVM

https://reviews.llvm.org/D53053