[PATCH] D52286: [Intrinsic] Signed Saturation Intirnsic

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:1121
+    auto *SatBitsNode = cast<VTSDNode>(SatBits);
+    unsigned NumSatBits = SatBitsNode->getVT().getScalarSizeInBits();
+    Action = TLI.getSaturationOperationAction(
----------------
craig.topper wrote:
> Should we jsut pass the SatBitsNode->getVT() into getSaturationOperationAction and isSupportedSaturationOperation
As far as I know, saturation does not really need to depend on anything other than the saturation width, which is the main reason for why we only need a constant integer or value type to hold this width.

In this sense, I feel like passing anything else other than the width seems unnecessary, but if a user down the line needs more than the saturation width for checking sat legality, they could change these methods to accept the EVT instead.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:8937
+
+SDValue SelectionDAG::getExpandedSignedSaturation(SDNode *Node) {
+  assert(Node->getOpcode() == ISD::SSAT);
----------------
craig.topper wrote:
> I think maybe this should be in TargetLowering if I understand anything about our division of labor today. It feels similar to expandMUL_LOHI, expandMUL, expandFP_TO_SINT, scalarizeVectorLoad, scalarizeVectorStore.
This function is essentially for preventing duplicating code. There are different instances where we want to expand the node in the event we find the type or operation is illegal, so for convenience we call this when expanding.


Repository:
  rL LLVM

https://reviews.llvm.org/D52286