[AArch64] Move machine bundle unpacking to PreEmit2 phase.

[fhahn]

Summary

This patch adjusts the placement of the bundle unpacking to just before
code emission. In particular, this means bundle unpacking happens AFTER
the machine outliner. With the previous position, the machine outliner
may outline parts of a bundle, which breaks them up.

This is an issue for BLR_RVMARKER handling, as illustrated by the
rvmarker-pseudo-expansion-and-outlining.mir test case. The machine
outliner should not break up the bundles created during pseudo
expansion.

This should fix PR49082.

[SjoerdMeijer]

This minor reshuffle looks very reasonable to me, but I was wondering about this:

With the previous position, the machine outliner may outline parts of a bundle, which breaks them up.

I am not that familiar with the instruction bundles, and the MIR langref description is a bit minimal, but was curious and wanted to ask if this is what the outliner should be doing, i.e. breaking up bundles?

llvm/test/CodeGen/AArch64/rvmarker-pseudo-expansion-and-outlining.mir

21

Nit: outline -> outlined, and > 80 columns?

79

Same

[fhahn]

In D96294#2557345, @SjoerdMeijer wrote:

This minor reshuffle looks very reasonable to me, but I was wondering about this:

With the previous position, the machine outliner may outline parts of a bundle, which breaks them up.

I am not that familiar with the instruction bundles, and the MIR langref description is a bit minimal, but was curious and wanted to ask if this is what the outliner should be doing, i.e. breaking up bundles?

Machine instruction bundles are sequences of instructions that should be kept together back-to-back. The problem is that the bundle expansion (which gets rid of the bundles) runs before the machine outliner, so the bundles are gone when the outliner runs and it can outline parts of the instructions in the bundle.

The only uses of bundles in the AArch64 backend is to keep SVE MOVPRFX instructions together with a 2nd instruction and the BLR_RVMARKER handling, which should expand to a call, followed by mov x29, x29. In both cases, the outliner should not outline only parts of the sequence.

I think in the MOVPRFX cases, it would be a mis-compile if the MOVPRRX instruction gets moved. And moving the marker instruction mov x29, x29 prevents runtime optimizations. So I think for both use cases, running bundle expansion late is the right thing to do.

I updated the test to more directly check for that behavior. Without the patch, mov x29, x29 (which gets added during BLR_RVMARKER expansion) gets outlined, which is not desirable.

[fhahn]

Drop fixme from test

[fhahn]

llvm/test/CodeGen/AArch64/rvmarker-pseudo-expansion-and-outlining.mir

21

Drastically reworked the test, so the comment is gone :)

[SjoerdMeijer]

Thanks, that's clear and LGTM.

llvm/lib/Target/AArch64/AArch64TargetMachine.cpp

public:
   bool addRegBankSelect() override;
   void addPreGlobalInstructionSelect() override;
   bool addGlobalInstructionSelect() override;
   bool addILPOpts() override;
   void addPreRegAlloc() override;
   void addPostRegAlloc() override;
   void addPreSched2() override;
   void addPreEmitPass() override;
+  void addPreEmitPass2() override;
 
   std::unique_ptr<CSEConfigBase> getCSEConfig() const override;
 };
 
 } // end anonymous namespace
 
 TargetTransformInfo
 AArch64TargetMachine::getTargetTransformInfo(const Function &F) {
void AArch64PassConfig::addPreEmitPass() {
   }
 
   if (TM->getOptLevel() != CodeGenOpt::None && EnableCompressJumpTables)
     addPass(createAArch64CompressJumpTablesPass());
 
   if (TM->getOptLevel() != CodeGenOpt::None && EnableCollectLOH &&
       TM->getTargetTriple().isOSBinFormatMachO())
     addPass(createAArch64CollectLOHPass());
+}
 
-  // SVE bundles move prefixes with destructive operations.
+void AArch64PassConfig::addPreEmitPass2() {
+  // SVE bundles move prefixes with destructive operations. BLR_RVMARKER pseudo
+  // instructions are lowered to bundles as well.
   addPass(createUnpackMachineBundles(nullptr));
 }
 
 yaml::MachineFunctionInfo *
 AArch64TargetMachine::createDefaultFuncInfoYAML() const {
   return new yaml::AArch64FunctionInfo();
 }
 

llvm/test/CodeGen/AArch64/O0-pipeline.ll

 ; CHECK-NEXT:       AArch64 Indirect Thunks
 ; CHECK-NEXT:       AArch64 sls hardening pass
 ; CHECK-NEXT:       Analyze Machine Code For Garbage Collection
 ; CHECK-NEXT:       Insert fentry calls
 ; CHECK-NEXT:       Insert XRay ops
 ; CHECK-NEXT:       Implement the 'patchable-function' attribute
 ; CHECK-NEXT:       AArch64 Branch Targets
 ; CHECK-NEXT:       Branch relaxation pass
-; CHECK-NEXT:       Unpack machine instruction bundles
 ; CHECK-NEXT:       Contiguously Lay Out Funclets
 ; CHECK-NEXT:       StackMap Liveness Analysis
 ; CHECK-NEXT:       Live DEBUG_VALUE analysis
