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

[Bevin Hansson via Phabricator via llvm-commits]

ebevhan added inline comments.


================
Comment at: include/llvm/CodeGen/ISDOpcodes.h:265
+    /// resulting value is this minimum value.
+    SATSADD,
+
----------------
This is a nitpick but I think it would be better if it were named SADDSAT to match the naming of the other nodes (SADDO, SMULO for example).


================
Comment at: include/llvm/CodeGen/TargetLowering.h:824
+      llvm_unreachable("Unexpected saturation operation");
+    case ISD::SATSADD:
+      Supported = isSupportedSaturationOperation(Op, VT, SatBitWidth);
----------------
The thing with saturating addition (and subtraction) is that the saturation bitwidth is determined by the width of the type itself anyway, so I don't think there's a need for a hook for it.

You only need a hook if the legality needs to be predicated on something other than the result and operand type.


================
Comment at: include/llvm/IR/Intrinsics.td:708
+                            [IntrNoMem, IntrSpeculatable]>;
+
 //===------------------------- Memory Use Markers -------------------------===//
----------------
Same naming scheme point here, to match the overflow intrinsics it should probably be `int_sadd_sat` or similar.


================
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));
----------------
craig.topper wrote:
> 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.
I think a more correct result could be obtained for iN to iM by doing ANY_EXTEND to iM->LSHR by M-N->SATSADD->ASHR M-N. I'm not sure if this is the best way, though.


================
Comment at: lib/CodeGen/TargetLoweringBase.cpp:1883
+  //   SatMin -> (LHS < 0) && (RHS < 0) && (Sum > 0)
+  //
+  SDValue Zero = DAG.getConstant(0, dl, LHS.getValueType());
----------------
Would there be a benefit to using SADDO here if it's legal? Minimum saturation occurs if `Overflow && Sum > 0` and maximum saturation if `Overflow && Sum < 0`.

It really depends on whether or not the target can chain the operations effectively, but I suspect that most targets won't be able to keep the overflow flag alive without materializing it.


Repository:
  rL LLVM

https://reviews.llvm.org/D53053