[PATCH] D52286: [Intrinsic] Signed Saturation Intirnsic

[Bevin Hansson via Phabricator via llvm-commits]

ebevhan added a comment.

I think we need to decide where these intrinsics should be 'lowered' if they aren't legal, and define how to let the target specify legality.

Lowering during isel is probably the 'right' place, but I think it gets tricky when you have to deal with lowering illegal operations on legal types where the legalized operation needs to be done in an illegal type. It's easier to do it in IR instead, but that goes against the way legalization is typically done.



================
Comment at: include/llvm/CodeGen/ISDOpcodes.h:264
+    /// signed value is returned instead.
+    SSAT,
+
----------------
With the way the rest is written, it doesn't seem like this DAG opcode is ever used.

The way you would normally do this in DAG would be to:
* Lower the intrinsic to this node in SelectionDAGBuilder, regardless of type or operation legality
* Deal with illegally typed nodes in LegalizeTypes
* Deal with illegal opcodes in LegalizeDAG

I think that doing the legalization in DAG for some of the fixed-point operations could be hard. Opcode legalization occurs after type legalization, so the legalization for opcodes that have legal types but where the target doesn't actually support the operation must be able to legalize to valid types. I think this could get unnecessarily tricky. For example, an N-bit, M-scale fixsmul must actually be done as N+M-bits or greater, and if that type (or anything greater than it) isn't legal it gets hairy.

There's also the issue of how a target specifies legality of these operations. A target might support ISD::FIXSMUL on MVT::i16, but only with a scale of 15. However, there's currently no way of expressing legality of an operation based on anything than the type (in general). Something would probably have to be added to TargetLowering/TargetLoweringBase that lets targets specify the legality of these operations. (The same issue applies to ssaturate; it might be legal to do 'i32 ssaturate (X, 15)', but not 'i32 ssaturate (X, 19)')




================
Comment at: lib/CodeGen/IntrinsicLowering.cpp:416
+    Ops[1] = CI->getArgOperand(1);
+    ReplaceCallWith("ssaturate", CI, Ops, Ops + 2,
+                    CI->getArgOperand(0)->getType());
----------------
This will replace the intrinsic with a call to 'ssaturate'. Given that llvm.ssaturate is overloaded, I'm not sure how that will work. If anything, you would want to replace the intrinsic with regular IR here instead.

This is the first time I've seen this 'IntrinsicLowering' component though. It seems like it's only part of the IR interpreter. Not sure how important that is.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5768
+  case Intrinsic::ssaturate: {
+    Value *Src = I.getArgOperand(0);
+    auto *SatBits = dyn_cast<ConstantInt>(I.getArgOperand(1));
----------------
This should probably be building an SSAT node rather than actually converting the intrinsic to normal DAG operations.


================
Comment at: lib/Transforms/FixedPoint/SaturationPass.cpp:13
+namespace {
+struct SignedSaturationPass : public FunctionPass {
+  static char ID;
----------------
I think this would be more futureproof if it didn't strictly deal with ssaturate, but also lowered other fixed-point operations as well.

However, signed saturation is not strictly a fixed-point operation, so I'm not really sure how to go about it without it seeming odd.

It should probably also ask the target somehow if the operations should be converted at all. If this were considered a 'fixed-point lowering pass' then it could be inserted into the pass pipeline towards the end to lower operations that are not legal according to the target.


Repository:
  rL LLVM

https://reviews.llvm.org/D52286