Disable machine function splitting for functions with inline asm br

[adriantong1024]

Summary

Terminator intstructions may need to be recoded after machine function splitting.
The ones in the inline assembly can not.

[adriantong1024]

Update test case.

[efriedma]

The ones in the inline assembly cannot.

This should be fixable on top of D129288. But an incremental fix separate from that would be okay.

Not sure the extra flag carries its weight; it's already hard enough to keep track of all the various flags we put on MachineFunction , and you can already easily check IsInlineAsmBrIndirectTarget() while you're iterating over basic blocks.

[efriedma]

In D129677#3649806, @efriedma wrote:

The ones in the inline assembly cannot.

This should be fixable on top of D129288. But an incremental fix separate from that would be okay.

Err, actually, now I'm confused. What exactly do you mean by "recoded"? Do you mean there's a rule that imposes a maximum distance between an INLINEASM_BR and its indirect destinations? Is that rule written down anywhere?

[adriantong1024]

In D129677#3649812, @efriedma wrote:

In D129677#3649806, @efriedma wrote:

The ones in the inline assembly cannot.

This should be fixable on top of D129288. But an incremental fix separate from that would be okay.

Err, actually, now I'm confused. What exactly do you mean by "recoded"? Do you mean there's a rule that imposes a maximum distance between an INLINEASM_BR and its indirect destinations? Is that rule written down anywhere?

In the inline assembly, there may be branch that can only jump a limited distance, if we run MFS on the function, the resulting distance maybe to far to encode into the instruction.

[adriantong1024]

In D129677#3649806, @efriedma wrote:

The ones in the inline assembly cannot.

This should be fixable on top of D129288. But an incremental fix separate from that would be okay.

Not sure the extra flag carries its weight; it's already hard enough to keep track of all the various flags we put on MachineFunction , and you can already easily check IsInlineAsmBrIndirectTarget() while you're iterating over basic blocks.

I agree we should be careful to add more and more stuff into MachineFunction. Yes, using IsInlineAsmBrIndirectTarget() on all the basic blocks would achieve my purpose here as well. Thanks !

[efriedma]

In the inline assembly, there may be branch that can only jump a limited distance, if we run MFS on the function, the resulting distance maybe to far to encode into the instruction.

If there's a rule like this, we should explicitly state in LangRef which branches are/are not allowed.

[nickdesaulniers]

In D129677#3649869, @efriedma wrote:

In the inline assembly, there may be branch that can only jump a limited distance, if we run MFS on the function, the resulting distance maybe to far to encode into the instruction.

If there's a rule like this, we should explicitly state in LangRef which branches are/are not allowed.

It also seems like such a case is easy to fix; in the inline asm the user should just use the wider encoding.

Maybe would be a surprising failure though, since you'd think the labels are nearby.

[adriantong1024]

In D129677#3649869, @efriedma wrote:

In the inline assembly, there may be branch that can only jump a limited distance, if we run MFS on the function, the resulting distance maybe to far to encode into the instruction.

If there's a rule like this, we should explicitly state in LangRef which branches are/are not allowed.

In D129677#3649883, @nickdesaulniers wrote:

In D129677#3649869, @efriedma wrote:

In the inline assembly, there may be branch that can only jump a limited distance, if we run MFS on the function, the resulting distance maybe to far to encode into the instruction.

If there's a rule like this, we should explicitly state in LangRef which branches are/are not allowed.

It also seems like such a case is easy to fix; in the inline asm the user should just use the wider encoding.

Maybe would be a surprising failure though, since you'd think the labels are nearby.

@efriedma Thanks for the comment. Should it not be the compiler's job not to break valid inline assembly user put into their code ? or this is where we should have a specification to tell the user to not assume the block to be very close by.
@nickdesaulniers Thanks for the comment. I agree this does not happen much. but in case of machine function splitting, I do see it happening once in one of our workloads. The cold blocks are relocated too far.

[jyknight]

I don't think we should make this change.

Should it not be the compiler's job not to break valid inline assembly user put into their code?

I would argue that in this case, the asm is not "valid" -- it makes an unwarranted assumption as to the compiler's output. Unless there is a way to specify with an inline-asm constraint that the target address must be nearby, the assembly code cannot in general make such an assumption.

[efriedma]

It also seems like such a case is easy to fix; in the inline asm the user should just use the wider encoding.

