[PATCH] D52286: [Intrinsic] Signed Saturation Intirnsic

[Bevin Hansson via Phabricator via llvm-commits]

ebevhan added a comment.

I think this looks pretty good to me now. It's probably a good time to add someone who is familiar with DAG as reviewer at this point.



================
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
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D52286