[PATCH] D17859: [InstCombine] convert 'isPositive' and 'isNegative' vector comparisons to shifts (PR26701, PR26819)

[Sanjay Patel via llvm-commits]

spatel created this revision.
spatel added reviewers: t.p.northover, rengolin, majnemer.
spatel added a subscriber: llvm-commits.
Herald added subscribers: mcrosier, aemerson.

This is an update of:
http://reviews.llvm.org/rL262424

That was reverted because it caused failures in an end-to-end clang AArch64 test ( clang/test/CodeGen/aarch64-neon-misc.c ).
That failure no longer happens after:
http://reviews.llvm.org/rL262623
Ie, the AArch64 backend recognizes the pattern produced by this patch and generates the expected code.

There is an end-to-end improvement for AArch64 from this patch that is not currently tested anywhere AFAIK. That is noted in PR26819:
https://llvm.org/bugs/show_bug.cgi?id=26819

This source:
  // Are elements of 'a' <= -1? Ie, is 'a' negative?
  int32x2_t test_vcle_s32(int32x2_t a) {
    return vcle_s32(a, vceq_s32(a, a) );
  }

Becomes:
  test_vcle_s32:
    sshr	v0.2s, v0.2s, #31
    ret

Instead of the current:
  test_vcle_s32:
    movi	d1, #0xffffffffffffffff
    cmge	v0.2s, v1.2s, v0.2s
    ret

Given the current controversy about end-to-end testing, I have not included a test for that in this patch. But it seems to me that we should have that kind of test *somewhere* to make sure that IR optimizations are correctly handled by a backend. In other words, if that current end-to-end AArch clang test didn't exist, I wouldn't have known that this patch pessimized AArch64. It's entirely possible that some other backend will still be pessimized by this change because it has no equivalent end-to-end test. The difference is sufficiently small that it is unlikely to show up as a perf regression anywhere, but it undoubtedly would be a regression.

http://reviews.llvm.org/D17859

Files:
  lib/Transforms/InstCombine/InstCombineCasts.cpp
  test/Transforms/InstCombine/vec_sext.ll

Index: test/Transforms/InstCombine/vec_sext.ll
===================================================================
--- test/Transforms/InstCombine/vec_sext.ll
+++ test/Transforms/InstCombine/vec_sext.ll
@@ -43,3 +43,31 @@
 ; CHECK:   and <4 x i32> %b.lobit.not, %sub
 ; CHECK:   or <4 x i32> %0, %1
 }
+
+;;; PR26701: https://llvm.org/bugs/show_bug.cgi?id=26701
+
+; Signed-less-than-or-equal to -1 is the same operation as above: smear the sign bit.
+
+define <2 x i32> @is_negative(<2 x i32> %a) {
+  %cmp = icmp sle <2 x i32> %a, <i32 -1, i32 -1>
+  %sext = sext <2 x i1> %cmp to <2 x i32>
+  ret <2 x i32> %sext
+
+; CHECK-LABEL: @is_negative(
+; CHECK-NEXT:  ashr <2 x i32> %a, <i32 31, i32 31>
+; CHECK-NEXT:  ret <2 x i32> 
+}
+
+; Signed-greater-than-or-equal to 0 is 'not' of the same operation as above.
+
+define <2 x i32> @is_positive(<2 x i32> %a) {
+  %cmp = icmp sge <2 x i32> %a, zeroinitializer
+  %sext = sext <2 x i1> %cmp to <2 x i32>
+  ret <2 x i32> %sext
+
+; CHECK-LABEL: @is_positive(
+; CHECK-NEXT:  [[SHIFT:%[a-zA-Z0-9.]+]] = ashr <2 x i32> %a, <i32 31, i32 31>
+; CHECK-NEXT:  xor <2 x i32> [[SHIFT]], <i32 -1, i32 -1>
+; CHECK-NEXT:  ret <2 x i32>
+}
+
Index: lib/Transforms/InstCombine/InstCombineCasts.cpp
===================================================================
--- lib/Transforms/InstCombine/InstCombineCasts.cpp
+++ lib/Transforms/InstCombine/InstCombineCasts.cpp
@@ -976,15 +976,27 @@
     // (x <s  0) ? -1 : 0 -> ashr x, 31        -> all ones if negative
     // (x >s -1) ? -1 : 0 -> not (ashr x, 31)  -> all ones if positive
     if ((Pred == ICmpInst::ICMP_SLT && Op1C->isNullValue()) ||
-        (Pred == ICmpInst::ICMP_SGT && Op1C->isAllOnesValue())) {
+        (Pred == ICmpInst::ICMP_SGT && Op1C->isAllOnesValue()) ||
 
+        // The following comparisons should only be present for vectors.
+        // For scalar integers, the comparison should be canonicalized to one of
+        // the above forms. We don't do that canonicalization for vectors
+        // because vector ISAs may not have a full range of comparison
+        // operators. This cmp+sext transform, however, will simplify the IR, so
+        // we always do it.
+        //
+        // (x <=s -1) ? -1 : 0 -> ashr x, 31        -> all ones if negative
+        // (x >=s  0) ? -1 : 0 -> not (ashr x, 31)  -> all ones if positive
+        (Pred == ICmpInst::ICMP_SLE && Op1C->isAllOnesValue()) ||
+        (Pred == ICmpInst::ICMP_SGE && Op1C->isNullValue())) {
       Value *Sh = ConstantInt::get(Op0->getType(),
                                    Op0->getType()->getScalarSizeInBits()-1);
       Value *In = Builder->CreateAShr(Op0, Sh, Op0->getName()+".lobit");
       if (In->getType() != CI.getType())
         In = Builder->CreateIntCast(In, CI.getType(), true/*SExt*/);
 
-      if (Pred == ICmpInst::ICMP_SGT)
+      // Invert the sign bit if the comparison was checking for 'is positive'.
+      if (Pred == ICmpInst::ICMP_SGT || Pred == ICmpInst::ICMP_SGE)
         In = Builder->CreateNot(In, In->getName()+".not");
       return replaceInstUsesWith(CI, In);
     }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D17859.49754.patch
Type: text/x-patch
Size: 3102 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160303/c525d9a0/attachment.bin>