There's always going to be some limit to the distance unless you use an indirect branch. Well, except on weird targets like x86, where a direct branch can reach everywhere. I guess on targets like AArch64, you could mark the branch destinations as functions, and let the linker could insert a stub. But people writing inline asm probably don't expect a branch to clobber x16/x17...

Once we start imposing any restriction on the distance, we need a patch like this: if the destination is in a different section, we can't promise anything about the distance.

I wouldn't expect this to be an issue on x86, though; even on 64-bit, people normally use the "small" code model, so a "jmp" should reach anywhere in the binary. So I'm not sure what the testcase is supposed to be testing.

[adriantong1024]

In D129677#3649972, @efriedma wrote:

It also seems like such a case is easy to fix; in the inline asm the user should just use the wider encoding.

There's always going to be some limit to the distance unless you use an indirect branch. Well, except on weird targets like x86, where a direct branch can reach everywhere. I guess on targets like AArch64, you could mark the branch destinations as functions, and let the linker could insert a stub. But people writing inline asm probably don't expect a branch to clobber x16/x17...

Once we start imposing any restriction on the distance, we need a patch like this: if the destination is in a different section, we can't promise anything about the distance.

I wouldn't expect this to be an issue on x86, though; even on 64-bit, people normally use the "small" code model, so a "jmp" should reach anywhere in the binary. So I'm not sure what the testcase is supposed to be testing.

Thanks for the discussion!

The problem this patch is trying to fix is discovered on AArch64 where the conditional branch b.ge is too short after MFS places hot and cold blocks apart.

I am convinced its a bad idea to impose any restriction on the distance. I think having a warning in MFS to help user prevent linker error would be something nice to have.

[nickdesaulniers]

In D129677#3653097, @adriantong1024 wrote:

The problem this patch is trying to fix is discovered on AArch64 where the conditional branch b.ge is too short after MFS places hot and cold blocks apart.

Isn't it straightforward to change b.ge to b.lt to the other branch destination, then b (no condition) to the label?

[adriantong1024]

In D129677#3653221, @nickdesaulniers wrote:

In D129677#3653097, @adriantong1024 wrote:

The problem this patch is trying to fix is discovered on AArch64 where the conditional branch b.ge is too short after MFS places hot and cold blocks apart.

Isn't it straightforward to change b.ge to b.lt to the other branch destination, then b (no condition) to the label?

We can do this. I am slightly worried about performance as this is in a performance critical part of the code.

[nickdesaulniers]

In D129677#3653271, @adriantong1024 wrote:

In D129677#3653221, @nickdesaulniers wrote:

In D129677#3653097, @adriantong1024 wrote:

The problem this patch is trying to fix is discovered on AArch64 where the conditional branch b.ge is too short after MFS places hot and cold blocks apart.

Isn't it straightforward to change b.ge to b.lt to the other branch destination, then b (no condition) to the label?

We can do this. I am slightly worried about performance as this is in a performance critical part of the code.

Generally, asm goto is modeled as the indirect branches being taken are the exceptional cases. So the indirect branch targets should be treated as if they were cold. If they're moved far away...good.

Otherwise if that's surprising for that code, it sounds like machine function splitting should be disabled for that function.

[adriantong1024]

In D129677#3653282, @nickdesaulniers wrote:

In D129677#3653271, @adriantong1024 wrote:

In D129677#3653221, @nickdesaulniers wrote:

In D129677#3653097, @adriantong1024 wrote:

The problem this patch is trying to fix is discovered on AArch64 where the conditional branch b.ge is too short after MFS places hot and cold blocks apart.

Isn't it straightforward to change b.ge to b.lt to the other branch destination, then b (no condition) to the label?

We can do this. I am slightly worried about performance as this is in a performance critical part of the code.

Generally, asm goto is modeled as the indirect branches being taken are the exceptional cases. So the indirect branch targets should be treated as if they were cold. If they're moved far away...good.

Otherwise if that's surprising for that code, it sounds like machine function splitting should be disabled for that function.

I think MFS is doing the right thing to move the target block away (as it is cold in the profile). Your suggestion of changing b.ge to b.lt is probably not as bad as I initially thought, because the b (no condition) is going to be rarely executed. However, it does make code size 4 bytes larger, which is probably not a big problem either.

[efriedma]

AArch64 "b" has a range of +-128MB. Which isn't enough for arbitrary programs. So in general, you need a sequence like the following (assuming small code model):

