[PATCH] D52286: [Intrinsic] Signed Saturation Intirnsic

Bevin Hansson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Sep 27 06:43:20 PDT 2018


ebevhan added inline comments.


================
Comment at: include/llvm/CodeGen/ISDOpcodes.h:259
 
+    /// SSAT(X, W) - Perform saturation on a signed value X to fit in W bits. If
+    /// X is greater than the largest signed value that can be represented in W
----------------
This comment doesn't mention that W must be constant.


================
Comment at: include/llvm/CodeGen/TargetLowering.h:808
+
+  /// Some scaled operations may be natively supported by the target but only
+  /// for specific scales. This method allows for checking if the scale is
----------------
This says 'scaled' but currently only looks at saturating operations (and is named as such).

If you want to make it generic, maybe it should be 'getSatOrScaledOperationAction' and corresponding for the hook above? Not sure what to call the parameter, though.


================
Comment at: include/llvm/CodeGen/TargetLowering.h:817
+
+    // This operation is supported but may only work on specific scales.
+    bool Supported;
----------------
'is supported in this type'?


================
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 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.
Looks good!


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:146
+    Res = PromoteIntRes_SSAT(N);
+    break;
+
----------------
Just a nit but this is short enough to go on one line.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:542
+SDValue DAGTypeLegalizer::PromoteIntRes_SSAT(SDNode *N) {
+  SDValue LHS = GetPromotedInteger(N->getOperand(0));
+  SDValue RHS = N->getOperand(1);
----------------
Is this correct in the face of extension to a wider type? I think you want SExtPromotedInteger.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeTypes.h:333
   SDValue PromoteIntRes_XMULO(SDNode *N, unsigned ResNo);
+  SDValue PromoteIntRes_SSAT(SDNode *N);
 
----------------
What about expansion? If you have an i64 ssat and i64 isn't a legal type, then it will likely decide to expand it during type legalization.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5769
+    SDValue Op2 = getValue(I.getArgOperand(1));
+    setValue(&I, DAG.getNode(ISD::SSAT, sdl, Op1.getValueType(), Op1, Op2));
+    return nullptr;
----------------
Do we know if Op2 here works properly if the type of the operand (i32) is not legal? I suspect it will try to legalize it even though it shouldn't (since it's not a real value per se, just a parameterization of the operation).

Perhaps it should be a TargetConstant (which the legalizer won't touch), but I don't know if there's precedent for this.


Repository:
  rL LLVM

https://reviews.llvm.org/D52286





More information about the llvm-commits mailing list