[ARM] Account for implicit IT when calculating inline asm size

[peter.smith]

Summary

When deciding if it is safe to optimize a conditional branch to a CBZ or CBNZ the offsets of the BasicBlocks from the start of the function are estimated. For inline assembly the generic getInlineAsmLength() function is used to get a worst case estimate of the inline assembly by multiplying the number of instructions by the max instruction size of 4 bytes. This unfortunately doesn't take into account the generation of Thumb implicit IT instructions. In edge cases such as when all the instructions in the block are 4-bytes in size and there is an implicit IT then the size is underestimated. This can cause an out of range CBZ or CBNZ to be generated.

The patch takes a conservative approach and assumes that every instruction in the inline assembly block may have an implicit IT.

Fixes pr31805 https://bugs.llvm.org/show_bug.cgi?id=31805 which seems to be intermittently causing the Linux Kernel to fail to build on Arm.

Possible alternatives:

• Be less conservative and a small constant, say 4-bytes to account for any overspill from implicit IT creation, this could still fail but would probably work well in practice.
• The Arm MaxInstLength could be increased to 6, but this may not be right for Arm as there is some code in the ConstantIslands pass that assumes 4-byte alignment of Arm instructions.
• Reimplement getInlineAsmLength() or some similar function to estimate how many implicit IT instructions are needed. Doing this accurately may require a lot of the functionality of an assembly parser though.

[efriedma]

The Arm MaxInstLength could be increased to 6

This seems like a better solution.

but this may not be right for Arm as there is some code in the ConstantIslands pass that assumes 4-byte alignment of Arm instructions.

We can just round up the size of an inline asm to a multiple of 4 if the function is in ARM mode.

lib/Target/ARM/ARMComputeBlockSize.cpp

62 ↗

