[PATCH] D52286: [Intrinsic] Signed Saturation Intirnsic

[Leonard Chan via Phabricator via llvm-commits]

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


Repository:
  rL LLVM

https://reviews.llvm.org/D52286