Enable MachineOutliner by default under -Oz for AArch64

[paquette]

Summary

This patch enables the MachineOutliner by default in AArch64 under -Oz.

The MachineOutliner offers around a 4.5% improvement on the current -Oz code size improvements.

We have done work into improving the debuggability of outlined code, so that users of -Oz won't be surprised by the optimization. We have also been executing the LLVM test suite and common external tests such as the SPEC suites continuously with no issue. The outliner has a low compile-time overhead of roughly 1%. At this point, the outliner would be a really good addition to the -Oz pass pipeline!

[javed.absar]

Could you please generate diff with -U999 to give more context to the patch. Thanks.

[paquette]

Added context to diff.

[efriedma]

lib/CodeGen/TargetPassConfig.cpp

120 ↗

(On Diff #143583)

Instead of having two flags, can you just check getNumOccurrences()?

[paquette]

Updated diff to reflect review comments and discussion on the RFC. In the RFC, we decided that it'd be better to base the decision to outline off of function attributes for compatibility with LTO and extensibility of the outliner. This is a far less invasive change, and means that every patch other than this can be abandoned.

I also added a comment on the "-enable-machine-outliner" flag saying what it's *actually* supposed to mean after the application of this patch. In a follow-up patch, we should change this flag to something that better implies what the flag actually does. The only reason I didn't do that here is that I'd have to update every test in *this* patch which is a little too much noise for review.

[paquette]

lib/CodeGen/TargetPassConfig.cpp

120 ↗

(On Diff #143583)

"enable-machine-outliner" implies "run it on everything, even things the target doesn't want by default". This isn't actually symmetric with "disable-machine-outliner", which says "don't run it on anything at all". I think "enable-machine-outliner" should be renamed to something that better describes the intended behaviour. I'll do that in a follow-up patch.

[efriedma]

lib/CodeGen/TargetPassConfig.cpp

904 ↗

(On Diff #144158)

We probably need to check getOptLevel() != CodeGenOpt::None here.

120 ↗

(On Diff #143583)

In other places, we sometimes do something like "true is always, false is never, unspecified is the default". But I guess this isn't really the same thing.

Could you rename the flags to be more clear? (-disable-machine-outliner-pass vs. -machine-outliner-all-functions).

[paquette]

Updated patch for review. Lots of tests modified this time because in this round the outliner is *always* added to the pass pipleline. It then decides whether or not it should run based off of user input/target default behaviour.

Also all the outliner tests have been updated because -enable-machine-outliner now accepts 3 choices: always, default, and never.

-enable-machine-outliner=always => Run the outliner on every function (modulo those with linkonceodr linkage)
-enable-machine-outliner=default => Run the outliner on functions the target decides should always be outlined from (e.g, in this case, those with the minsize attribute in AArch64)
-enable-machine-outliner=never => Do nothing and return false

[paquette]

lib/CodeGen/TargetPassConfig.cpp

120 ↗

(On Diff #143583)

Done for the current version of the patch

[paquette]

Funnily enough, this version of the patch uncovered a bug in a different pass, which caused its lit tests to fail (D46330).

Since I just committed the fix in r331307, this should pass every lit test.

[efriedma]

The code in this patch looks fine.

In terms of the policy decision of actually turning it on by default, I'm mostly comfortable with it. Outlining has gotten a reasonable amount of testing, and I think the AArch64 hooks are correct. One potential issue which I still need to look into is whether we need to disable outlining for fragments which use r16 and r17; it would be rare and difficult to reproduce, but the linker could theoretically insert a veneer that clobbers those registers in some cases.

[yroux]

Hi Jessica,

I passed your patch into our pre-commit validation (on x86_64, AArch64 and ARM boxes) and besides the 2 failures on the recently added AArch64 outliner testcases (which need to be updated) I got regressions on AMDGPU when -march=5600 is used and on PowerPC when mcpu=pwr7 is used without -mattr=-crbits, and I've troubles to figure out why...

Here are the logs for x86: https://ci.linaro.org/job/tcwg-llvm-staged-build/140/label=tcwg-x86_64-cam,target=tcwg-x86_64-cam/console

[paquette]

Updated the patch so that it no longer causes issues for the AMDGPU target. The outliner is now only added to the pass pipeline if a target specifies that it supports it in its TargetOptions. After this, whether or not a target supports the outliner must be defined within a specific TargetMachine (similar to GlobalIsel).

It seems like although the outliner wasn't actually *doing* anything in the AMDGPU target, the verifier was running on the module after it was added, raising issues there.

[efriedma]

This is getting complicated enough that I'd like to see it split into two patches; one for the MachineOutliner option/configuration changes, and one to actually change the default for AArch64.

[paquette]

Yeah this is too big. I'll split this up into a few patches then update this one to just reflect turning the outliner on once I'm done...

[paquette]

Updated the patch so that it relies on the changes in D48776. That patch provides the scaffolding for allowing the outliner to be turned on by default by some target. This one flips the switch on AArch64 for -Oz.

[efriedma]

LGTM.

test/CodeGen/AArch64/O3-pipeline.ll

158

I'm a little concerned about the impact of this change on peak memory usage, but hopefully it isn't a big deal in practice.

[paquette]

Thanks!

Committed in r338133. (https://reviews.llvm.org/rL338133)

lib/Target/AArch64/AArch64InstrInfo.h

public:
   getOutliningType(MachineBasicBlock::iterator &MIT, unsigned Flags) const override;
   unsigned getMachineOutlinerMBBFlags(MachineBasicBlock &MBB) const override;
   void buildOutlinedFrame(MachineBasicBlock &MBB, MachineFunction &MF,
                             const outliner::TargetCostInfo &TCI) const override;
   MachineBasicBlock::iterator
   insertOutlinedCall(Module &M, MachineBasicBlock &MBB,
                      MachineBasicBlock::iterator &It, MachineFunction &MF,
                      const outliner::TargetCostInfo &TCI) const override;
+  bool shouldOutlineFromFunctionByDefault(MachineFunction &MF) const override;
   /// Returns true if the instruction sets to an immediate value that can be
   /// executed more efficiently.
   bool isExynosResetFast(const MachineInstr &MI) const;
   /// Returns true if the instruction has a shift left that can be executed
   /// more efficiently.
   bool isExynosShiftLeftFast(const MachineInstr &MI) const;
   /// Returns true if the instruction has a shift by immediate that can be
   /// executed in one cycle less.

lib/Target/AArch64/AArch64InstrInfo.cpp

MachineInstr *LDRXpost = BuildMI(MF, DebugLoc(), get(AArch64::LDRXpost))
                                .addReg(AArch64::SP, RegState::Define)
                                .addReg(AArch64::LR, RegState::Define)
                                .addReg(AArch64::SP)
                                .addImm(16);
   It = MBB.insert(It, LDRXpost);
 
   return CallPt;
 }
+
+bool AArch64InstrInfo::shouldOutlineFromFunctionByDefault(
+  MachineFunction &MF) const {
+  return MF.getFunction().optForMinSize();
+}

lib/Target/AArch64/AArch64TargetMachine.cpp

AArch64TargetMachine::AArch64TargetMachine(const Target &T, const Triple &TT,
   }
 
   // Enable GlobalISel at or below EnableGlobalISelAt0.
   if (getOptLevel() <= EnableGlobalISelAtO)
     setGlobalISel(true);
 
   // AArch64 supports the MachineOutliner.
   setMachineOutliner(true);
+
+  // AArch64 supports default outlining behaviour.
+  setSupportsDefaultOutlining(true);
 }
 
 AArch64TargetMachine::~AArch64TargetMachine() = default;
 
 const AArch64Subtarget *
 AArch64TargetMachine::getSubtargetImpl(const Function &F) const {
   Attribute CPUAttr = F.getFnAttribute("target-cpu");
   Attribute FSAttr = F.getFnAttribute("target-features");

test/CodeGen/AArch64/O3-pipeline.ll

 ; CHECK-NEXT:       Branch Probability Basic Block Placement
 ; CHECK-NEXT:       Branch relaxation pass
 ; CHECK-NEXT:       Contiguously Lay Out Funclets
 ; CHECK-NEXT:       StackMap Liveness Analysis
 ; CHECK-NEXT:       Live DEBUG_VALUE analysis
 ; CHECK-NEXT:       Insert fentry calls
 ; CHECK-NEXT:       Insert XRay ops
 ; CHECK-NEXT:       Implement the 'patchable-function' attribute
+; CHECK-NEXT:     Machine Outliner
+; CHECK-NEXT:     FunctionPass Manager
 ; CHECK-NEXT:       Lazy Machine Block Frequency Analysis
 ; CHECK-NEXT:       Machine Optimization Remark Emitter
 ; CHECK-NEXT:       AArch64 Assembly Printer
 ; CHECK-NEXT:       Free MachineFunction
 ; CHECK-NEXT: Pass Arguments:  -domtree
 ; CHECK-NEXT:   FunctionPass Manager
 ; CHECK-NEXT:     Dominator Tree Construction
 
 define void @f() {
   ret void
 }

test/CodeGen/AArch64/arm64-memset-to-bzero.ll

-; RUN: llc %s -mtriple=arm64-apple-darwin -o - | \
-; RUN:   FileCheck --check-prefixes=CHECK,CHECK-DARWIN %s
-; RUN: llc %s -mtriple=arm64-linux-gnu -o - | \
+; RUN: llc %s -enable-machine-outliner=never -mtriple=arm64-apple-darwin -o - \
+; RUN: | FileCheck --check-prefixes=CHECK,CHECK-DARWIN %s
+; RUN: llc %s -enable-machine-outliner=never -mtriple=arm64-linux-gnu -o - | \
 ; RUN:   FileCheck --check-prefixes=CHECK,CHECK-LINUX %s
 ; <rdar://problem/14199482> ARM64: Calls to bzero() replaced with calls to memset()
 
 ; CHECK-LABEL: fct1:
 ; For small size (<= 256), we do not change memset to bzero.
 ; CHECK-DARWIN: {{b|bl}} _memset
 ; CHECK-LINUX: {{b|bl}} memset
 define void @fct1(i8* nocapture %ptr) minsize {

test/CodeGen/AArch64/arm64-opt-remarks-lazy-bfi.ll

 ; HOTNESS-NEXT: MachineBlockFrequencyInfo is available
 ; HOTNESS-NEXT: Executing Pass 'Greedy Register Allocator'
 
 
 ; Verify that we only populate MachineBFI on behalf of ORE when hotness is
 ; requested.  (This hard-codes the previous pass to the Assembly Printer,
 ; please adjust accordingly.)
 
-; HOTNESS:      Executing Pass 'Implement the 'patchable-function' attribute'
-; HOTNESS-NEXT:  Freeing Pass 'Implement the 'patchable-function' attribute'
+; HOTNESS:      Freeing Pass 'Machine Outliner'
+; HOTNESS-NEXT:  Executing Pass 'Function Pass Manager'
 ; HOTNESS-NEXT: Executing Pass 'Lazy Machine Block Frequency Analysis'
 ; HOTNESS-NEXT: Executing Pass 'Machine Optimization Remark Emitter'
 ; HOTNESS-NEXT: Building MachineBlockFrequencyInfo on the fly
 ; HOTNESS-NEXT: Building LoopInfo on the fly
 ; HOTNESS-NEXT: Building DominatorTree on the fly
 ; HOTNESS-NOT: Executing Pass
 ; HOTNESS: block-frequency: empty_func
 ; HOTNESS-NOT: Executing Pass
 ; HOTNESS: Executing Pass 'AArch64 Assembly Printer'
 
 ; HOTNESS: arm64-summary-remarks.ll:5:0: 1 instructions in function (hotness: 33)
 
 
-; NO_HOTNESS:      Executing Pass 'Implement the 'patchable-function' attribute'
-; NO_HOTNESS-NEXT:  Freeing Pass 'Implement the 'patchable-function' attribute'
+; NO_HOTNESS:      Freeing Pass 'Machine Outliner'
+; NO_HOTNESS-NEXT:  Executing Pass 'Function Pass Manager'
 ; NO_HOTNESS-NEXT: Executing Pass 'Lazy Machine Block Frequency Analysis'
 ; NO_HOTNESS-NEXT: Executing Pass 'Machine Optimization Remark Emitter'
 ; NO_HOTNESS-NEXT: Executing Pass 'AArch64 Assembly Printer'
 
 ; NO_HOTNESS: arm64-summary-remarks.ll:5:0: 1 instructions in function{{$}}
 
 define void @empty_func() nounwind ssp !dbg !3 !prof !4 {
   ret void

test/CodeGen/AArch64/cond-sel.ll

-; RUN: llc -verify-machineinstrs < %s -mtriple=aarch64-none-linux-gnu -mcpu=cyclone | FileCheck %s
-; RUN: llc -verify-machineinstrs < %s -mtriple=aarch64-none-linux-gnu -mattr=-fp-armv8 | FileCheck --check-prefix=CHECK-NOFP %s
+; RUN: llc -enable-machine-outliner=never -verify-machineinstrs < %s -mtriple=aarch64-none-linux-gnu -mcpu=cyclone | FileCheck %s
+; RUN: llc -enable-machine-outliner=never -verify-machineinstrs < %s -mtriple=aarch64-none-linux-gnu -mattr=-fp-armv8 | FileCheck --check-prefix=CHECK-NOFP %s
 
 @var32 = global i32 0
 @var64 = global i64 0
 
 define void @test_csel(i32 %lhs32, i32 %rhs32, i64 %lhs64) minsize {
 ; CHECK-LABEL: test_csel:
 
   %tst1 = icmp ugt i32 %lhs32, %rhs32

test/CodeGen/AArch64/machine-outliner-default.mir

+# RUN: llc -mtriple=aarch64--- -run-pass=machine-outliner \
+# RUN: -verify-machineinstrs %s -o - | FileCheck %s
+
+--- |
+  define void @outline_1() #0 { ret void }
+  define void @outline_2() #0 { ret void }
+  define void @outline_3() #0 { ret void }
+  define void @dont_outline() #1 { ret void }
+
+  attributes #0 = { noredzone minsize optsize }
+  attributes #1 = { noredzone }
+...
+---
+
+name:           outline_1
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    ; CHECK-LABEL: bb.0:
+    ; CHECK: OUTLINED
+    liveins: $w8, $wzr
+    $w8 = ORRWri $wzr, 1
+    $w8 = ORRWri $wzr, 2
+    $w8 = ORRWri $wzr, 3
+    $w8 = ORRWri $wzr, 4
+    RET undef $lr
+...
+---
+
+name:           outline_2
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    ; CHECK-LABEL: bb.0:
+    ; CHECK: OUTLINED
+    liveins: $w8, $wzr
+    $w8 = ORRWri $wzr, 1
+    $w8 = ORRWri $wzr, 2
+    $w8 = ORRWri $wzr, 3
+    $w8 = ORRWri $wzr, 4
+    RET undef $lr
+...
+---
+
+name:           outline_3
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    ; CHECK-LABEL: bb.0:
+    ; CHECK: OUTLINED
+    liveins: $w8, $wzr
+    $w8 = ORRWri $wzr, 1
+    $w8 = ORRWri $wzr, 2
+    $w8 = ORRWri $wzr, 3
+    $w8 = ORRWri $wzr, 4
+    RET undef $lr
+...
+---
+
+name:           dont_outline
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    ; CHECK-LABEL: bb.0:
+    ; CHECK-NOT: BL
+    liveins: $w8, $wzr
+    $w8 = ORRWri $wzr, 1
+    $w8 = ORRWri $wzr, 2
+    $w8 = ORRWri $wzr, 3
+    $w8 = ORRWri $wzr, 4
+    RET undef $lr

test/CodeGen/AArch64/machine-outliner-flags.ll

 ; REQUIRES: asserts
 ; RUN: llc %s -debug-pass=Structure -verify-machineinstrs \
 ; RUN: --debug-only=machine-outliner -enable-machine-outliner=always \
 ; RUN: -mtriple arm64---- -o /dev/null 2>&1 \
 ; RUN: | FileCheck %s -check-prefix=ALWAYS
 
 ; RUN: llc %s -debug-pass=Structure -verify-machineinstrs \
 ; RUN: --debug-only=machine-outliner -enable-machine-outliner \
 ; RUN: -mtriple arm64---- -o /dev/null 2>&1 \
 ; RUN: | FileCheck %s -check-prefix=ENABLE
 
 ; RUN: llc %s -debug-pass=Structure -verify-machineinstrs \
 ; RUN: -enable-machine-outliner=never -mtriple arm64---- -o /dev/null 2>&1 \
 ; RUN: | FileCheck %s -check-prefix=NEVER
 
 ; RUN: llc %s -debug-pass=Structure -verify-machineinstrs \
-; RUN: -mtriple arm64---- -o /dev/null 2>&1 \
+; RUN: --debug-only=machine-outliner -mtriple arm64---- -o /dev/null 2>&1 \
 ; RUN: | FileCheck %s -check-prefix=NOT-ADDED
 
 ; RUN: llc %s -O=0 -debug-pass=Structure -verify-machineinstrs \
 ; RUN: -mtriple arm64---- -o /dev/null 2>&1 \
 ; RUN: | FileCheck %s -check-prefix=OPTNONE
 
 ; Make sure that the outliner is added to the pass pipeline only when the
 ; appropriate flags/settings are set. Make sure it isn't added otherwise.
 ;
 ; Cases where it should be added:
 ;  * -enable-machine-outliner
 ;  * -enable-machine-outliner=always
+;  * -enable-machine-outliner is not passed (AArch64 supports
+;     target-default outlining)
 ;
 ; Cases where it should not be added:
 ;  * -O0 or equivalent
-;  * -enable-machine-outliner is not passed
 ;  * -enable-machine-outliner=never is passed
 
 ; ALWAYS: Machine Outliner
 ; ALWAYS: Machine Outliner: Running on all functions
 ; ENABLE: Machine Outliner
 ; ENABLE: Machine Outliner: Running on all functions
 ; NEVER-NOT: Machine Outliner
-; NOT-ADDED-NOT: Machine Outliner
+; NOT-ADDED: Machine Outliner
+; NOT-ADDED: Machine Outliner: Running on target-default functions
 ; OPTNONE-NOT: Machine Outliner
 
 define void @foo() {
   ret void;
 }
 

test/CodeGen/AArch64/max-jump-table.ll

 ; CHECK4-NEXT: %jump-table.0:  %bb.1 %bb.2 %bb.3 %bb.4{{$}}
 ; CHECK4-NOT: %jump-table.1
 ; CHECK8-NEXT: %jump-table.0:  %bb.1 %bb.2 %bb.3 %bb.4{{$}}
 ; CHECK8-NOT: %jump-table.1
 ; CHECKM1-NEXT: %jump-table.0:  %bb.1 %bb.2 %bb.3 %bb.4{{$}}
 ; CHECKM1-NOT: %jump-table.1
 ; CHECKM3-NEXT: %jump-table.0:  %bb.1 %bb.2 %bb.3 %bb.4 %bb.7 %bb.7 %bb.7 %bb.7 %bb.7 %bb.7 %bb.7 %bb.7 %bb.7 %bb.5 %bb.6{{$}}
 ; CHECKM3-NOT: %jump-table.1
+; CHECK-DAG: End machine code for function jt2.
 
 bb1: tail call void @ext(i32 1) br label %return
 bb2: tail call void @ext(i32 2) br label %return
 bb3: tail call void @ext(i32 3) br label %return
 bb4: tail call void @ext(i32 4) br label %return
 bb5: tail call void @ext(i32 5) br label %return
 bb6: tail call void @ext(i32 6) br label %return
 return: ret void
 }