(On Diff #168125)

Doing the adjustment *here* is pretty clearly wrong; getInstSizeInBytes is supposed to return a conservative estimate of the instruction size. If it isn't conservative enough, we should fix it *there*, rather than try to hack around it afterwards.

[peter.smith]

Thanks for the comments. I've made an attempt at a diff where the MaxInstLength is increased to 6. This moves all the significant code to getInstSizeInBytes(). I've applied the change to all Targets (Elf, Macho, Windows ...) as it is possible to use implicit-IT on all of them.

[efriedma]

Code changes seem fine.

test/CodeGen/ARM/cbz-implicit-it-range.ll

2 ↗

(On Diff #168278)

The second RUN line here doesn't actually run anything?

[peter.smith]

Updated diff to make the second run: command actually run and test. My apologies for missing that.

[efriedma]

LGTM

llvm/trunk/lib/Target/ARM/ARMBaseInstrInfo.cpp

unsigned ARMBaseInstrInfo::getInstSizeInBytes(const MachineInstr &MI) const {
   const MachineFunction *MF = MBB.getParent();
   const MCAsmInfo *MAI = MF->getTarget().getMCAsmInfo();
 
   const MCInstrDesc &MCID = MI.getDesc();
   if (MCID.getSize())
     return MCID.getSize();
 
   // If this machine instr is an inline asm, measure it.
-  if (MI.getOpcode() == ARM::INLINEASM)
-    return getInlineAsmLength(MI.getOperand(0).getSymbolName(), *MAI);
+  if (MI.getOpcode() == ARM::INLINEASM) {
+    unsigned Size = getInlineAsmLength(MI.getOperand(0).getSymbolName(), *MAI);
+    if (!MF->getInfo<ARMFunctionInfo>()->isThumbFunction())
+      Size = alignTo(Size, 4);
+    return Size;
+  }
   unsigned Opc = MI.getOpcode();
   switch (Opc) {
   default:
     // pseudo-instruction sizes are zero.
     return 0;
   case TargetOpcode::BUNDLE:
     return getInstBundleLength(MI);
   case ARM::MOVi16_ga_pcrel:

llvm/trunk/lib/Target/ARM/MCTargetDesc/ARMMCAsmInfo.cpp

ARMMCAsmInfoDarwin::ARMMCAsmInfoDarwin(const Triple &TheTriple) {
   Data64bitsDirective = nullptr;
   CommentString = "@";
   Code16Directive = ".code\t16";
   Code32Directive = ".code\t32";
   UseDataRegionDirectives = true;
 
   SupportsDebugInformation = true;
 
+  // Conditional Thumb 4-byte instructions can have an implicit IT.
+  MaxInstLength = 6;
+
   // Exceptions handling
   ExceptionsType = (TheTriple.isOSDarwin() && !TheTriple.isWatchABI())
                        ? ExceptionHandling::SjLj
                        : ExceptionHandling::DwarfCFI;
 
   UseIntegratedAssembler = true;
 }
 
ARMELFMCAsmInfo::ARMELFMCAsmInfo(const Triple &TheTriple) {
 
   Data64bitsDirective = nullptr;
   CommentString = "@";
   Code16Directive = ".code\t16";
   Code32Directive = ".code\t32";
 
   SupportsDebugInformation = true;
 
+  // Conditional Thumb 4-byte instructions can have an implicit IT.
+  MaxInstLength = 6;
+
   // Exceptions handling
   switch (TheTriple.getOS()) {
   case Triple::NetBSD:
     ExceptionsType = ExceptionHandling::DwarfCFI;
     break;
   default:
     ExceptionsType = ExceptionHandling::ARM;
     break;

 void ARMCOFFMCAsmInfoMicrosoft::anchor() { }
 
 ARMCOFFMCAsmInfoMicrosoft::ARMCOFFMCAsmInfoMicrosoft() {
   AlignmentIsInBytes = false;
   ExceptionsType = ExceptionHandling::WinEH;
   PrivateGlobalPrefix = "$M";
   PrivateLabelPrefix = "$M";
   CommentString = ";";
+
+  // Conditional Thumb 4-byte instructions can have an implicit IT.
+  MaxInstLength = 6;
 }
 
 void ARMCOFFMCAsmInfoGNU::anchor() { }
 
 ARMCOFFMCAsmInfoGNU::ARMCOFFMCAsmInfoGNU() {
   AlignmentIsInBytes = false;
   HasSingleParameterDotFile = true;
 
   CommentString = "@";
   Code16Directive = ".code\t16";
   Code32Directive = ".code\t32";
   PrivateGlobalPrefix = ".L";
   PrivateLabelPrefix = ".L";
 
   SupportsDebugInformation = true;
   ExceptionsType = ExceptionHandling::DwarfCFI;
   UseParensForSymbolVariant = true;
 
   UseIntegratedAssembler = true;
   DwarfRegNumForCFI = false;
-}
 
+  // Conditional Thumb 4-byte instructions can have an implicit IT.
+  MaxInstLength = 6;
+}

llvm/trunk/test/CodeGen/ARM/cbz-implicit-it-range.ll

+;RUN: llc -O2 -mtriple=thumbv7a-linux-gnueabihf -arm-implicit-it=always %s -o - | FileCheck %s
+;RUN: llc -O2 -mtriple=thumbv7a-linux-gnueabihf -no-integrated-as %s -o - | FileCheck %s
+
+; Check that we do not produce a CBZ instruction to jump over the inline
+; assembly as the distance is too far if the implicit IT instructions are
+; added.
+
+define void @f0(i32 %p1, i32 %p2, i32 %p3) nounwind {
+entry:
+  %cmp = icmp eq i32 %p1, 0
+  br i1 %cmp, label %if.else, label %if.then
+
+if.then:
+  tail call void asm sideeffect "movseq r0, #0\0A", ""()
+  tail call void asm sideeffect "movseq r0, #0\0A", ""()
+  tail call void asm sideeffect "movseq r0, #0\0A", ""()
+  tail call void asm sideeffect "movseq r0, #0\0A", ""()
+  tail call void asm sideeffect "movseq r0, #0\0A", ""()
+  tail call void asm sideeffect "movseq r0, #0\0A", ""()
+  tail call void asm sideeffect "movseq r0, #0\0A", ""()
+  tail call void asm sideeffect "movseq r0, #0\0A", ""()
+  tail call void asm sideeffect "movseq r0, #0\0A", ""()
+  tail call void asm sideeffect "movseq r0, #0\0A", ""()
+  tail call void asm sideeffect "movseq r0, #0\0A", ""()
+  tail call void asm sideeffect "movseq r0, #0\0A", ""()
+  tail call void asm sideeffect "movseq r0, #0\0A", ""()
+  tail call void asm sideeffect "movseq r0, #0\0A", ""()
+  tail call void asm sideeffect "movseq r0, #0\0A", ""()
+  tail call void asm sideeffect "movseq r0, #0\0A", ""()
+  tail call void asm sideeffect "movseq r0, #0\0A", ""()
+  tail call void asm sideeffect "movseq r0, #0\0A", ""()
+  tail call void asm sideeffect "movseq r0, #0\0A", ""()
+  tail call void asm sideeffect "movseq r0, #0\0A", ""()
+  tail call void asm sideeffect "movseq r0, #0\0A", ""()
+  tail call void asm sideeffect "movseq r0, #0\0A", ""()
+  br label %if.end
+
+if.else:
+  tail call void asm sideeffect "nop\0A", ""()
+  br label %if.end
+
+if.end:
+  ret void
+}
+; CHECK-LABEL: f0:
+; CHECK: beq .LBB0_{{[0-9]+}}
+