[AArch64] Avoid materializing constant values when generating csel instructions.

[mcrosier]

Summary

Please take a look.

Chad

[efriedma]

test/CodeGen/AArch64/arm64-csel.ll

241 ↗

(On Diff #68561)

Currently, trunk generates the following for foo20:

cmp     w0, #1
orr     w8, wzr, #0x1
cneg    w0, w8, ne
ret

This is "w0 == 1 ? 1 : -1"

With your patch, we generate... this?

cmp     w0, #1
cneg    w0, w0, ne
ret

Unless I'm missing something, this is "w0 == 1 ? w0 : -w0", which is not the same thing.

[mcrosier]

test/CodeGen/AArch64/arm64-csel.ll

241 ↗

(On Diff #68561)

Ah, yes.. For example, if w0 holds 5 the cneg would put -5 in w0, rather than -1.

[mcrosier]

I've uploaded a new patch, which was my original solution to solving PR28965. My first patch tried to be more general, but obviously made an incorrect assumption.

[efriedma]

lib/Target/AArch64/AArch64ISelLowering.cpp

4001 ↗

(On Diff #68597)

There are really two transformations going on here...

1. You can avoid materializing -1 using CSINV on the zero register. (Similarly, you can avoid materializing 1 with CSINC on the zero register.)
2. You can transform "a == C ? C : x" to "a == C ? a : x" to avoid materializing C.

We want to perform each of these transformations whether or not the other is possible.

[mcrosier]

lib/Target/AArch64/AArch64ISelLowering.cpp

4001 ↗

(On Diff #68597)

I made the same realization last night.

I've been working on a new patch this morning. This general transform (#2) is applicable to CSEL, CSNEG, CSINV, etc. Transform #1 is specific to CSINV and is independent of tranform #2 as you've already pointed out.

I'll post a revised patch shortly.

[mcrosier]

-Address Eli's comment.

[efriedma]

Possibly interesting testcase: "x == 7 ? 7 : -1". Should generate cmp+csinv. Also, the related "x == 7 ? 7 : 1" should generate cmp+csinc.

lib/Target/AArch64/AArch64ISelLowering.cpp

4003 ↗

(On Diff #69006)

Maybe you could just make this fall through to the general case, rather than explicitly selecting CSINV?

4015 ↗

(On Diff #69006)

Maybe it makes sense to write something more like if ((TrueVal == SignExtend64(FalseVal + 1, ValueBits)) || (SignExtend64(TrueVal + 1, ValueBits) == FalseVal)) {. This also applies to your later code.

[mcrosier]

I found very few cases where the csinc/csinv optimizations actually hit, so I'm limiting this patch to just csel to simplify the logic/review.

[mcrosier]

lib/Target/AArch64/AArch64ISelLowering.cpp

4002 ↗

(On Diff #69393)

Deferring this work for another day.

4005 ↗

(On Diff #69393)

Again, deferring this for now.

[gberry]

lib/Target/AArch64/AArch64ISelLowering.cpp

4039 ↗

(On Diff #69393)

typo "know" -> "known"

[mcrosier]

Address Geoff's comment.

[efriedma]

LGTM.

llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp

if (CTVal && CFVal && CTVal->isAllOnesValue() && CFVal->isNullValue()) {
 
       if (Opcode != AArch64ISD::CSEL) {
         // Drop FVal since we can get its value by simply inverting/negating
         // TVal.
         FVal = TVal;
       }
     }
 
+    // Avoid materializing a constant when possible by reusing a known value in
+    // a register.  However, don't perform this optimization if the known value
+    // is one, zero or negative one.  We can always materialize these values
+    // using CSINC, CSEL and CSINV with wzr/xzr as the FVal, respectively.
+    ConstantSDNode *RHSVal = dyn_cast<ConstantSDNode>(RHS);
+    if (Opcode == AArch64ISD::CSEL && RHSVal && !RHSVal->isOne() &&
+        !RHSVal->isNullValue() && !RHSVal->isAllOnesValue()) {
+      AArch64CC::CondCode AArch64CC = changeIntCCToAArch64CC(CC);
+      // Transform "a == C ? C : x" to "a == C ? a : x" and "a != C ? x : C" to
+      // "a != C ? x : a" to avoid materializing C.
+      if (CTVal && CTVal == RHSVal && AArch64CC == AArch64CC::EQ)
+        TVal = LHS;
+      else if (CFVal && CFVal == RHSVal && AArch64CC == AArch64CC::NE)
+        FVal = LHS;
+    }
+
     SDValue CCVal;
     SDValue Cmp = getAArch64Cmp(LHS, RHS, CC, CCVal, DAG, dl);
 
     EVT VT = TVal.getValueType();
     return DAG.getNode(Opcode, dl, VT, TVal, FVal, CCVal, Cmp);
   }
 
   // Now we know we're dealing with FP values.

llvm/trunk/test/CodeGen/AArch64/cond-sel-value-prop.ll

+; RUN: llc -verify-machineinstrs < %s -mtriple=aarch64-none-linux-gnu | FileCheck %s
+
+; Transform "a == C ? C : x" to "a == C ? a : x" to avoid materializing C.
+; CHECK-LABEL: test1:
+; CHECK: cmp w[[REG1:[0-9]+]], #2
+; CHECK: orr w[[REG2:[0-9]+]], wzr, #0x7
+; CHECK: csel w0, w[[REG1]], w[[REG2]], eq
+define i32 @test1(i32 %x) {
+  %cmp = icmp eq i32 %x, 2
+  %res = select i1 %cmp, i32 2, i32 7
+  ret i32 %res
+}
+
+; Transform "a == C ? C : x" to "a == C ? a : x" to avoid materializing C.
+; CHECK-LABEL: test2:
+; CHECK: cmp x[[REG1:[0-9]+]], #2
+; CHECK: orr w[[REG2:[0-9]+]], wzr, #0x7
+; CHECK: csel x0, x[[REG1]], x[[REG2]], eq
+define i64 @test2(i64 %x) {
+  %cmp = icmp eq i64 %x, 2
+  %res = select i1 %cmp, i64 2, i64 7
+  ret i64 %res
+}
+
+; Transform "a != C ? x : C" to "a != C ? x : a" to avoid materializing C.
+; CHECK-LABEL: test3:
+; CHECK: cmp x[[REG1:[0-9]+]], #7
+; CHECK: orr w[[REG2:[0-9]+]], wzr, #0x2
+; CHECK: csel x0, x[[REG2]], x[[REG1]], ne
+define i64 @test3(i64 %x) {
+  %cmp = icmp ne i64 %x, 7
+  %res = select i1 %cmp, i64 2, i64 7
+  ret i64 %res
+}
+
+; Don't transform "a == C ? C : x" to "a == C ? a : x" if a == 0.  If we did we
+; would needlessly extend the live range of x0 when we can just use xzr.
+; CHECK-LABEL: test4:
+; CHECK: cmp x0, #0
+; CHECK: orr w8, wzr, #0x7
+; CHECK: csel x0, xzr, x8, eq
+define i64 @test4(i64 %x) {
+  %cmp = icmp eq i64 %x, 0
+  %res = select i1 %cmp, i64 0, i64 7
+  ret i64 %res
+}
+
+; Don't transform "a == C ? C : x" to "a == C ? a : x" if a == 1.  If we did we
+; would needlessly extend the live range of x0 when we can just use xzr with
+; CSINC to materialize the 1.
+; CHECK-LABEL: test5:
+; CHECK: cmp x0, #1
+; CHECK: orr w[[REG:[0-9]+]], wzr, #0x7
+; CHECK: csinc x0, x[[REG]], xzr, ne
+define i64 @test5(i64 %x) {
+  %cmp = icmp eq i64 %x, 1
+  %res = select i1 %cmp, i64 1, i64 7
+  ret i64 %res
+}
+
+; Don't transform "a == C ? C : x" to "a == C ? a : x" if a == -1.  If we did we
+; would needlessly extend the live range of x0 when we can just use xzr with
+; CSINV to materialize the -1.
+; CHECK-LABEL: test6:
+; CHECK: cmn x0, #1
+; CHECK: orr w[[REG:[0-9]+]], wzr, #0x7
+; CHECK: csinv x0, x[[REG]], xzr, ne
+define i64 @test6(i64 %x) {
+  %cmp = icmp eq i64 %x, -1
+  %res = select i1 %cmp, i64 -1, i64 7
+  ret i64 %res
+}
+
+; CHECK-LABEL: test7:
+; CHECK: cmp x[[REG:[0-9]]], #7
+; CHECK: csinc x0, x[[REG]], xzr, eq
+define i64 @test7(i64 %x) {
+  %cmp = icmp eq i64 %x, 7
+  %res = select i1 %cmp, i64 7, i64 1
+  ret i64 %res
+}
+
+; CHECK-LABEL: test8:
+; CHECK: cmp x[[REG:[0-9]]], #7
+; CHECK: csinc x0, x[[REG]], xzr, eq
+define i64 @test8(i64 %x) {
+  %cmp = icmp ne i64 %x, 7
+  %res = select i1 %cmp, i64 1, i64 7
+  ret i64 %res
+}
+
+; CHECK-LABEL: test9:
+; CHECK: cmp x[[REG:[0-9]]], #7
+; CHECK: csinv x0, x[[REG]], xzr, eq
+define i64 @test9(i64 %x) {
+  %cmp = icmp eq i64 %x, 7
+  %res = select i1 %cmp, i64 7, i64 -1
+  ret i64 %res
+}