[BreakFalseDeps] ignore function with minsize attribute

[spatel]

Summary

This came up in the x86-specific:
https://bugs.llvm.org/show_bug.cgi?id=43239
...but it seems like a general problem for the BreakFalseDeps pass. Dependencies are always broken by adding some other instruction, so that should be avoided if the overall goal is to minimize size.

[RKSimon]

This would make it policy for -Oz builds to not bother to break dependencies but -Os/-O0+ builds would still do.

Does anything else use BreakFalseDeps?

llvm/lib/CodeGen/BreakFalseDeps.cpp

15–16

NFC

[spatel]

In D67363#1663974, @RKSimon wrote:

This would make it policy for -Oz builds to not bother to break dependencies but -Os/-O0+ builds would still do.

Correct - that matches my interpretation of the vague clang docs:

-Os Like -O2 with extra optimizations to reduce code size.
-Oz Like -Os (and thus -O2), but reduces code size further.

In my experience, -Oz ("minsize") is used to ensure that code fits in some constrained space, so any extras that can be eliminated should be eliminated. Perf is a secondary concern. This is different than -Os ("optsize") where we are trying to balance speed and size.

Does anything else use BreakFalseDeps?

x86 and ARM are the only targets in trunk that use it from what I can see. Both use it the same way: insert dummy (not required for correctness) instructions to avoid known uarch hazards.

[spatel]

Patch updated:
Pre-committed NFC code comment diffs in rL371516.

[spatel]

Adding some ARM folks since ARM is (the only) other in-tree target that would be affected by this change.
It's not strictly stated that dependency-breaking always means adding an instruction, but that’s the practical definition based on x86 and ARM's implementation of this pass's hooks.

[craig.topper]

For VEX instructions don’t we just use the other input register to break the dependency without adding an instruction?

[spatel]

In D67363#1664689, @craig.topper wrote:

For VEX instructions don’t we just use the other input register to break the dependency without adding an instruction?

That sounds right, but it's not controlled by this pass. That's part of memory op folding?
If I'm seeing it correctly, that's already more aggressively optimizing for size than what we're doing here:

// Avoid partial and undef register update stalls unless optimizing for size.
if (!MF.getFunction().hasOptSize() &&
    (hasPartialRegUpdate(MI.getOpcode(), Subtarget, /*ForLoadFold*/true) ||
     shouldPreventUndefRegUpdateMemFold(MF, MI)))
  return nullptr;

[spatel]

Patch updated:
Rebased with improved test coverage for ARM and x86:
rL371526
rL371525

[spatel]

llvm/test/CodeGen/X86/sqrt-partial.ll

41–42

The AVX form of sqrtsd doesn't have a partial reg update, so no difference here.

[craig.topper]

In D67363#1664746, @spatel wrote:

In D67363#1664689, @craig.topper wrote:

For VEX instructions don’t we just use the other input register to break the dependency without adding an instruction?

That sounds right, but it's not controlled by this pass. That's part of memory op folding?
If I'm seeing it correctly, that's already more aggressively optimizing for size than what we're doing here:

// Avoid partial and undef register update stalls unless optimizing for size.
if (!MF.getFunction().hasOptSize() &&
    (hasPartialRegUpdate(MI.getOpcode(), Subtarget, /*ForLoadFold*/true) ||
     shouldPreventUndefRegUpdateMemFold(MF, MI)))
  return nullptr;

We disable folding if it takes away our opportunity to use the other input register. But something still needs to pick a register for the implicit_def operand. I though that was this pass? I think in absense of this pass it may just be set to the xmm0 by the register allocator. Do we have tests where xmm0 isn't the right choice?

[craig.topper]

I believe this test case compiled with avx needs this pass.

define double @minsize(double %x, double %y) minsize {

%t6 = tail call fast double @llvm.sqrt.f64(double %y)
%t= fadd fast double %t6, %y
ret double %t6

}
declare double @llvm.sqrt.f64(double)

[spatel]

In D67363#1664788, @craig.topper wrote:

I believe this test case compiled with avx needs this pass.

define double @minsize(double %x, double %y) minsize {

%t6 = tail call fast double @llvm.sqrt.f64(double %y)
%t= fadd fast double %t6, %y
ret double %t6

}
declare double @llvm.sqrt.f64(double)

Thanks! I didn't understand where that happened - it's in here (and no existing regression tests appear to capture that).
So yes, to limit this pass, we need to use a slightly finer check to distinguish between transforms that are only changing registers vs. adding instructions.
Added more tests with rL371528 and rL371551.

