[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.

Sorry it took so long, I was put of by some of the comments and tests and kept getting distracted.

Some comments to start... :)

cheers,
--renato


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:3792
@@ -3758,1 +3791,3 @@
 
+  // Try to lower (select_cc k k (select_cc ~k ~k x)) into (SSAT e x),
+  // where 2^e = k
----------------
This comment is misleading (and it took me a while to figure out :).

This is not lowering (select o select), but (select o (lower o (select o upper), which is a completely different case. I think the comment above on the possibilities should actually be here.

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:3850
@@ +3849,3 @@
+          return DAG.getNode(ARMISD::SSAT, dl, VT, R1, SatImmValue);
+        }
+      }
----------------
This is a slightly deeper nesting that we normally like. Can you refactor this into a new function and use early returns?

================
Comment at: test/CodeGen/ARM/ssat.ll:12
@@ +11,3 @@
+
+define i32 @sat24_c1_1(i32 %x) #0 {
+; CHECK-LABEL: sat24_c1_1:
----------------
I'd add the base test for i8 and i16, just to make sure we're getting it right with all the trivially extensible types.

================
Comment at: test/CodeGen/ARM/ssat.ll:29
@@ +28,3 @@
+  %cmp1 = icmp sgt i32 %x, 8388607
+  %cond = select i1 %cmp1, i32 8388607, i32 %x
+  %cond5 = select i1 %cmp, i32 %cond, i32 -8388608
----------------
cond is a bad name and also gave me some time to think about... Maybe max and min would be better names for those.

It might sound picky to comment on those names, but reading tests is the best way to understand what the intention of the code is, and so act much more as documentation than comment lines or commit messages. :)

================
Comment at: test/CodeGen/ARM/ssat.ll:199
@@ +198,3 @@
+
+define i32 @no_sat24_1(i32 %x, i32 %y) #0 {
+; CHECK-LABEL: no_sat24_1:
----------------
Try to give more meaningful names of why not. Is the range wrong? Are they not powers of two? Are the conditions reversed? etc. At the very least, add a comment to that effect.

================
Comment at: test/CodeGen/ARM/ssat.ll:201
@@ +200,3 @@
+; CHECK-LABEL: no_sat24_1:
+; CHECK-NOT: ssat    r0, #24, r0
+entry:
----------------
The NOT tests are better with just "ssat" not with the #24, since you could still emit another sized SSAT and it will still be wrong.


http://reviews.llvm.org/D21372