[X86] Use bundle for CALL_RVMARKER expansion.

[fhahn]

Summary

This patch updates expandCALL_RVMARKER to wrap the call, marker and
objc runtime call in an instruction bundle. This ensures later passes,
like machine block placement, cannot break them up.

On AArch64, the instruction sequence is already wrapped in a bundle.
Keeping the whole instruction sequence together is highly desirable for
performance and outweighs potential other benefits from breaking the
sequence up.

[fhahn]

Updated to only use bundles during expansion for Darwin platforms. This keeps it in sync with TargetMachine (for now the bundle expansion pass is only scheduled on Darwin platforms).

[ahatanak]

In D115230#3180259, @fhahn wrote:

Updated to only use bundles during expansion for Darwin platforms. This keeps it in sync with TargetMachine (for now the bundle expansion pass is only scheduled on Darwin platforms).

Is it possible to pass a predicate functor to createUnpackMachineBundle? The functor would just check the presence of ObjC module flags or declarations of the ObjC ARC runtime functions in the module and UnpackMachineBundles::runOnMachineFunction would return early if they don't exist in the module.

I'm assuming the pass is enabled only on Darwin to avoid the overhead of running the pass when it's not needed.

[fhahn]

In D115230#3186380, @ahatanak wrote:

In D115230#3180259, @fhahn wrote:

Updated to only use bundles during expansion for Darwin platforms. This keeps it in sync with TargetMachine (for now the bundle expansion pass is only scheduled on Darwin platforms).

Is it possible to pass a predicate functor to createUnpackMachineBundle? The functor would just check the presence of ObjC module flags or declarations of the ObjC ARC runtime functions in the module and UnpackMachineBundles::runOnMachineFunction would return early if they don't exist in the module.

I'm assuming the pass is enabled only on Darwin to avoid the overhead of running the pass when it's not needed.

I updated the code to check for 2 runtime functions, but this requires including hard-coded runtime strings. Do you have any better ideas/suggestions?

[ahatanak]

Since the strings are used in other places (e.g., PreISelIntrinsicLowering.cpp) too, you could place them in a header in llvm/IR or llvm/Analysis. But I think it's okay to do so in a separate patch.

llvm/lib/Target/X86/X86ExpandPseudo.cpp

void X86ExpandPseudo::ExpandICallBranchFunnel(
   }
   JTMBB->erase(JTInst);
 }
 
 void X86ExpandPseudo::expandCALL_RVMARKER(MachineBasicBlock &MBB,
                                           MachineBasicBlock::iterator MBBI) {
   // Expand CALL_RVMARKER pseudo to call instruction, followed by the special
   //"movq %rax, %rdi" marker.
-  // TODO: Mark the sequence as bundle, to avoid passes moving other code
-  // in between.
   MachineInstr &MI = *MBBI;
 
   MachineInstr *OriginalCall;
   assert((MI.getOperand(1).isGlobal() || MI.getOperand(1).isReg()) &&
          "invalid operand for regular call");
   unsigned Opc = -1;
   if (MI.getOpcode() == X86::CALL64m_RVMARKER)
     Opc = X86::CALL64m;
auto *Marker = BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(X86::MOV64rr))
                      .addReg(X86::RAX)
                      .getInstr();
   if (MI.shouldUpdateCallSiteInfo())
     MBB.getParent()->moveCallSiteInfo(&MI, Marker);
 
   // Emit call to ObjC runtime.
   const uint32_t *RegMask =
       TRI->getCallPreservedMask(*MBB.getParent(), CallingConv::C);
+  MachineInstr *RtCall =
       BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(X86::CALL64pcrel32))
           .addGlobalAddress(MI.getOperand(0).getGlobal(), 0, 0)
           .addRegMask(RegMask)
           .addReg(X86::RAX,
                   RegState::Implicit |
                       (RAXImplicitDead ? (RegState::Dead | RegState::Define)
                                        : RegState::Define))
           .getInstr();
   MI.eraseFromParent();
