[PATCH] D52286: [Intrinsic] Signed Saturation Intirnsic

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

In https://reviews.llvm.org/D52286#1253498, @ebevhan wrote:

> In https://reviews.llvm.org/D52286#1253162, @craig.topper wrote:
>
> > I'm not sure why you would need an intrinsic at all. Why can't you just emit the IR equivalent from the beginning. This looks the equivalent of doing something like "x = std::min(std::max(x, -2048), 2047)" in C++ which would be implemented in IR with 2 compares and 2 selects. This is not an uncommon pattern to find in real code and we spend a lot of effort to make sure its handled well. Adding an intrinsic means you'll miss out on the constant folding, knownbits, etc that we've already put in for min/max IR patterns.
>
>
> The issue with using pure IR for these operations is that if the optimizer so much as sneezes on the representation of the operation, there's a high risk that you won't properly select a native operation. This is extremely important on targets for which these operations -- fixed-point in particular, but also saturation -- are legal, like our downstream one.
>
> It is pretty unfortunate that it means we can't take advantage of a lot of the work put into min-max patterns, which is one of the reasons why I thought an early lowering-to-IR pass would be interesting to consider on targets for which these operations are not legal, rather than doing the expansion as late as isel.


I don't know anything about your downstream target. For this particular intrinsic, if the middle end optimizers mess it up how bad would it be. How bad is 2 compares and 2 selects?



================
Comment at: include/llvm/CodeGen/TargetLowering.h:824
+    case ISD::SSAT:
+      Supported = isSupportedSaturationOperation(Op, VT, SatBitWidth);
+    }
----------------
This case needs a "break;" so you don't surprise the next person who adds a case.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:1121
+    auto *SatBitsNode = cast<VTSDNode>(SatBits);
+    unsigned NumSatBits = SatBitsNode->getVT().getScalarSizeInBits();
+    Action = TLI.getSaturationOperationAction(
----------------
Should we jsut pass the SatBitsNode->getVT() into getSaturationOperationAction and isSupportedSaturationOperation


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:8937
+
+SDValue SelectionDAG::getExpandedSignedSaturation(SDNode *Node) {
+  assert(Node->getOpcode() == ISD::SSAT);
----------------
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.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:8938
+SDValue SelectionDAG::getExpandedSignedSaturation(SDNode *Node) {
+  assert(Node->getOpcode() == ISD::SSAT);
+  assert(Node->getNumOperands() == 2);
----------------
Asserts need messages.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:8946
+  SDValue Op1 = Node->getOperand(0);
+  unsigned NumSatBits = SatBitsNode->getVT().getScalarSizeInBits();
+  unsigned SrcBits = Op1.getValueSizeInBits();
----------------
This can just be getSizeInBits can't it?


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5766
+    assert(SatBitsNode &&
+           "Second argument of ssaturate intrinsic must be a constant integer");
+
----------------
I think we should also verify that the second argument is contant in lib/IR/Verifier.cpp as well.


Repository:
  rL LLVM

https://reviews.llvm.org/D52286