[PATCH] D52286: [Intrinsic] Signed Saturation Intirnsic

[Leonard Chan via Phabricator via llvm-commits]

leonardchan marked an inline comment as done.
leonardchan added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:1119
+  case ISD::SSAT:
+    // Target legalization checked here?
+    Action = TargetLowering::Expand;
----------------
ebevhan wrote:
> leonardchan wrote:
> > ebevhan wrote:
> > > 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.
> > Got it. Just to clarify though, legalizations regarding the scale would only apply for our mul/div intrinsics right? Operations like saturation and sat add/sub which don't care about the scale do not need this hook right, since it would also be useful for these operations to work on regular integers?
> Saturating add/sub won't need anything, but I still think saturation requires some extra check to determine if an operation with a specific saturating bitwidth should be legal or expanded. As I mentioned, a saturation with a type might be legal for one saturating bitwidth but not for another.
Added the extra case to `LegalizeOp` and a `getSaturationOperationAction()` method.


Repository:
  rL LLVM

https://reviews.llvm.org/D52286