[PATCH] D52286: [Intrinsic] Signed Saturation Intirnsic

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added inline comments.


================
Comment at: include/llvm/CodeGen/TargetLowering.h:808
+
+  /// Some scaled operations may be natively supported by the target but only
+  /// for specific scales. This method allows for checking if the scale is
----------------
ebevhan wrote:
> leonardchan wrote:
> > ebevhan wrote:
> > > This says 'scaled' but currently only looks at saturating operations (and is named as such).
> > > 
> > > If you want to make it generic, maybe it should be 'getSatOrScaledOperationAction' and corresponding for the hook above? Not sure what to call the parameter, though.
> > I wanted to separate them both into 2 functions that could be called after each other on intrinsics that use both saturation and scale. I initially wrote this as the scaled method when I meant saturation, but forgot to redo some of the comments.
> Hm, okay. I'm assuming that the opcode (e.g. fixsmul_sat) would be passed down into the hooks in that case? Since the hook already knows what opcode is being examined, why not just have one hook? The legality of 'fixsmul_sat' for a certain type is not predicated on whether 'fixsmul' and 'ssat' are legal. And presumably, if the fixsmul_sat is legal, both hooks would return true anyway.
Yes, fixsmul_sat would be passed to both hooks. Are you saying that an intrinsic that uses both scale and saturation like fixsmul_sat should have its own hook that checks both at the same time?

Something like

```
getScaledSaturationOperationAction(unsigned Op, EVT VT, unsigned Scale, unsigned SatBitWidth)
```

I imagine that for our purposes, 2 separate functions would work and be independent from each other because it seems cleaner, but if a user down the line requires this combined function, it could be reworked then in the future.


Repository:
  rL LLVM

https://reviews.llvm.org/D52286