+
+  auto &TM = MBB.getParent()->getTarget();
+  // On Darwin platforms, wrap the expanded sequence in a bundle to prevent
+  // later optimizations from breaking up the sequence.
+  if (TM.getTargetTriple().isOSDarwin())
+    finalizeBundle(MBB, OriginalCall->getIterator(),
+                   std::next(RtCall->getIterator()));
 }
 
 /// If \p MBBI is a pseudo instruction, this method expands
 /// it to the corresponding (sequence of) actual instruction(s).
 /// \returns true if \p MBBI has been expanded.
 bool X86ExpandPseudo::ExpandMI(MachineBasicBlock &MBB,
                                MachineBasicBlock::iterator MBBI) {
   MachineInstr &MI = *MBBI;

llvm/lib/Target/X86/X86TargetMachine.cpp

if (TT.isOSWindows()) {
     addPass(createCFGuardLongjmpPass());
     // Identify valid eh continuation targets for Windows EHCont Guard.
     addPass(createEHContGuardCatchretPass());
   }
   addPass(createX86LoadValueInjectionRetHardeningPass());
 
   // Insert pseudo probe annotation for callsite profiling
   addPass(createPseudoProbeInserter());
+
+  // On Darwin platforms, BLR_RVMARKER pseudo instructions are lowered to
+  // bundles.
+  if (TT.isOSDarwin())
+    addPass(createUnpackMachineBundles([](const MachineFunction &MF) {
+      // Only run bundle expansion if there are relevant ObjC runtime functions
+      // present in the module.
+      const Function &F = MF.getFunction();
+      const Module *M = F.getParent();
+      return M->getFunction("objc_retainAutoreleasedReturnValue") ||
+             M->getFunction("objc_unsafeClaimAutoreleasedReturnValue");
+    }));
 }
 
 bool X86PassConfig::addPostFastRegAllocRewrite() {
   addPass(createX86FastTileConfigPass());
   return true;
 }
 
 bool X86PassConfig::addPreRewrite() {
   addPass(createX86TileConfigPass());
   return true;
 }
 
 std::unique_ptr<CSEConfigBase> X86PassConfig::getCSEConfig() const {
   return getStandardCSEConfigForOpt(TM->getOptLevel());
 }

llvm/test/CodeGen/X86/call-rv-marker.ll

 ; CHECK-LABEL: _rv_marker_block_placement:
 ; CHECK:        pushq   %rax
 ; CHECK-NEXT:   .cfi_def_cfa_offset 16
 ; CHECK-NEXT:   testb   $1, %dil
 ; CHECK-NEXT:   je  LBB8_2
 
 ; CHECK-NEXT: ## %bb.1:
 ; CHECK-NEXT:   callq   _fn1
+; CHECK-NEXT:   movq    %rax, %rdi
+; CHECK-NEXT:   callq   _objc_retainAutoreleasedReturnValue
 ; CHECK-NEXT:   jmp LBB8_3
 
 ; CHECK-NEXT: LBB8_2:
 ; CHECK-NEXT:   callq   _fn2
-
-; CHECK-NEXT: LBB8_3:
 ; CHECK-NEXT:   movq    %rax, %rdi
 ; CHECK-NEXT:   callq   _objc_retainAutoreleasedReturnValue
+
+; CHECK-NEXT: LBB8_3:
 ; CHECK-NEXT:   xorl    %eax, %eax
 ; CHECK-NEXT:   popq    %rcx
 ; CHECK-NEXT:   retq
 ;
 entry:
   br i1 %c.0, label %then, label %else
 
 then:

llvm/test/CodeGen/X86/expand-call-rvmarker.mir

--- |
   }
 
 ...
 ---
 # CHECK-LABEL: name: test_objc_retainAutoreleaseReturnedValue
 # CHECK:      bb.0
 # CHECK-NEXT:   frame-setup PUSH64r undef $rax, implicit-def $rsp, implicit $rsp
 # CHECK-NEXT:   CFI_INSTRUCTION def_cfa_offset 16
-# CHECK-NEXT:   CALL64pcrel32 @fn, csr_64, implicit $rsp, implicit $ssp, implicit $rsp, implicit $ssp, implicit-def $rsp, implicit-def $ssp, implicit-def $rax
-# CHECK-NEXT:   $rdi = MOV64rr $rax
-# CHECK-NEXT:   CALL64pcrel32 @objc_retainAutoreleasedReturnValue, csr_64, implicit $rsp, implicit $ssp, implicit-def $rax
+# CHECK-NEXT:   BUNDLE
+# CHECK-NEXT:     CALL64pcrel32 @fn, csr_64, implicit $rsp, implicit $ssp, implicit $rsp, implicit $ssp, implicit-def $rsp, implicit-def $ssp, implicit-def $rax
+# CHECK-NEXT:     $rdi = MOV64rr internal $rax
+# CHECK-NEXT:     CALL64pcrel32 @objc_retainAutoreleasedReturnValue, csr_64, implicit internal $rsp, implicit internal $ssp, implicit-def $rax
+# CHECK-NEXT:   }
 # CHECK-NEXT:   $rcx = frame-destroy POP64r implicit-def $rsp, implicit $rsp
 # CHECK-NEXT:   RET64
 #
 
 name:            test_objc_retainAutoreleaseReturnedValue
 alignment:       16
 tracksRegLiveness: true
 frameInfo:
bb.0 (%ir-block.0):
     RET 0, $rax
 
 ...
 ---
 # CHECK-LABEL: name: test_objc_unsafeClaimAutoreleasedReturnValue
 # CHECK:      bb.0
 # CHECK-NEXT:   frame-setup PUSH64r undef $rax, implicit-def $rsp, implicit $rsp
 # CHECK-NEXT:   CFI_INSTRUCTION def_cfa_offset 16
