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

[Bevin Hansson via Phabricator via llvm-commits]

ebevhan added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:550
+
+  SDValue Op1Promoted = GetPromotedInteger(Op1);
+  Op1Promoted =
----------------
If I understand correctly, Op1Promoted and Op2Promoted are already any-extended to the wider type: "The low bits of the promoted value corresponding to the original type are exactly equal to Op." and "The extra bits contain rubbish". So there shouldn't be a need for an explicit ANY_EXTEND after.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:3008
   SDValue LHSLo, LHSHi, RHSLo, RHSHi;
+  NewLHS.dump();
   GetExpandedInteger(NewLHS, LHSLo, LHSHi);
----------------
Seems like this snuck in.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeTypes.h:418
   void ExpandIntRes_XMULO             (SDNode *N, SDValue &Lo, SDValue &Hi);
+  void ExpandIntRes_SATSADD           (SDNode *N, SDValue &Lo, SDValue &Hi);
 
----------------
Should be named SADDSAT.


================
Comment at: lib/CodeGen/TargetLoweringBase.cpp:1883
+  //   SatMin -> (LHS < 0) && (RHS < 0) && (Sum > 0)
+  //
+  SDValue Zero = DAG.getConstant(0, dl, LHS.getValueType());
----------------
leonardchan wrote:
> ebevhan wrote:
> > 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.
> I'm running into legality issues when type expansion is involved. I was able to replace this block to use SADDO but ran into an error saying the SADDO node was not expanded when legalizing types. I'm not sure if I should call another function to "force check" expansion on my newly created SADDO node, but I couldn't find any other examples that show usage of SADDO during expansion.
> 
> Alternatively, I could move the code for expanding SADDO into its own method that I could call from here. 
Odd... It looks to me like expanding SADDO during type legalization should work fine. I was really just curious to see if it produced better code for the legal type case than expanding outright. Maybe someone else knows more about why it doesn't want to work.

If it's just a problem during type expansion you could always check if the type is legal, and if so, use SADDO. But supporting both SADDO and the full expansion seems like overkill.


Repository:
  rL LLVM

https://reviews.llvm.org/D53053