[AArch64InstrInfo] Stop getInstSizeInBytes returning non-zero for meta instructions.

[paulwalker-arm]

Summary

AArch64InstrInfo::getInstSizeInBytes is incorrectly treating meta
instructions (e.g. CFI_INSTRUCTION) as normal instructions and
giving them a size of 4.

This results in branch relaxation calculating block sizes wrong.
Branch relaxation also considers alignment and thus a single
mistake can result in later blocks being incorrectly sized even
when they themselves do not contain meta instructions.

The net result is we might not relax a branch whose destination is
not within range.

[peter.smith]

Looks good to me.

Cross checking against ARM to see if the same problem exists there, it looks like a different approach is taken, where anything unrecognized is assumed to be 0 size. This would handle meta instructions, but could underestimate block size if it came across something it didn't recognise. So I think this approach works best for AArch64.

The net result is we might not relax a branch whose destination is not within range.

I'm guessing you mean, without this change we might not relax a branch whose destination is within range.

[paulwalker-arm]

Have I got the terminology the wrong way round? I assumed relaxing meant the act of converting a branch who destination is not within range into a one that is within range either by inversion or introducing a second branch with sufficient range. I just want to make sure my commit message makes sense.

[peter.smith]

In D66337#1632855, @paulwalker-arm wrote:

Have I got the terminology the wrong way round? I assumed relaxing meant the act of converting a branch who destination is not within range into a one that is within range either by inversion or introducing a second branch with sufficient range. I just want to make sure my commit message makes sense.

Sorry, I think it is me that has it wrong. I'm used to Linkers where relaxation can mean replace something like replace an expensive GlobalDynamic TLS sequence with a cheaper LocalDynamic one. In LLVM the branch relaxation pass starts with a small branch and replaces with a larger one if the smaller one is out of range so I think it is better to stick to your original one.

[leonardchan]

This test seems to be failing on our mac builders:

Testing: 0 .. 10..
FAIL: LLVM :: CodeGen/AArch64/branch-relax-block-size.mir (6095 of 33081)
******************** TEST 'LLVM :: CodeGen/AArch64/branch-relax-block-size.mir' FAILED ********************
Script:
--
: 'RUN: at line 2';   /b/s/w/ir/k/recipe_cleanup/clangOx7vi0/llvm_build_dir/bin/llc -mtriple=aarch64--linux-gnu -run-pass=branch-relaxation -debug-only=branch-relaxation /b/s/w/ir/k/llvm-project/llvm/test/CodeGen/AArch64/branch-relax-block-size.mir -o /dev/null 2>&1 | /b/s/w/ir/k/recipe_cleanup/clangOx7vi0/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/llvm/test/CodeGen/AArch64/branch-relax-block-size.mir
--
Exit Code: 1

Command Output (stderr):
--
/b/s/w/ir/k/llvm-project/llvm/test/CodeGen/AArch64/branch-relax-block-size.mir:8:15: error: CHECK-NEXT: expected string not found in input
# CHECK-NEXT: %bb.0 offset=00000000 size=0x18
              ^
<stdin>:3:1: note: scanning from here
bb.0 offset=00000000 size=0x18
^

--

********************
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
Testing Time: 119.28s
********************
Failing Tests (1):
    LLVM :: CodeGen/AArch64/branch-relax-block-size.mir

Seems to just be failing from a missing %.

[paulwalker-arm]

Thanks for the report, I've committed a version without the %s that will hopefully fix the issue.

[eugenis]

I've committed a better fix in r369138.

<stdin>:3:1: note: non-matching line after previous match is here

215234==AddressSanitizer: WARNING: unexpected format specifier in printf interceptor: %b (reported once per process)

[paulwalker-arm]

In D66337#1633466, @eugenis wrote:

I've committed a better fix in r369138.

Many Thanks. As I was a bit trigger happy with my revert/fix cycle I've restore the original commit.

lib/Target/AArch64/AArch64InstrInfo.cpp

