[SelectOpt] Don't convert constant selects to branches.

[davemgreen]

Selects that choose between two constants will be less profitable to turn into branches, especially if the constants can be folded somehow into the surrounding instructions. They will also be cost modelled in a way that can make them over-optimistically converted to branches, as neither branch will have a latency depth but the constants still need to be materialized.

This patch disabled selectopt for selects with two constant branches. It is currently in the target independent part, as it sounds generic, but I could move it into AArch64 if needed.

[llvmbot]

@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-llvm-analysis

Author: David Green (davemgreen)

Changes

Selects that choose between two constants will be less profitable to turn into branches, especially if the constants can be folded somehow into the surrounding instructions. They will also be cost modelled in a way that can make them over-optimistically converted to branches, as neither branch will have a latency depth but the constants still need to be materialized.

This patch disabled selectopt for selects with two constant branches. It is currently in the target independent part, as it sounds generic, but I could move it into AArch64 if needed.

---

Full diff: https://github.com/llvm/llvm-project/pull/110858.diff

2 Files Affected:

• (modified) llvm/include/llvm/Analysis/TargetTransformInfoImpl.h (+5-1)
• (added) llvm/test/CodeGen/AArch64/selectopt-const.ll (+72)

diff --git a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
index eca8818cc25e62..50040dc8f6165b 100644
--- a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
+++ b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
@@ -407,9 +407,13 @@ class TargetTransformInfoImplBase {
   bool enableSelectOptimize() const { return true; }
 
   bool shouldTreatInstructionLikeSelect(const Instruction *I) {
+    // A select with two constant operands will usually be better left as a
+    // select.
+    using namespace llvm::PatternMatch;
+    if (match(I, m_Select(m_Value(), m_Constant(), m_Constant())))
+      return false;
     // If the select is a logical-and/logical-or then it is better treated as a
     // and/or by the backend.
-    using namespace llvm::PatternMatch;
     return isa<SelectInst>(I) &&
            !match(I, m_CombineOr(m_LogicalAnd(m_Value(), m_Value()),
                                  m_LogicalOr(m_Value(), m_Value())));
diff --git a/llvm/test/CodeGen/AArch64/selectopt-const.ll b/llvm/test/CodeGen/AArch64/selectopt-const.ll
new file mode 100644
index 00000000000000..f10327e136ad13
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/selectopt-const.ll
@@ -0,0 +1,72 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc -mtriple=aarch64-linux-gnu -mcpu=neoverse-v2 -O3 < %s | FileCheck %s
+
+define i32 @test_const(ptr %in1, ptr %in2, ptr %out, i32 %n, ptr %tbl) {
+; CHECK-LABEL: test_const:
+; CHECK:       // %bb.0: // %entry
+; CHECK-NEXT:    cmp w3, #1
+; CHECK-NEXT:    b.lt .LBB0_3
+; CHECK-NEXT:  // %bb.1: // %for.body.preheader
+; CHECK-NEXT:    mov w9, #1267 // =0x4f3
+; CHECK-NEXT:    fmov s1, #1.00000000
+; CHECK-NEXT:    fmov d2, #5.00000000
+; CHECK-NEXT:    mov w8, w3
+; CHECK-NEXT:    movk w9, #16309, lsl #16
+; CHECK-NEXT:    fmov s0, w9
+; CHECK-NEXT:    .p2align 5, , 16
+; CHECK-NEXT:  .LBB0_2: // %for.body
+; CHECK-NEXT:    // =>This Inner Loop Header: Depth=1
+; CHECK-NEXT:    ldr s4, [x1], #4
+; CHECK-NEXT:    ldr w9, [x0], #4
+; CHECK-NEXT:    add w9, w9, #10
+; CHECK-NEXT:    scvtf d3, w9
+; CHECK-NEXT:    fmadd s4, s4, s0, s1
+; CHECK-NEXT:    fabs s4, s4
+; CHECK-NEXT:    fcvt d4, s4
+; CHECK-NEXT:    fdiv d3, d3, d4
+; CHECK-NEXT:    fcmp d3, d2
+; CHECK-NEXT:    cset w9, lt
+; CHECK-NEXT:    subs x8, x8, #1
+; CHECK-NEXT:    ubfiz x9, x9, #4, #32
+; CHECK-NEXT:    ldr s3, [x4, x9]
+; CHECK-NEXT:    fcvtzs w9, s3
+; CHECK-NEXT:    str w9, [x2], #4
+; CHECK-NEXT:    b.ne .LBB0_2
+; CHECK-NEXT:  .LBB0_3: // %for.cond.cleanup
+; CHECK-NEXT:    mov w0, wzr
+; CHECK-NEXT:    ret
+entry:
+  %cmp15 = icmp sgt i32 %n, 0
+  br i1 %cmp15, label %for.body.preheader, label %for.cond.cleanup
+
+for.body.preheader:                               ; preds = %entry
+  %wide.trip.count = zext nneg i32 %n to i64
+  br label %for.body
+
+for.cond.cleanup:                                 ; preds = %for.body, %entry
+  ret i32 0
+
+for.body:                                         ; preds = %for.body.preheader, %for.body
+  %indvars.iv = phi i64 [ 0, %for.body.preheader ], [ %indvars.iv.next, %for.body ]
+  %arrayidx = getelementptr inbounds i32, ptr %in1, i64 %indvars.iv
+  %0 = load i32, ptr %arrayidx, align 4
+  %add = add nsw i32 %0, 10
+  %conv = sitofp i32 %add to double
+  %arrayidx2 = getelementptr inbounds float, ptr %in2, i64 %indvars.iv
+  %1 = load float, ptr %arrayidx2, align 4
+  %mul = fmul fast float %1, 0x3FF6A09E60000000
+  %add3 = fadd fast float %mul, 1.000000e+00
+  %2 = tail call fast float @llvm.fabs.f32(float %add3)
+  %3 = fpext float %2 to double
+  %div = fdiv fast double %conv, %3
+  %cmp5 = fcmp fast olt double %div, 5.000000e+00
+  %idxprom6 = select i1 %cmp5, i64 4, i64 0
+  %arrayidx7 = getelementptr inbounds float, ptr %tbl, i64 %idxprom6
+  %4 = load float, ptr %arrayidx7, align 4
+  %conv8 = fptosi float %4 to i32
+  %arrayidx10 = getelementptr inbounds i32, ptr %out, i64 %indvars.iv
+  store i32 %conv8, ptr %arrayidx10, align 4
+  %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
+  %exitcond.not = icmp eq i64 %indvars.iv.next, %wide.trip.count
+  br i1 %exitcond.not, label %for.cond.cleanup, label %for.body
+}

[sapostolakis]

Makes sense to specialize for constant values but the condition cost also matters.
With a select the condition is on the critical path, while with a branch computing the condition is not blocking the instruction.
Enabling it in a target-independent manner makes sense but a bit worried about any potential downsides.
Was this change motivated by performance diff on some workload/microbenchmark?

[davemgreen]

Makes sense to specialize for constant values but the condition cost also matters. With a select the condition is on the critical path, while with a branch computing the condition is not blocking the instruction. Enabling it in a target-independent manner makes sense but a bit worried about any potential downsides. Was this change motivated by performance diff on some workload/microbenchmark?

Hi. Yeah we found a case where performance was getting worse. I think the new test case captures what was happening, although the real example is a little more complex and had two selects in the loop.

The places where I've seen selectopt really help is where there are cross iteration dependency chains through the select, and a huge dependency chain gets created for the select version that is broken by converting it to a branch. Or if less instruction can be executed thanks to moving instructions into the then/else branch. If both the operands of the select are constant (or I guess loop invariant might be more accurate?) then the same long chains can't happen and the out of order engine should be fine working through the independent iterations. The branch has a higher chance of slowing things down if it gets mis-predicted.

That is my hope at least. I ran some benchmarks and they performed OK overall, giving a 0.8% improvement in geomean for the benchmarks that changed over llvm-test-suite + various version of spec + some other internal benchmarks which all run at the same time. But SelectOpt is going to be relatively noisy as it depends so heavily on the predictability of the branch and how well the branch predictor manages to behave.

[sapostolakis]

Yeah I agree that this is reasonable when considering the value operands and does not miss out on any of the core opportunities of selectopt. I still think the select condition cost might play a role in the cost model, but I see that in your test case and probably in your motivating example the condition was actually expensive and still the select was the optimal choice. I also remember that a heuristic that I tried that focused on the condition was not profitable. So, overall, the proposed change sounds reasonable.

I tested this change on an internal Google macrobenchmark for x86 and the performance was neutral, so I am okay with landing this change, given your observed gains and lack of indication of regressions.

[davemgreen]

Thanks for checking. Let us know if this does cause any problems and we can adjust it as needed.