[PATCH] D21372: [ARM] Lower (select_cc k k (select_cc ~k ~k x)) into (SSAT l_k x)

[Renato Golin via llvm-commits]

rengolin added a comment.

Thanks Pablo, it's looking much better!

A few remaining comments...

--renato


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:3786
@@ +3785,3 @@
+  SDValue RHS2 = Op2.getOperand(1);
+  ISD::CondCode CC2 = cast<CondCodeSDNode>(Op2.getOperand(4))->get();
+  SDValue TrueVal2 = Op2.getOperand(2);
----------------
Minor nit, maybe move this line down like the other above?

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:3837
@@ +3836,3 @@
+  uint64_t Val2 = cast<ConstantSDNode>(*K2)->getSExtValue();
+  uint64_t PosVal = Val1 < Val2 ? Val1 : Val2;
+
----------------
Wait, this looks backwards...

Shouldn't the positive value be something like:

    std::max(Val1, Val2);

but you seem to be getting the lowest of both sign extended values.

================
Comment at: test/CodeGen/ARM/ssat.ll:183
@@ +182,3 @@
+; CHECK-LABEL: no_sat_incorrect_interval:
+; CHECK-NOT: ssat    r0, #24, r0
+entry:
----------------
change all CHECK-NOT lines to:

    ; CHECK-NOT: ssat

as we don't want *an* ssat being generated, no matter how the test changes in the future.


http://reviews.llvm.org/D21372