unsigned AArch64InstrInfo::getInstSizeInBytes(const MachineInstr &MI) const {
   const MCAsmInfo *MAI = MF->getTarget().getMCAsmInfo();
 
   {
     auto Op = MI.getOpcode();
     if (Op == AArch64::INLINEASM || Op == AArch64::INLINEASM_BR)
       return getInlineAsmLength(MI.getOperand(0).getSymbolName(), *MAI);
   }
 
+  // Meta-instructions emit no code.
+  if (MI.isMetaInstruction())
+    return 0;
+
   // FIXME: We currently only handle pseudoinstructions that don't get expanded
   //        before the assembly printer.
   unsigned NumBytes = 0;
   const MCInstrDesc &Desc = MI.getDesc();
   switch (Desc.getOpcode()) {
   default:
     // Anything not explicitly designated otherwise is a normal 4-byte insn.
     NumBytes = 4;
     break;
-  case TargetOpcode::DBG_VALUE:
-  case TargetOpcode::EH_LABEL:
-  case TargetOpcode::IMPLICIT_DEF:
-  case TargetOpcode::KILL:
-    NumBytes = 0;
-    break;
   case TargetOpcode::STACKMAP:
     // The upper bound for a stackmap intrinsic is the full length of its shadow
     NumBytes = StackMapOpers(&MI).getNumPatchBytes();
     assert(NumBytes % 4 == 0 && "Invalid number of NOP bytes requested!");
     break;
   case TargetOpcode::PATCHPOINT:
     // The size of the patchpoint intrinsic is the number of bytes requested
     NumBytes = PatchPointOpers(&MI).getNumPatchBytes();

test/CodeGen/AArch64/branch-relax-block-size.mir

+# REQUIRES: asserts
+# RUN: llc -mtriple=aarch64--linux-gnu -run-pass=branch-relaxation -debug-only=branch-relaxation %s -o /dev/null 2>&1 | FileCheck %s
+
+# Ensure meta instructions (e.g. CFI_INSTRUCTION) don't contribute to the code
+# size of a basic block.
+
+# CHECK:  Basic blocks before relaxation
+# CHECK-NEXT: %bb.0 offset=00000000 size=0x18
+# CHECK-NEXT: %bb.1 offset=00000018 size=0x4
+# CHECK-NEXT: %bb.2 offset=0000001c size=0xc
+
+--- |
+  target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+  target triple = "aarch64--linux-gnu"
+
+  define i32 @test(i32* %a) #0 {
+  entry:
+    %call = tail call i32 @validate(i32* %a)
+    %tobool = icmp eq i32 %call, 0
+    br i1 %tobool, label %return, label %if.then
+
+  if.then:                                          ; preds = %entry
+    %0 = load i32, i32* %a, align 4
+    br label %return
+
+  return:                                           ; preds = %entry, %if.then
+    %retval.0 = phi i32 [ %0, %if.then ], [ 0, %entry ]
+    ret i32 %retval.0
+  }
+
+  declare i32 @validate(i32*)
+
+  attributes #0 = { nounwind uwtable "disable-tail-calls"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" }
+
+...
+---
+name:            test
+alignment:       2
+tracksRegLiveness: true
+liveins:
+  - { reg: '$x0' }
+frameInfo:
+  stackSize:       32
+  maxAlignment:    16
+  adjustsStack:    true
+  hasCalls:        true
+  maxCallFrameSize: 0
+stack:
+  - { id: 0, type: spill-slot, offset: -8, size: 8, alignment: 8, callee-saved-register: '$lr' }
+  - { id: 1, type: spill-slot, offset: -16, size: 8, alignment: 8, callee-saved-register: '$fp' }
+  - { id: 2, type: spill-slot, offset: -32, size: 8, alignment: 16, callee-saved-register: '$x19' }
+body:             |
+  bb.0.entry:
+    successors: %bb.2(0x30000000), %bb.1(0x50000000)
+    liveins: $x0, $x19, $lr
+
+    early-clobber $sp = frame-setup STRXpre killed $x19, $sp, -32 :: (store 8 into %stack.2)
+    frame-setup STPXi $fp, killed $lr, $sp, 2 :: (store 8 into %stack.1), (store 8 into %stack.0)
+    $fp = frame-setup ADDXri $sp, 16, 0
+    frame-setup CFI_INSTRUCTION def_cfa $w29, 16
+    frame-setup CFI_INSTRUCTION offset $w30, -8
+    frame-setup CFI_INSTRUCTION offset $w29, -16
+    frame-setup CFI_INSTRUCTION offset $w19, -32
+    $x19 = ORRXrs $xzr, $x0, 0
+    BL @validate, csr_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit killed $x0, implicit-def $sp, implicit-def $w0
+    CBZW renamable $w0, %bb.2
+
+  bb.1.if.then:
+    liveins: $x19
+
+    renamable $w0 = LDRWui killed renamable $x19, 0 :: (load 4 from %ir.a)
+
+  bb.2.return:
+    liveins: $w0
+
+    $fp, $lr = frame-destroy LDPXi $sp, 2 :: (load 8 from %stack.1), (load 8 from %stack.0)
+    early-clobber $sp, $x19 = frame-destroy LDRXpost $sp, 32 :: (load 8 from %stack.2)
+    RET undef $lr, implicit killed $w0
+
+...