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

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added inline comments.


================
Comment at: lib/CodeGen/TargetLoweringBase.cpp:1883
+  //   SatMin -> (LHS < 0) && (RHS < 0) && (Sum > 0)
+  //
+  SDValue Zero = DAG.getConstant(0, dl, LHS.getValueType());
----------------
ebevhan wrote:
> 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.
I don't know what happened or what got fixed, but I'm able to use SADDO now for some reason :p. And the generated code is shorter now in the tests.


Repository:
  rL LLVM

https://reviews.llvm.org/D53053