[PATCH] D52286: [Intrinsic] Signed Saturation Intirnsic

Mon Sep 24 14:29:34 PDT 2018

leonardchan added a comment.

Of the intrinsics we plan to implement, it seems that the ordering of legalization affects specifically the fixed point mul/div intrinsics since if they get expanded into other nodes, we will need to check again for legal types since performing scaled mul/div requires larger integers for a buffer. The remaining ones though I don't think require any extra legalization. Both saturation and saturated add/sub can be done without different sized operands.

For ssat specifically, I think this patch has most of the necessary code to have this ready for a formal review. I removed the passes for this one since it seems all expansion can be done safely in the DAG without any extra type or operation legalization.

================
Comment at: include/llvm/CodeGen/ISDOpcodes.h:264
+    /// signed value is returned instead.
+    SSAT,
+
----------------
ebevhan wrote:
> leonardchan wrote:
> > 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?
> > 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?
> 
> Correct. Operation legalization (as I understand it; I could be wrong!) can not introduce illegal types into the DAG since type legalization occurs first. Say that you have a 'i16 fixsmul(X, Y, 15)' on a target where i16 is legal and i32 is not, and the fixsmul is not legal. This passes type legalization just fine, because i16 is legal. Then we get to operation legalization. It determines that the fixsmul node is not legal, so it needs to either be expanded or promoted according to what the target says (generally from TLI.getOperationAction).
> 
> However, we know how a fixsmul should be lowered. It needs to be done as just a regular mul in a wider type; but we can't make this wider type, because it's not legal! If the target specified Expand (which it probably did in this case, since i32 is not legal), we need to do it as a bunch of i8 operations*. This just seems really messy to me; it would have been much cleaner if it had just been properly promoted from the start and we let isel legalize the higher level lowering instead.
> 
> *I'm unsure about this. It might be the case that if we need to expand an illegal node with a legal type, it is allowed to do the lowered operations in the legal type anyway, even though we said expand.
> 
> > 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.
> 
> If the type of the node is legal, type legalization won't do anything. And N+M necessarily being legal for an N-bit fixsmul is not a requirement, since the fixsmul might itself be a legal, actual target operation, in which case the legality of N+M doesn't matter. I suppose you could ask about N+M if the node isn't legal as N, but that mixes type and operation legalization (since type legalization would then depend on whether an operation was legal in a type that we haven't even seen) and I'm not sure if the existing implementation ever does this.
> 
> > Also having trouble understanding this one. Is the problem that you don't have access to operand types during the instruction legalization step?
> 
> Generally, DAG will call `TLI.getOperationAction` to determine the legality of operations. It passes the opcode and the type, but we need more than that to determine if one of these operations is legal. A new hook would have to be added to TargetLowering that takes both the opcode, the type and the scaling factor/saturating bitwidth of the node. I think I mentioned it in another comment.
> However, we know how a fixsmul should be lowered. It needs to be done as just a regular mul in a wider type; but we can't make this wider type, because it's not legal! If the target specified Expand (which it probably did in this case, since i32 is not legal), we need to do it as a bunch of i8 operations*. This just seems really messy to me; it would have been much cleaner if it had just been properly promoted from the start and we let isel legalize the higher level lowering instead.

I see. I'll add John on this patch also to get his opinion and see if anyone else has more suggestions on llvm-dev.

> If the type of the node is legal, type legalization won't do anything. And N+M necessarily being legal for an N-bit fixsmul is not a requirement, since the fixsmul might itself be a legal, actual target operation, in which case the legality of N+M doesn't matter. I suppose you could ask about N+M if the node isn't legal as N, but that mixes type and operation legalization (since type legalization would then depend on whether an operation was legal in a type that we haven't even seen) and I'm not sure if the existing implementation ever does this.

Ah, so it seems what we really need is a way to check if the operation is legal first before the types, because if the operation is supported, then we don't need to care about any other requirements on the expanded operations (like the N+M bit int for an expanded fixsmul). In this case, it seems like this would be another reason to have the LLVM pass that also does some special legalization for the intrinsic.

Alternatively, it seems that we actually have all the information we need in the `LegalizeOp` step, that is we know which intrinsic we have, if this target supports this operation, and the bit widths of the operands and result in the event it is unsupported and needs to be expanded into other operations.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:1119
+  case ISD::SSAT:
+    // Target legalization checked here?
+    Action = TargetLowering::Expand;
----------------
ebevhan wrote:
> leonardchan wrote:
> > @ebevhan Do you know if this is where checking if this intrinsic is supported by the target should be? 
> Yes, this would be it. For nodes like ssat and fixed-point ops, you could ask a custom target hook, maybe `TLI.getScaledOperationAction(<opcode>, <type>, <scale/sat_bw>)` to determine the legality of a given node.
> 
> When type-legalizing you also need to handle things for specific nodes as well, since things behave differently when you legalize operands/results on different nodes.
> 
> (Of course, this all depends on whether we do the legalization here or in IR instead.)
Made the default action `Expand` for this in `TargetLoweringBase` and targets can override this themselves.

I think ssat may not need a custom hook since I don't think it requires anything extra like fixsmul if it has to be expanded. But I see what you mean now by how different nodes may require different steps for legalization.

Repository:
  rL LLVM

https://reviews.llvm.org/D52286