+; CHECK-NEXT:       Unpack machine instruction bundles
 ; CHECK-NEXT:       Lazy Machine Block Frequency Analysis
 ; CHECK-NEXT:       Machine Optimization Remark Emitter
 ; CHECK-NEXT:       AArch64 Assembly Printer
 ; CHECK-NEXT:       Free MachineFunction
 
 define void @f() {
   ret void
 }

llvm/test/CodeGen/AArch64/O3-pipeline.ll

 ; CHECK-NEXT:       Branch Probability Basic Block Placement
 ; CHECK-NEXT:       Insert fentry calls
 ; CHECK-NEXT:       Insert XRay ops
 ; CHECK-NEXT:       Implement the 'patchable-function' attribute
 ; CHECK-NEXT:       AArch64 load / store optimization pass
 ; CHECK-NEXT:       AArch64 Branch Targets
 ; CHECK-NEXT:       Branch relaxation pass
 ; CHECK-NEXT:       AArch64 Compress Jump Tables
-; CHECK-NEXT:       Unpack machine instruction bundles
 ; CHECK-NEXT:       Contiguously Lay Out Funclets
 ; CHECK-NEXT:       StackMap Liveness Analysis
 ; CHECK-NEXT:       Live DEBUG_VALUE analysis
 ; CHECK-NEXT:     Machine Outliner
 ; CHECK-NEXT:     FunctionPass Manager
+; CHECK-NEXT:       Unpack machine instruction bundles
 ; 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
 ; CHECK-NEXT: Pass Arguments:  -assumption-cache-tracker -targetlibinfo -domtree -loops -scalar-evolution -stack-safety-local

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

 ; 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:      Freeing Pass 'Machine Outliner'
 ; HOTNESS-NEXT:  Executing Pass 'Function Pass Manager'
 ; HOTNESS-NEXT: Executing Pass 'Verify generated machine code'
 ; HOTNESS-NEXT: Freeing Pass 'Verify generated machine code'
+; HOTNESS-NEXT: Executing Pass 'Unpack machine instruction bundles'
+; HOTNESS-NEXT: Freeing Pass 'Unpack machine instruction bundles'
+; HOTNESS-NEXT: Executing Pass 'Verify generated machine code'
+; HOTNESS-NEXT: Freeing Pass 'Verify generated machine code'
 ; 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:      Freeing Pass 'Machine Outliner'
 ; NO_HOTNESS-NEXT:  Executing Pass 'Function Pass Manager'
 ; NO_HOTNESS-NEXT: Executing Pass 'Verify generated machine code'
 ; NO_HOTNESS-NEXT: Freeing Pass 'Verify generated machine code'
+; NO_HOTNESS-NEXT: Executing Pass 'Unpack machine instruction bundles'
+; NO_HOTNESS-NEXT: Freeing Pass 'Unpack machine instruction bundles'
+; NO_HOTNESS-NEXT: Executing Pass 'Verify generated machine code'
+; NO_HOTNESS-NEXT: Freeing Pass 'Verify generated machine code'
 ; 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

llvm/test/CodeGen/AArch64/rvmarker-pseudo-expansion-and-outlining.mir

 # RUN: llc -enable-machine-outliner -start-before=aarch64-expand-pseudo -mtriple=arm64-apple-ios -o - %s | FileCheck %s
 #
-# FIXME: The calls to _cb1 & _cb2 should be followed by the marker instruction
-#        'mov x29, x29'. The marker should not get outlined.
+#  The calls to _cb1 & _cb2 should be followed by the marker instruction
+#  'mov x29, x29'. The marker should not get outlined.
 #
 # CHECK-LABEL: _fn1:
 # CHECK:       bb.0:
 # CHECK:        bl _cb1
+# CHECK-NEXT:   mov x29, x29
 # CHECK-NEXT:   bl _OUTLINED_FUNCTION_0
 #
 # CHECK-LABEL: _fn2:
 # CHECK:       bb.0:
 # CHECK:        bl _cb2
+# CHECK-NEXT:   mov x29, x29
 # CHECK-NEXT:   bl _OUTLINED_FUNCTION_0
 #
 # CHECK-LABEL: _OUTLINED_FUNCTION_0:
 # CHECK:       bb.0:
-# CHECK-NEXT:    mov x29, x29
 # CHECK-NEXT:    orr w12, wzr, #0x3
 # CHECK-NEXT:    orr w12, wzr, #0x3
 # CHECK-NEXT:    orr w12, wzr, #0x3
 # CHECK-NEXT:    orr w12, wzr, #0x3
 # CHECK-NEXT:    ret
 
 --- |
   target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
   target triple = "arm64-apple-ios"
 
bb.0:
     $w12 = ORRWri $wzr, 1
     $w12 = ORRWri $wzr, 1
     $w12 = ORRWri $wzr, 1
     $w12 = ORRWri $wzr, 1
     $w12 = ORRWri $wzr, 1
     $lr = ORRXri $xzr, 1
 
     RET undef $lr
 ...