[PATCH] D52286: [Intrinsic] Signed Saturation Intirnsic

Tue Sep 25 01:20:32 PDT 2018

ebevhan added inline comments.

================
Comment at: include/llvm/CodeGen/ISDOpcodes.h:264
+    /// signed value is returned instead.
+    SSAT,
+
----------------
leonardchan wrote:
> 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.
> 
> 
For the multiplications, I now realized there are the UMUL_LOHI and SMUL_LOHI nodes. Technically we want to be doing the operation as iN+M, but i2*N works fine too (and is better; it's more likely to be a legal type) since the scale cannot be larger than the bitwidth. Those nodes should be valid even for an iN type, even if i2*N isn't valid. I don't think there's an equivalent node for divisions, though.

Also, for saturating multiplication, you need to do saturation on the wider type, meaning that you need to do saturation on two separate parts of an integer. This would be the same as doing expanding type legalization on ssat, but since type legalization has already been performed, you can't reuse that part of the code for operation legalization.

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

If you reach LegalizeOp, then the types of the operands and result must be legal since we have passed type legalization. We don't know if the target considers that operation, with that type, //with that scale// to be legal, though. Type legalization won't catch this, and `getOperationAction` does not let you pass enough information to determine it.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:1119
+  case ISD::SSAT:
+    // Target legalization checked here?
+    Action = TargetLowering::Expand;
----------------
leonardchan wrote:
> 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.
You don't need an extra hook to determine anything extra for expansion, but you do need the hook to determine if it's legal in the first place. As I mentioned in another comment, an i16 ssat with a saturating width of 8 might be legal, but an i16 ssat with saturating width 12 might not. The former must not be expanded, but the latter must.

Repository:
  rL LLVM

https://reviews.llvm.org/D52286