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

[Roger Ferrer Ibanez via Phabricator via llvm-commits]

rogfer01 added inline comments.


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:4402-4404
+  if (VT != MVT::i32) {
+    return false;
+  }
----------------
For consistency with the rest of the file I think you want to remove the braces here.


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:4417
+  // No constant operation in comparison, early out
+  if (!K) {
+    return false;
----------------
Ditto.


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:4427
+  // does not match, early out
+  if (*K != KTmp || V != VTmp) {
+    return false;
----------------
Ditto


================
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)) {
----------------
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?


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:4468
+          return DAG.getNode(ISD::AND, dl, VT, SatValue, NotShiftV);
+    } else if (isAllOnesConstant(LowerSatConstant)) {
+      return DAG.getNode(ISD::OR, dl, VT, SatValue, ShiftV);
----------------
Braces.


https://reviews.llvm.org/D42574