-# CHECK-NEXT:   CALL64pcrel32 @fn, csr_64, implicit $rsp, implicit $ssp, implicit $rsp, implicit $ssp, implicit-def $rsp, implicit-def $ssp, implicit-def $rax
-# CHECK-NEXT:   $rdi = MOV64rr $rax
-# CHECK-NEXT:   CALL64pcrel32 @objc_unsafeClaimAutoreleasedReturnValue, csr_64, implicit $rsp, implicit $ssp, implicit-def $rax
+# CHECK-NEXT:   BUNDLE
+# CHECK-NEXT:     CALL64pcrel32 @fn, csr_64, implicit $rsp, implicit $ssp, implicit $rsp, implicit $ssp, implicit-def $rsp, implicit-def $ssp, implicit-def $rax
+# CHECK-NEXT:     $rdi = MOV64rr internal $rax
+# CHECK-NEXT:     CALL64pcrel32 @objc_unsafeClaimAutoreleasedReturnValue, csr_64, implicit internal $rsp, implicit internal $ssp, implicit-def $rax
+# CHECK-NEXT:   }
 # CHECK-NEXT:   $rcx = frame-destroy POP64r implicit-def $rsp, implicit $rsp
 # CHECK-NEXT:   RET64
 #
 name:            test_objc_unsafeClaimAutoreleasedReturnValue
 alignment:       16
 tracksRegLiveness: true
 frameInfo:
   stackSize:       8

 ...
 ---
 # CHECK-LABEL: name: test_objc_unsafeClaimAutoreleasedReturnValue_2_args
 # CHECK:      bb.0
 # CHECK:        frame-setup PUSH64r undef $rax, implicit-def $rsp, implicit $rsp
 # CHECK-NEXT:   $rax = MOV64rr $rdi
 # CHECK-NEXT:   $rdi = MOV64rr killed $rdx
 # CHECK-NEXT:   $rdx = MOV64rr killed $rax
-# CHECK-NEXT:   CALL64pcrel32 @fn, csr_64, implicit $rsp, implicit $ssp, implicit $rsp, implicit $ssp, implicit $rdi, implicit $rsi, implicit $rdx, implicit-def $rsp, implicit-def $ssp, implicit-def $rax
-# CHECK-NEXT:   $rdi = MOV64rr $rax
-# CHECK-NEXT:   CALL64pcrel32 @objc_retainAutoreleasedReturnValue, csr_64, implicit $rsp, implicit $ssp, implicit-def dead $rax
+# CHECK-NEXT:   BUNDLE
+# CHECK-NEXT:     CALL64pcrel32 @fn, csr_64, implicit $rsp, implicit $ssp, implicit $rsp, implicit $ssp, implicit $rdi, implicit $rsi, implicit $rdx, implicit-def $rsp, implicit-def $ssp, implicit-def $rax
+# CHECK-NEXT:     $rdi = MOV64rr internal $rax
+# CHECK-NEXT:     CALL64pcrel32 @objc_retainAutoreleasedReturnValue, csr_64, implicit internal $rsp, implicit internal $ssp, implicit-def dead $rax
+# CHECK-NEXT:  }
 # CHECK-NEXT:   $rax = frame-destroy POP64r implicit-def $rsp, implicit $rsp
 # CHECK-NEXT:   RET64
 #
 name:            test_objc_unsafeClaimAutoreleasedReturnValue_2_args
 alignment:       16
 tracksRegLiveness: true
 frameInfo:
   stackSize:       8
bb.0 (%ir-block.0):
     $rax = frame-destroy POP64r implicit-def $rsp, implicit $rsp
     RET 0, $rax
 
 ...
 # CHECK-LABEL: name: test_ret_void
 # CHECK:      bb.0
 # CHECK-NEXT:   frame-setup PUSH64r undef $rax, implicit-def $rsp, implicit $rsp
 # CHECK-NEXT:   CFI_INSTRUCTION def_cfa_offset 16
-# CHECK-NEXT:   CALL64pcrel32 @fn, csr_64, implicit $rsp, implicit $ssp, implicit $rsp, implicit $ssp, implicit-def $rsp, implicit-def $ssp, implicit-def $rax
-# CHECK-NEXT:   $rdi = MOV64rr $rax
-# CHECK-NEXT:   CALL64pcrel32 @objc_retainAutoreleasedReturnValue, csr_64, implicit $rsp, implicit $ssp, implicit-def dead $rax
+# CHECK-NEXT:   BUNDLE
+# CHECK-NEXT:     CALL64pcrel32 @fn, csr_64, implicit $rsp, implicit $ssp, implicit $rsp, implicit $ssp, implicit-def $rsp, implicit-def $ssp, implicit-def $rax
+# CHECK-NEXT:     $rdi = MOV64rr internal $rax
+# CHECK-NEXT:     CALL64pcrel32 @objc_retainAutoreleasedReturnValue, csr_64, implicit internal $rsp, implicit internal $ssp, implicit-def dead $rax
+# CHECK-NEXT:  }
 # CHECK-NEXT:   $rax = frame-destroy POP64r implicit-def $rsp, implicit $rsp
 # CHECK-NEXT:   RET64
 #
 name:            test_ret_void
 alignment:       16
 tracksRegLiveness: true
 frameInfo:
   stackSize:       8