[spatel]

Patch updated:
Refine the places where minsize comes into play (where the target is called to add an instruction via TII->breakPartialRegDependency()).

[spatel]

llvm/test/CodeGen/X86/sqrt-partial.ll

55–56

This is the test showing that the pass is still capable of changing a reg.

[craig.topper]

X86 looks good to me.

[spatel]

Ping - ARM ok?

[efriedma]

llvm/test/CodeGen/ARM/a15-partial-update.ll

59 ↗

(On Diff #219595)

There's a missed optimization here: we could avoid both the false dependence and the extra instruction by modifying the vld1 to a splat load. But generally looks fine.

[spatel]

Patch updated:
Added a TODO comment about using a splat load on the ARM tests.

[spatel]

Marking patch as accepted based on "looks good" and "looks fine". I'll hold off on commit for a ~day in case there are any more comments.

[RKSimon]

LGTM - maybe raise a bug about the missing splat loads in a15-partial-update.ll ?

[spatel]

In D67363#1678217, @RKSimon wrote:

LGTM - maybe raise a bug about the missing splat loads in a15-partial-update.ll ?

Sure - https://bugs.llvm.org/show_bug.cgi?id=43410

llvm/lib/CodeGen/BreakFalseDeps.cpp

 //==- llvm/CodeGen/BreakFalseDeps.cpp - Break False Dependency Fix -*- C++ -*==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 /// \file Break False Dependency pass.
 ///
 /// Some instructions have false dependencies which cause unnecessary stalls.
 /// For example, instructions may write part of a register and implicitly
 /// need to read the other parts of the register. This may cause unwanted
 /// stalls preventing otherwise unrelated instructions from executing in
 /// parallel in an out-of-order CPU.
 /// This pass is aimed at identifying and avoiding these dependencies.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/CodeGen/LivePhysRegs.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/ReachingDefAnalysis.h"
 #include "llvm/CodeGen/RegisterClassInfo.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
if (!MI.isDebugInstr())
       processDefs(&MI);
   }
   processUndefReads(MBB);
 }
 
 bool BreakFalseDeps::runOnMachineFunction(MachineFunction &mf) {
   if (skipFunction(mf.getFunction()))
     return false;
+
+  // This pass adds instructions to remove dependencies. That opposes the goal
+  // of minimizing size.
+  if (mf.getFunction().hasMinSize())
+    return false;
+
   MF = &mf;
   TII = MF->getSubtarget().getInstrInfo();
   TRI = MF->getSubtarget().getRegisterInfo();
   RDA = &getAnalysis<ReachingDefAnalysis>();
 
   RegClassInfo.runOnMachineFunction(mf);
 
   LLVM_DEBUG(dbgs() << "********** BREAK FALSE DEPENDENCIES **********\n");
 
   // Traverse the basic blocks.
   for (MachineBasicBlock &MBB : mf) {
     processBasicBlock(&MBB);
   }
 
   return false;
 }

llvm/test/CodeGen/X86/sqrt-partial.ll

 ; CHECK-NEXT:    sqrtsd %xmm0, %xmm0
 ; CHECK-NEXT:    retq
 ; CHECK-NEXT:  .LBB1_2: # %call.sqrt
 ; CHECK-NEXT:    jmp sqrt # TAILCALL
   %res = tail call double @sqrt(double %val)
   ret double %res
 }
 
 define double @minsize(double %x, double %y) minsize {
 ; CHECK-LABEL: minsize:
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    mulsd %xmm0, %xmm0
 ; CHECK-NEXT:    mulsd %xmm1, %xmm1
 ; CHECK-NEXT:    addsd %xmm0, %xmm1
-; CHECK-NEXT:    xorps %xmm0, %xmm0
 ; CHECK-NEXT:    sqrtsd %xmm1, %xmm0
 ; CHECK-NEXT:    retq
   %t3 = fmul fast double %x, %x
   %t4 = fmul fast double %y, %y
   %t5 = fadd fast double %t3, %t4
   %t6 = tail call fast double @llvm.sqrt.f64(double %t5)
   ret double %t6
 }
 
 declare float @sqrtf(float)
 declare double @sqrt(double)
 declare double @llvm.sqrt.f64(double)

