[PATCH] D52286: [Intrinsic] Signed Saturation Intirnsic

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added inline comments.


================
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
----------------
ebevhan wrote:
> 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.
I wanted to separate them both into 2 functions that could be called after each other on intrinsics that use both saturation and scale. I initially wrote this as the scaled method when I meant saturation, but forgot to redo some of the comments.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeTypes.h:333
   SDValue PromoteIntRes_XMULO(SDNode *N, unsigned ResNo);
+  SDValue PromoteIntRes_SSAT(SDNode *N);
 
----------------
ebevhan wrote:
> 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.
Added a test case for illegal type.


================
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;
----------------
ebevhan wrote:
> 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.
Didn't think about this either, but I also imagine the type will be legalized somehow also after exploring the type legalization code. Regardless, I still made it a TargetConstant.


Repository:
  rL LLVM

https://reviews.llvm.org/D52286