[PATCH] D42574: [ARM] Lower lower saturate to 0 and lower saturate to -1 using bit-operations

[Marten Svanfeldt via Phabricator via llvm-commits]

thebolt marked 5 inline comments as done.
thebolt added inline comments.


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:4463
+  if (isLowerSaturatingConditional(Op, SatValue, LowerSatConstant)) {
+    SDValue ShiftV = DAG.getNode(ISD::SRA, dl, VT, SatValue, DAG.getConstant(31, dl, VT));
+    if (isNullConstant(LowerSatConstant)) {
----------------
rogfer01 wrote:
> I'm not convinced of the implicit dependency between the function returning false when `Op.getValueType() != MVT::i32` and the 32 bit constant created here.
> 
> I wonder if it would be better (just suggesting! I'm happy to be convinced otherwise :)) if you want to do the check here (instead of inside the function). 
> 
> 
> ```lang=c
> if (VT == MVT::i32 && isLowerSaturatingConditional(Op, SatValue, LowerSatConstant)) {
> ```
> 
> This way the dependency of types is more obvious. Perhaps you foresee that we will call `isLowerSaturatingConditional` in other places and then the we would have to repeat the check?
I buy your argument. Technically there is not any 32-bit constant created (the constant will as far as I can tell have same width as VT) however the transformation from select_cc->bitoperation is only done on 32-bit operations.
I have moved out the condition check on the type as you suggested (and removed it from the function that checks if it is a lower saturate, making it more general)


https://reviews.llvm.org/D42574