llvm/test/CodeGen/X86/stack-folding-fp-sse42.ll

 
 define float @stack_fold_cvtsd2ss(double %a0) minsize {
 ; CHECK-LABEL: stack_fold_cvtsd2ss:
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    movsd %xmm0, {{[-0-9]+}}(%r{{[sb]}}p) # 8-byte Spill
 ; CHECK-NEXT:    #APP
 ; CHECK-NEXT:    nop
 ; CHECK-NEXT:    #NO_APP
-; CHECK-NEXT:    xorps %xmm0, %xmm0
 ; CHECK-NEXT:    cvtsd2ss {{[-0-9]+}}(%r{{[sb]}}p), %xmm0 # 8-byte Folded Reload
 ; CHECK-NEXT:    retq
   %1 = tail call <2 x i64> asm sideeffect "nop", "=x,~{xmm1},~{xmm2},~{xmm3},~{xmm4},~{xmm5},~{xmm6},~{xmm7},~{xmm8},~{xmm9},~{xmm10},~{xmm11},~{xmm12},~{xmm13},~{xmm14},~{xmm15},~{flags}"()
   %2 = fptrunc double %a0 to float
   ret float %2
 }
 
 define <4 x float> @stack_fold_cvtsd2ss_int(<2 x double> %a0) optsize {

 
 define double @stack_fold_cvtss2sd(float %a0) minsize {
 ; CHECK-LABEL: stack_fold_cvtss2sd:
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    movss %xmm0, {{[-0-9]+}}(%r{{[sb]}}p) # 4-byte Spill
 ; CHECK-NEXT:    #APP
 ; CHECK-NEXT:    nop
 ; CHECK-NEXT:    #NO_APP
-; CHECK-NEXT:    xorps %xmm0, %xmm0
 ; CHECK-NEXT:    cvtss2sd {{[-0-9]+}}(%r{{[sb]}}p), %xmm0 # 4-byte Folded Reload
 ; CHECK-NEXT:    retq
   %1 = tail call <2 x i64> asm sideeffect "nop", "=x,~{xmm1},~{xmm2},~{xmm3},~{xmm4},~{xmm5},~{xmm6},~{xmm7},~{xmm8},~{xmm9},~{xmm10},~{xmm11},~{xmm12},~{xmm13},~{xmm14},~{xmm15},~{flags}"()
   %2 = fpext float %a0 to double
   ret double %2
 }
 
 define <2 x double> @stack_fold_cvtss2sd_int(<4 x float> %a0) optsize {

 
 define float @stack_fold_roundss(float %a0) minsize {
 ; CHECK-LABEL: stack_fold_roundss:
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    movss %xmm0, {{[-0-9]+}}(%r{{[sb]}}p) # 4-byte Spill
 ; CHECK-NEXT:    #APP
 ; CHECK-NEXT:    nop
 ; CHECK-NEXT:    #NO_APP
-; CHECK-NEXT:    xorps %xmm0, %xmm0
 ; CHECK-NEXT:    roundss $9, {{[-0-9]+}}(%r{{[sb]}}p), %xmm0 # 4-byte Folded Reload
 ; CHECK-NEXT:    retq
   %1 = tail call <2 x i64> asm sideeffect "nop", "=x,~{xmm1},~{xmm2},~{xmm3},~{xmm4},~{xmm5},~{xmm6},~{xmm7},~{xmm8},~{xmm9},~{xmm10},~{xmm11},~{xmm12},~{xmm13},~{xmm14},~{xmm15},~{flags}"()
   %2 = call float @llvm.floor.f32(float %a0)
   ret float %2
 }
 declare float @llvm.floor.f32(float) nounwind readnone
 

 
 define float @stack_fold_sqrtss(float %a0) minsize {
 ; CHECK-LABEL: stack_fold_sqrtss:
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    movss %xmm0, {{[-0-9]+}}(%r{{[sb]}}p) # 4-byte Spill
 ; CHECK-NEXT:    #APP
 ; CHECK-NEXT:    nop
 ; CHECK-NEXT:    #NO_APP
-; CHECK-NEXT:    xorps %xmm0, %xmm0
 ; CHECK-NEXT:    sqrtss {{[-0-9]+}}(%r{{[sb]}}p), %xmm0 # 4-byte Folded Reload
 ; CHECK-NEXT:    retq
   %1 = tail call <2 x i64> asm sideeffect "nop", "=x,~{xmm1},~{xmm2},~{xmm3},~{xmm4},~{xmm5},~{xmm6},~{xmm7},~{xmm8},~{xmm9},~{xmm10},~{xmm11},~{xmm12},~{xmm13},~{xmm14},~{xmm15},~{flags}"()
   %2 = call float @llvm.sqrt.f32(float %a0)
   ret float %2
 }
 declare float @llvm.sqrt.f32(float) nounwind readnone