[PATCH] D52286: [Intrinsic] Signed Saturation Intirnsic

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added a comment.

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

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


My bad. This is my first time touching the LLVM backend so a lot of this is still new to me and I'm still trying to grasp how the IR lowering works, but I think I understand it a little better now and everything's coming together.

So if I understand this correctly, we can handle this intrinsic in 2 ways:

1. The intrinsic gets converted to IR in a pass before touching the DAG.
2. The intrinsic gets passed to the DAG and gets legalized and lowered to machine code there.

It seems that from the thread, the consensus was for going with (2), but I also see now how things would be much easier just having this run through a pass in (1). If I recall correctly, the main reason for offsetting the intrinsic "expansion" to the DAG was to determine if the intrinsic was supported by the hardware during the legalization steps. If so, couldn't this also be done in a pass when converting the intrinsic to IR in (1)? That is, we determine early in the pass if this intrinsic call is supported by this target. Those that aren't legal get expanded into IR whereas those that are legal get passed to DAG instruction selection.

I think this might avoid the trouble of having to check types and operations in the DAG, and ISel can choose whatever appropriate instructions for whatever supported intrinsic (if that makes sense).



================
Comment at: include/llvm/CodeGen/ISDOpcodes.h:264
+    /// signed value is returned instead.
+    SSAT,
+
----------------
ebevhan wrote:
> 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)')
> 
> 
> 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.

I'm not sure if I follow still. (Still learning how the instruction selection process works). So for the fixsmul example, is the problem that during the operation legalization step, the target may not be able to support an int that has N+M bits? Wouldn't this legalization occur during the first type legalization step before operation legalization, like we check if the target can support a N+M bit int before operation legalization? Then during the following optimization stage, have a DAG pass that will expand an illegal intrinsic operation into other DAG nodes.

> 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)')

Also having trouble understanding this one. Is the problem that you don't have access to operand types during the instruction legalization step?


================
Comment at: lib/CodeGen/IntrinsicLowering.cpp:416
+    Ops[1] = CI->getArgOperand(1);
+    ReplaceCallWith("ssaturate", CI, Ops, Ops + 2,
+                    CI->getArgOperand(0)->getType());
----------------
ebevhan wrote:
> 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.
Ah, ok. This is also my first time touching the LLVM backend so I'm not sure if this was the correct place to work on. I also noticed that this code seems to only be touched when using `lli`, so this probably doesn't need to be touched.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5768
+  case Intrinsic::ssaturate: {
+    Value *Src = I.getArgOperand(0);
+    auto *SatBits = dyn_cast<ConstantInt>(I.getArgOperand(1));
----------------
ebevhan wrote:
> This should probably be building an SSAT node rather than actually converting the intrinsic to normal DAG operations.
Ah, ok, I think I'm starting to understand the DAG process better. I tried this earlier, but wasn't sure if that was the correct way to approach this. I initially had `setValue(&I, DAG.getNode(ISD::SSAT, sdl, DAG.getVTList(...), Op1, Op2))` (which I believe creates an SSAT node), but always ran into this fatal error:

```
fatal error: error in backend: Cannot select: t4: i32 = ssaturate t2, Constant:i32<4>
  t2: i32,ch = CopyFromReg t0, Register:i32 %2
    t1: i32 = Register %2
  t3: i32 = Constant<4>
In function: main
```

But I think this is because I didn't make this node type illegal during either legalization step.

If I understand the purpose of this file correctly: this is where the initial build of the DAG is done, then legalization occurs, then for targets where specific intrinsic operations are illegal, this DAG should be "expanded" into other DAG nodes before instruction selection into machine instructions.

If this is the case, where should the actual expansion (which I think is what this code also does) occur? I imagine it would be during the optimization step between the operation legalization and actual machine instruction selection from the DAG.


================
Comment at: lib/Transforms/FixedPoint/SaturationPass.cpp:13
+namespace {
+struct SignedSaturationPass : public FunctionPass {
+  static char ID;
----------------
ebevhan wrote:
> 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.
> 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.

I see, in a sense we have 2 different sets of intrinsics: the saturation and add/sub ones that work generically for integers, and the mul/div ones that care about the scale of the fixed point operands. Do you think it would be ok to propose this as 2 separate passes then where one is specifically applied to fixed point types that take a scale? This would just be creating another directory and renaming this one.


Repository:
  rL LLVM

https://reviews.llvm.org/D52286