adrp x0, dest
add x0, x0, :lo12:dest
blr x0

That is, unless you're okay with the restriction that your binary is at most 128MB. Which might be reasonable for the Linux kernel, I guess. But again, something you'd want to document...

---

That said, I'm surprised machine function splitting on aarch64 works without any other changes; currently, branch relaxation isn't aware of section markings at all. Or do you have some other out-of-tree patches?

[adriantong1024]

In D129677#3653328, @efriedma wrote:

AArch64 "b" has a range of +-128MB. Which isn't enough for arbitrary programs. So in general, you need a sequence like the following (assuming small code model):

adrp x0, dest
add x0, x0, :lo12:dest
blr x0

That is, unless you're okay with the restriction that your binary is at most 128MB. Which might be reasonable for the Linux kernel, I guess. But again, something you'd want to document...

---

That said, I'm surprised machine function splitting on aarch64 works without any other changes; currently, branch relaxation isn't aware of section markings at all. Or do you have some other out-of-tree patches?

I think in case 128MB is not enough, the linker will help here. https://reviews.llvm.org/D39744

MFS does not work on AArch64, I am trying to make it work. I extended branch relaxation to handle cross-section branches. I plan to send out a RFC before sending up the out-of-tree patches I have.

Thanks !

[efriedma]

The primary issue with range extension thunks is that they clobber x16 (and in theory are allowed to clobber x17). So we'd need to ensure that all asm goto blocks clobber x16 and x17.

I can't think of any other issue with depending on range extension thunks, I guess. (See also https://github.com/ARM-software/abi-aa/blob/main/aaelf64/aaelf64.rst#call-and-jump-relocations .)

[nickdesaulniers]

In D129677#3653627, @efriedma wrote:

The primary issue with range extension thunks is that they clobber x16 (and in theory are allowed to clobber x17). So we'd need to ensure that all asm goto blocks clobber x16 and x17.

I can't think of any other issue with depending on range extension thunks, I guess. (See also https://github.com/ARM-software/abi-aa/blob/main/aaelf64/aaelf64.rst#call-and-jump-relocations .)

