[PATCH] D52286: [Intrinsic] Signed Saturation Intirnsic

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

In https://reviews.llvm.org/D52286#1257532, @leonardchan wrote:

> *Ping*
>
> @ebevhan @craig.topper Any more comments on this patch?
>
> I'm also unfamiliar with @ebevhan's target, but I imagine there are different CPUs that have their own operations for performing saturation that could use this as an alternative to selects and compares.


Agreed, but that doesn't mean you can't pattern match the select+cmp sequence or a pair of ISD::MIN/MAX to a native operation with a target specific DAG combine. See for example detectSSatPattern and detectUSatPattern in X86ISelLowering.cpp. But maybe middle end IR can mess it up ever so slightly and make it not emit quite the right pattern and then you're left with something close to the pattern, but not quite. Maybe the MIN got removed or the MAX got removed because we could prove the number was positive or something.  So then you end up with an incomplete pattern. I just want to understand what the worst case incomplete pattern would look like for @ebevhans's target.



================
Comment at: lib/CodeGen/TargetLoweringBase.cpp:1881
+  EVT Ty = Op1.getValueType();
+  auto MinVal = APInt::getSignedMinValue(NumSatBits).sext(SrcBits);
+  auto MaxVal = APInt::getSignedMaxValue(NumSatBits).sext(SrcBits);
----------------
Should have caught this earlier. LLVM style prefers to use auto only when its super obvious like on the result of dyn_cast or cast. So these should be APInt and SDValue. Though it could be argued that APInt is pretty obvious since its mentioned in the class on the right hand side, but APInt is only one character longer than auto since doesn't really save much.


Repository:
  rL LLVM

https://reviews.llvm.org/D52286