I guess we could error if the inline asm used x16 or x17 and we did do MFS? IIRC, we already error for 32b r7 being reserved (under some conditions, which I've forgotten). Or maybe avoid MFS if the inline asm had x16 or x17 in its clobber list?

[adriantong1024]

In D129677#3655977, @nickdesaulniers wrote:

In D129677#3653627, @efriedma wrote:

The primary issue with range extension thunks is that they clobber x16 (and in theory are allowed to clobber x17). So we'd need to ensure that all asm goto blocks clobber x16 and x17.

I can't think of any other issue with depending on range extension thunks, I guess. (See also https://github.com/ARM-software/abi-aa/blob/main/aaelf64/aaelf64.rst#call-and-jump-relocations .)

I guess we could error if the inline asm used x16 or x17 and we did do MFS? IIRC, we already error for 32b r7 being reserved (under some conditions, which I've forgotten). Or maybe avoid MFS if the inline asm had x16 or x17 in its clobber list?

Yes, I think this is a good option. Thanks !

llvm/include/llvm/CodeGen/MachineFunction.h

class LLVM_EXTERNAL_VISIBILITY MachineFunction {
   /// the attribute itself.
   /// This is used to limit optimizations which cannot reason
   /// about the control flow of such functions.
   bool ExposesReturnsTwice = false;
 
   /// True if the function includes any inline assembly.
   bool HasInlineAsm = false;
 
+  /// True if the function includes any inline assembly br.
+  bool HasInlineAsmBr = false;
+
   /// True if any WinCFI instruction have been emitted in this function.
   bool HasWinCFI = false;
 
   /// Current high-level properties of the IR of the function (e.g. is in SSA
   /// form or whether registers have been allocated)
   MachineFunctionProperties Properties;
 
   // Allocation management for pseudo source values.
void setExposesReturnsTwice(bool B) {
     ExposesReturnsTwice = B;
   }
 
   /// Returns true if the function contains any inline assembly.
   bool hasInlineAsm() const {
     return HasInlineAsm;
   }
 
+  /// Returns true if the function contains any inline assembly br.
+  bool hasInlineAsmBr() const { return HasInlineAsmBr; }
+
   /// Set a flag that indicates that the function contains inline assembly.
   void setHasInlineAsm(bool B) {
     HasInlineAsm = B;
   }
 
+  /// Set a flag that indicates that the function contains inline assembly br.
+  void setHasInlineAsmBr(bool B) { HasInlineAsmBr = B; }
+
   bool hasWinCFI() const {
     return HasWinCFI;
   }
   void setHasWinCFI(bool v) { HasWinCFI = v; }
 
   /// True if this function needs frame moves for debug or exceptions.
   bool needsFrameMoves() const;
 

llvm/include/llvm/CodeGen/MachineInstr.h

public:
   }
   bool isKill() const { return getOpcode() == TargetOpcode::KILL; }
   bool isImplicitDef() const { return getOpcode()==TargetOpcode::IMPLICIT_DEF; }
   bool isInlineAsm() const {
     return getOpcode() == TargetOpcode::INLINEASM ||
            getOpcode() == TargetOpcode::INLINEASM_BR;
   }
 
+  bool isInlineAsmBr() const {
+    return getOpcode() == TargetOpcode::INLINEASM_BR;
+  }
+
   /// FIXME: Seems like a layering violation that the AsmDialect, which is X86
   /// specific, be attached to a generic MachineInstr.
   bool isMSInlineAsm() const {
     return isInlineAsm() && getInlineAsmDialect() == InlineAsm::AD_Intel;
   }
 
   bool isStackAligningInlineAsm() const;
   InlineAsm::AsmDialect getInlineAsmDialect() const;

llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp

if (MF.size() != NumBlocks) {
     reportGISelFailure(MF, TPC, MORE, R);
     return false;
   }
 #endif
   // Determine if there are any calls in this machine function. Ported from
   // SelectionDAG.
   MachineFrameInfo &MFI = MF.getFrameInfo();
   for (const auto &MBB : MF) {
-    if (MFI.hasCalls() && MF.hasInlineAsm())
+    if (MFI.hasCalls() && MF.hasInlineAsm() && MF.hasInlineAsmBr())
       break;
 
     for (const auto &MI : MBB) {
       if ((MI.isCall() && !MI.isReturn()) || MI.isStackAligningInlineAsm())
         MFI.setHasCalls(true);
       if (MI.isInlineAsm())
         MF.setHasInlineAsm(true);
+      if (MI.isInlineAsmBr())
+        MF.setHasInlineAsmBr(true);
     }
   }
 
   // FIXME: FinalizeISel pass calls finalizeLowering, so it's called twice.
   auto &TLI = *MF.getSubtarget().getTargetLowering();
   TLI.finalizeLowering(MF);
 
   LLVM_DEBUG({

llvm/lib/CodeGen/MIRParser/MIRParser.cpp

static bool isSSA(const MachineFunction &MF) {
   return true;
 }
 
 void MIRParserImpl::computeFunctionProperties(MachineFunction &MF) {
   MachineFunctionProperties &Properties = MF.getProperties();
 
   bool HasPHI = false;
   bool HasInlineAsm = false;
+  bool HasInlineAsmBr = false;
   bool AllTiedOpsRewritten = true, HasTiedOps = false;
   for (const MachineBasicBlock &MBB : MF) {
     for (const MachineInstr &MI : MBB) {
       if (MI.isPHI())
         HasPHI = true;
       if (MI.isInlineAsm())
         HasInlineAsm = true;
+      if (MI.isInlineAsmBr())
+        HasInlineAsmBr = true;
       for (unsigned I = 0; I < MI.getNumOperands(); ++I) {
         const MachineOperand &MO = MI.getOperand(I);
         if (!MO.isReg() || !MO.getReg())
           continue;
         unsigned DefIdx;
         if (MO.isUse() && MI.isRegTiedToDefOperand(I, &DefIdx)) {
           HasTiedOps = true;
           if (MO.getReg() != MI.getOperand(DefIdx).getReg())
             AllTiedOpsRewritten = false;
         }
       }
     }
   }
   if (!HasPHI)
     Properties.set(MachineFunctionProperties::Property::NoPHIs);
   MF.setHasInlineAsm(HasInlineAsm);
+  MF.setHasInlineAsmBr(HasInlineAsmBr);
 
   if (HasTiedOps && AllTiedOpsRewritten)
     Properties.set(MachineFunctionProperties::Property::TiedOpsRewritten);
 
   if (isSSA(MF))
     Properties.set(MachineFunctionProperties::Property::IsSSA);
   else
     Properties.reset(MachineFunctionProperties::Property::IsSSA);

llvm/lib/CodeGen/MachineFunctionSplitter.cpp

 }
 
 bool MachineFunctionSplitter::runOnMachineFunction(MachineFunction &MF) {
   // TODO: We only target functions with profile data. Static information may
   // also be considered but we don't see performance improvements yet.
   if (!MF.getFunction().hasProfileData())
     return false;
 
+  // The terminator in the inline assembly may not be able to be rewritten.
+  if (MF.hasInlineAsmBr())
+    return false;
+
   // TODO: We don't split functions where a section attribute has been set
   // since the split part may not be placed in a contiguous region. It may also
   // be more beneficial to augment the linker to ensure contiguous layout of
   // split functions within the same section as specified by the attribute.
   if (MF.getFunction().hasSection() ||
       MF.getFunction().hasFnAttribute("implicit-section-name"))
     return false;
 

llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

bool SelectionDAGISel::runOnMachineFunction(MachineFunction &mf) {
   if (OptLevel != CodeGenOpt::None)
     AA = &getAnalysis<AAResultsWrapperPass>().getAAResults();
   else
     AA = nullptr;
 
   SDB->init(GFI, AA, LibInfo);
 
   MF->setHasInlineAsm(false);
+  MF->setHasInlineAsmBr(false);
 
   FuncInfo->SplitCSR = false;
 
   // We split CSR if the target supports it for the given function
   // and the function has only return exits.
   if (OptLevel != CodeGenOpt::None && TLI->supportSplitCSR(MF)) {
     FuncInfo->SplitCSR = true;
 
bool SelectionDAGISel::runOnMachineFunction(MachineFunction &mf) {
   // For debug-info, in instruction referencing mode, we need to perform some
   // post-isel maintenence.
   if (UseInstrRefDebugInfo)
     MF->finalizeDebugInstrRefs();
 
   // Determine if there are any calls in this machine function.
   MachineFrameInfo &MFI = MF->getFrameInfo();
   for (const auto &MBB : *MF) {
-    if (MFI.hasCalls() && MF->hasInlineAsm())
+    if (MFI.hasCalls() && MF->hasInlineAsm() && MF->hasInlineAsmBr())
       break;
 
     for (const auto &MI : MBB) {
       const MCInstrDesc &MCID = TII->get(MI.getOpcode());
       if ((MCID.isCall() && !MCID.isReturn()) ||
           MI.isStackAligningInlineAsm()) {
         MFI.setHasCalls(true);
       }
       if (MI.isInlineAsm()) {
         MF->setHasInlineAsm(true);
       }
+      if (MI.isInlineAsmBr()) {
+        MF->setHasInlineAsmBr(true);
+      }
     }
   }
 
   // Determine if there is a call to setjmp in the machine function.
   MF->setExposesReturnsTwice(Fn.callsFunctionThatReturnsTwice());
 
   // Determine if floating point is used for msvc
   computeUsesMSVCFloatingPoint(TM.getTargetTriple(), Fn, MF->getMMI());

llvm/test/CodeGen/X86/machine-function-splitter.ll

4:                                                ; preds = %1
   br label %6
 
 6:                                                ; preds = %4, %2
   %7 = tail call i32 @qux()
   ret void
 }
 
 
+; InlineAsmBr prevents machine function splitting on the function.
+define void @inlineasmbr(i1 zeroext %0) nounwind !prof !14 !section_prefix !15 {
+;; Check that no text.split is generated for this function.
+; MFS-DEFAULTS-LABEL: inlineasmbr
+; MFS-DEFAULTS-NOT:       .section        .text.split.inlineasmbr
+  callbr void asm sideeffect "# jump to $0", "i,~{dirflag},~{fpsr},~{flags}"(ptr blockaddress(@inlineasmbr, %5))
+          to label %7 [label %5]
+  br i1 %0, label %3, label %5, !prof !17
+
+3:                                                ; preds = %1
+  %4= call i32 @bar()
+  br label %7
+
+5:                                                ; preds = %1
+  %6 = call i32 @baz()
+  br label %7
+
+7:                                                ; preds = %5, %3
+  %8 = tail call i32 @qux()
+  ret void
+}
+
+
 declare i32 @bar()
 declare i32 @baz()
 declare i32 @bam()
 declare i32 @qux()
 declare void @_Z1fv()
 declare i32 @__gxx_personality_v0(...)
 
 @_ZTIi = external constant ptr