[AVR] Force relocations for jumps and calls

Author: Patryk27

llvm/lib/Target/AVR/AVRInstrInfo.cpp

@@ -557,17 +557,23 @@ void AVRInstrInfo::insertIndirectBranch(MachineBasicBlock &MBB,
                                         MachineBasicBlock &RestoreBB,
                                         const DebugLoc &DL, int64_t BrOffset,
                                         RegScavenger *RS) const {
-  // This method inserts a *direct* branch (JMP), despite its name.
-  // LLVM calls this method to fixup unconditional branches; it never calls
-  // insertBranch or some hypothetical "insertDirectBranch".
-  // See lib/CodeGen/RegisterRelaxation.cpp for details.
-  // We end up here when a jump is too long for a RJMP instruction.
+  // When an instruction's jump target is too far away¹, the branch relaxation
+  // pass might split an existing, large basic block into two pieces - if that
+  // happens, this function gets called to create a new jump between the blocks.
+  //
+  // Since this new jump might be too large for an `rjmp` anyway, let's
+  // conservatively emit `jmp` and back off to `rjmp` only if the target arch
+  // doesn't support `jmp`.
+  //
+  // TODO maybe we could use `BrOffset` to determine whether it's safe to emit
+  //      `rjmp`? (it takes one byte less to encode, so it'd be a small win)
+  //
+  // ¹ "too far" in the sense of "the instruction encoding doesn't allow for an
+  //   offset that large"
+
   if (STI.hasJMPCALL())
     BuildMI(&MBB, DL, get(AVR::JMPk)).addMBB(&NewDestBB);
   else
-    // The RJMP may jump to a far place beyond its legal range. We let the
-    // linker to report 'out of range' rather than crash, or silently emit
-    // incorrect assembly code.
     BuildMI(&MBB, DL, get(AVR::RJMPk)).addMBB(&NewDestBB);
 }
 

llvm/lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp

@@ -27,129 +27,32 @@
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/raw_ostream.h"
 
-// FIXME: we should be doing checks to make sure asm operands
-// are not out of bounds.
-
 namespace adjust {
 
 using namespace llvm;
 
-static void signed_width(unsigned Width, uint64_t Value,
-                         std::string Description, const MCFixup &Fixup,
-                         MCContext *Ctx = nullptr) {
-  if (!isIntN(Width, Value)) {
-    std::string Diagnostic = "out of range " + Description;
-
-    int64_t Min = minIntN(Width);
-    int64_t Max = maxIntN(Width);
-
-    Diagnostic += " (expected an integer in the range " + std::to_string(Min) +
-                  " to " + std::to_string(Max) + ")";
-
-    if (Ctx) {
-      Ctx->reportError(Fixup.getLoc(), Diagnostic);
-    } else {
-      llvm_unreachable(Diagnostic.c_str());
-    }
-  }
-}
-
-static void unsigned_width(unsigned Width, uint64_t Value,
-                           std::string Description, const MCFixup &Fixup,
-                           MCContext *Ctx = nullptr) {
+static void ensureUnsignedWidth(unsigned Width, uint64_t Value,
+                                std::string Description, const MCFixup &Fixup,
+                                MCContext *Ctx) {
   if (!isUIntN(Width, Value)) {
     std::string Diagnostic = "out of range " + Description;
 
     int64_t Max = maxUIntN(Width);
 
-    Diagnostic +=
-        " (expected an integer in the range 0 to " + std::to_string(Max) + ")";
+    Diagnostic += " (expected an unsigned integer in the range 0 to " +
+                  std::to_string(Max) + ", got " + std::to_string(Value) + ")";
 
-    if (Ctx) {
-      Ctx->reportError(Fixup.getLoc(), Diagnostic);
-    } else {
-      llvm_unreachable(Diagnostic.c_str());
-    }
+    Ctx->reportError(Fixup.getLoc(), Diagnostic);
   }
 }
 
-/// Adjusts the value of a branch target before fixup application.
-static void adjustBranch(unsigned Size, const MCFixup &Fixup, uint64_t &Value,
-                         MCContext *Ctx = nullptr) {
-  // We have one extra bit of precision because the value is rightshifted by
-  // one.
-  unsigned_width(Size + 1, Value, std::string("branch target"), Fixup, Ctx);
-
-  // Rightshifts the value by one.
-  AVR::fixups::adjustBranchTarget(Value);
-}
-
-/// Adjusts the value of a relative branch target before fixup application.
-static void adjustRelativeBranch(unsigned Size, const MCFixup &Fixup,
-                                 uint64_t &Value, MCContext *Ctx = nullptr) {
-  // Jumps are relative to the current instruction.
-  Value -= 2;
-
-  // We have one extra bit of precision because the value is rightshifted by
-  // one.
-  signed_width(Size + 1, Value, std::string("branch target"), Fixup, Ctx);
-
-  // Rightshifts the value by one.
-  AVR::fixups::adjustBranchTarget(Value);
-}
-
-/// 22-bit absolute fixup.
-///
-/// Resolves to:
-/// 1001 kkkk 010k kkkk kkkk kkkk 111k kkkk
-///
-/// Offset of 0 (so the result is left shifted by 3 bits before application).
-static void fixup_call(unsigned Size, const MCFixup &Fixup, uint64_t &Value,
-                       MCContext *Ctx = nullptr) {
-  adjustBranch(Size, Fixup, Value, Ctx);
-
-  auto top = Value & (0xf00000 << 6);   // the top four bits
-  auto middle = Value & (0x1ffff << 5); // the middle 13 bits
-  auto bottom = Value & 0x1f;           // end bottom 5 bits
-
-  Value = (top << 6) | (middle << 3) | (bottom << 0);
-}
-
-/// 7-bit PC-relative fixup.
-///
-/// Resolves to:
-/// 0000 00kk kkkk k000
-/// Offset of 0 (so the result is left shifted by 3 bits before application).
-static void fixup_7_pcrel(unsigned Size, const MCFixup &Fixup, uint64_t &Value,
-                          MCContext *Ctx = nullptr) {
-  adjustRelativeBranch(Size, Fixup, Value, Ctx);
-
-  // Because the value may be negative, we must mask out the sign bits
-  Value &= 0x7f;
-}
-
-/// 12-bit PC-relative fixup.
-/// Yes, the fixup is 12 bits even though the name says otherwise.
-///
-/// Resolves to:
-/// 0000 kkkk kkkk kkkk
-/// Offset of 0 (so the result isn't left-shifted before application).
-static void fixup_13_pcrel(unsigned Size, const MCFixup &Fixup, uint64_t &Value,
-                           MCContext *Ctx = nullptr) {
-  adjustRelativeBranch(Size, Fixup, Value, Ctx);
-
-  // Because the value may be negative, we must mask out the sign bits
-  Value &= 0xfff;
-}
-
 /// 6-bit fixup for the immediate operand of the STD/LDD family of
 /// instructions.
 ///
 /// Resolves to:
 /// 10q0 qq10 0000 1qqq
-static void fixup_6(const MCFixup &Fixup, uint64_t &Value,
-                    MCContext *Ctx = nullptr) {
-  unsigned_width(6, Value, std::string("immediate"), Fixup, Ctx);
+static void fixup_6(const MCFixup &Fixup, uint64_t &Value, MCContext *Ctx) {
+  ensureUnsignedWidth(6, Value, std::string("immediate"), Fixup, Ctx);
 
   Value = ((Value & 0x20) << 8) | ((Value & 0x18) << 7) | (Value & 0x07);
 }
@@ -160,8 +63,8 @@ static void fixup_6(const MCFixup &Fixup, uint64_t &Value,
 /// Resolves to:
 /// 0000 0000 kk00 kkkk
 static void fixup_6_adiw(const MCFixup &Fixup, uint64_t &Value,
-                         MCContext *Ctx = nullptr) {
-  unsigned_width(6, Value, std::string("immediate"), Fixup, Ctx);
+                         MCContext *Ctx) {
+  ensureUnsignedWidth(6, Value, std::string("immediate"), Fixup, Ctx);
 
   Value = ((Value & 0x30) << 2) | (Value & 0x0f);
 }
@@ -170,9 +73,8 @@ static void fixup_6_adiw(const MCFixup &Fixup, uint64_t &Value,
 ///
 /// Resolves to:
 /// 0000 0000 AAAA A000
-static void fixup_port5(const MCFixup &Fixup, uint64_t &Value,
-                        MCContext *Ctx = nullptr) {
-  unsigned_width(5, Value, std::string("port number"), Fixup, Ctx);
+static void fixup_port5(const MCFixup &Fixup, uint64_t &Value, MCContext *Ctx) {
+  ensureUnsignedWidth(5, Value, std::string("port number"), Fixup, Ctx);
 
   Value &= 0x1f;
 
@@ -183,9 +85,8 @@ static void fixup_port5(const MCFixup &Fixup, uint64_t &Value,
 ///
 /// Resolves to:
 /// 1011 0AAd dddd AAAA
-static void fixup_port6(const MCFixup &Fixup, uint64_t &Value,
-                        MCContext *Ctx = nullptr) {
-  unsigned_width(6, Value, std::string("port number"), Fixup, Ctx);
+static void fixup_port6(const MCFixup &Fixup, uint64_t &Value, MCContext *Ctx) {
+  ensureUnsignedWidth(6, Value, std::string("port number"), Fixup, Ctx);
 
   Value = ((Value & 0x30) << 5) | (Value & 0x0f);
 }
@@ -195,8 +96,8 @@ static void fixup_port6(const MCFixup &Fixup, uint64_t &Value,
 /// Resolves to:
 /// 1010 ikkk dddd kkkk
 static void fixup_lds_sts_16(const MCFixup &Fixup, uint64_t &Value,
-                             MCContext *Ctx = nullptr) {
-  unsigned_width(7, Value, std::string("immediate"), Fixup, Ctx);
+                             MCContext *Ctx) {
+  ensureUnsignedWidth(7, Value, std::string("immediate"), Fixup, Ctx);
   Value = ((Value & 0x70) << 8) | (Value & 0x0f);
 }
 
@@ -213,7 +114,7 @@ namespace ldi {
 /// 0000 KKKK 0000 KKKK
 /// Offset of 0 (so the result isn't left-shifted before application).
 static void fixup(unsigned Size, const MCFixup &Fixup, uint64_t &Value,
-                  MCContext *Ctx = nullptr) {
+                  MCContext *Ctx) {
   uint64_t upper = Value & 0xf0;
   uint64_t lower = Value & 0x0f;
 
@@ -223,25 +124,25 @@ static void fixup(unsigned Size, const MCFixup &Fixup, uint64_t &Value,
 static void neg(uint64_t &Value) { Value *= -1; }
 
 static void lo8(unsigned Size, const MCFixup &Fixup, uint64_t &Value,
-                MCContext *Ctx = nullptr) {
+                MCContext *Ctx) {
   Value &= 0xff;
   ldi::fixup(Size, Fixup, Value, Ctx);
 }
 
 static void hi8(unsigned Size, const MCFixup &Fixup, uint64_t &Value,
-                MCContext *Ctx = nullptr) {
+                MCContext *Ctx) {
   Value = (Value & 0xff00) >> 8;
   ldi::fixup(Size, Fixup, Value, Ctx);
 }
 
 static void hh8(unsigned Size, const MCFixup &Fixup, uint64_t &Value,
-                MCContext *Ctx = nullptr) {
+                MCContext *Ctx) {
   Value = (Value & 0xff0000) >> 16;
   ldi::fixup(Size, Fixup, Value, Ctx);
 }
 
 static void ms8(unsigned Size, const MCFixup &Fixup, uint64_t &Value,
-                MCContext *Ctx = nullptr) {
+                MCContext *Ctx) {
   Value = (Value & 0xff000000) >> 24;
   ldi::fixup(Size, Fixup, Value, Ctx);
 }
@@ -263,13 +164,9 @@ void AVRAsmBackend::adjustFixupValue(const MCFixup &Fixup,
   default:
     llvm_unreachable("unhandled fixup");
   case AVR::fixup_7_pcrel:
-    adjust::fixup_7_pcrel(Size, Fixup, Value, Ctx);
-    break;
   case AVR::fixup_13_pcrel:
-    adjust::fixup_13_pcrel(Size, Fixup, Value, Ctx);
-    break;
   case AVR::fixup_call:
-    adjust::fixup_call(Size, Fixup, Value, Ctx);
+    // Handled by linker, see `shouldForceRelocation()`
     break;
   case AVR::fixup_ldi:
     adjust::ldi::fixup(Size, Fixup, Value, Ctx);
@@ -330,13 +227,15 @@ void AVRAsmBackend::adjustFixupValue(const MCFixup &Fixup,
     adjust::ldi::ms8(Size, Fixup, Value, Ctx);
     break;
   case AVR::fixup_16:
-    adjust::unsigned_width(16, Value, std::string("port number"), Fixup, Ctx);
+    adjust::ensureUnsignedWidth(16, Value, std::string("port number"), Fixup,
+                                Ctx);
 
     Value &= 0xffff;
     break;
   case AVR::fixup_16_pm:
     Value >>= 1; // Flash addresses are always shifted.
-    adjust::unsigned_width(16, Value, std::string("port number"), Fixup, Ctx);
+    adjust::ensureUnsignedWidth(16, Value, std::string("port number"), Fixup,
+                                Ctx);
 
     Value &= 0xffff;
     break;
@@ -517,8 +416,6 @@ bool AVRAsmBackend::shouldForceRelocation(const MCAssembler &Asm,
     return Fixup.getKind() >= FirstLiteralRelocationKind;
   case AVR::fixup_7_pcrel:
   case AVR::fixup_13_pcrel:
-    // Always resolve relocations for PC-relative branches
-    return false;
   case AVR::fixup_call:
     return true;
   }

llvm/test/CodeGen/AVR/branch-relaxation-long.ll

@@ -1,24 +1,25 @@
-; RUN: llc < %s -march=avr -mattr=avr3 | FileCheck %s
-; RUN: llc < %s -march=avr -mattr=avr2 | FileCheck --check-prefix=AVR2 %s
+; RUN: llc < %s -march=avr -mcpu=avr25 -filetype=obj -o - | llvm-objdump --mcpu=avr25 -dr --no-show-raw-insn --no-leading-addr - | FileCheck --check-prefix=AVR25 %s
+; RUN: llc < %s -march=avr -mcpu=avr3 -filetype=obj -o - | llvm-objdump --mcpu=avr3 -dr --no-show-raw-insn --no-leading-addr - | FileCheck --check-prefix=AVR3 %s
 
-; CHECK-LABEL: relax_to_jmp:
-; CHECK: cpi     r{{[0-9]+}}, 0
-; CHECK: brne    [[BB1:.LBB[0-9]+_[0-9]+]]
-; CHECK: jmp     [[BB2:.LBB[0-9]+_[0-9]+]]
-; CHECK: [[BB1]]:
-; CHECK: nop
-; CHECK: [[BB2]]:
+; AVR25: <relax_to_jmp>:
+; AVR25-NEXT: andi r24, 0x1
+; AVR25-NEXT: cpi r24, 0x0
+; AVR25-NEXT: brne .+0
+; AVR25-NEXT: R_AVR_7_PCREL .text+0x8
+; AVR25-NEXT: rjmp .+0
+; AVR25-NEXT: R_AVR_13_PCREL .text+0x100c
+; AVR25: ldi r24, 0x3
+; AVR25-NEXT: ret
 
-;; A `RJMP` is generated instead of expected `JMP` for AVR2,
-;; and it is up to the linker to report 'out of range' or
-;; 'exceed flash maximum size'.
-; AVR2-LABEL: relax_to_jmp:
-; AVR2: cpi     r{{[0-9]+}}, 0
-; AVR2: brne    [[BB1:.LBB[0-9]+_[0-9]+]]
-; AVR2: rjmp    [[BB2:.LBB[0-9]+_[0-9]+]]
-; AVR2: [[BB1]]:
-; AVR2: nop
-; AVR2: [[BB2]]:
+; AVR3: <relax_to_jmp>:
+; AVR3-NEXT: andi r24, 0x1
+; AVR3-NEXT: cpi r24, 0x0
+; AVR3-NEXT: brne .+0
+; AVR3-NEXT: R_AVR_7_PCREL .text+0xa
+; AVR3-NEXT: jmp 0x0
+; AVR3-NEXT: R_AVR_CALL .text+0x100e
+; AVR3: ldi r24, 0x3
+; AVR3-NEXT: ret
 
 define i8 @relax_to_jmp(i1 %a) {
 entry-block:
@@ -2081,24 +2082,25 @@ finished:
   ret i8 3
 }
 
-; CHECK-LABEL: relax_to_jmp_backwards:
-; CHECK: [[BB1:.LBB[0-9]+_[0-9]+]]
-; CHECK: nop
-; CHECK: cpi     r{{[0-9]+}}, 0
-; CHECK: breq    [[BB2:.LBB[0-9]+_[0-9]+]]
-; CHECK: jmp     [[BB1]]
-; CHECK: [[BB2]]:
+; AVR25: <relax_to_jmp_backwards>:
+; AVR25-NEXT: andi r24, 0x1
+; AVR25: cpi r24, 0x0
+; AVR25-NEXT: breq .+0
+; AVR25-NEXT: R_AVR_7_PCREL .text+0x201c
+; AVR25-NEXT: rjmp .+0
+; AVR25-NEXT: R_AVR_13_PCREL .text+0x1012
+; AVR25: ldi r24, 0x3
+; AVR25-NEXT: ret
 
-;; A `RJMP` is generated instead of expected `JMP` for AVR2,
-;; and it is up to the linker to report 'out of range' or
-;; 'exceed flash maximum size'.
-; AVR2-LABEL: relax_to_jmp_backwards:
-; AVR2: [[BB1:.LBB[0-9]+_[0-9]+]]
-; AVR2: nop
-; AVR2: cpi     r{{[0-9]+}}, 0
-; AVR2: breq    [[BB2:.LBB[0-9]+_[0-9]+]]
-; AVR2: rjmp    [[BB1]]
-; AVR2: [[BB2]]:
+; AVR3: <relax_to_jmp_backwards>:
+; AVR3-NEXT: andi r24, 0x1
+; AVR3: cpi r24, 0x0
+; AVR3-NEXT: breq .+0
+; AVR3-NEXT: R_AVR_7_PCREL .text+0x2020
+; AVR3-NEXT: jmp 0x0
+; AVR3-NEXT: R_AVR_CALL .text+0x1014
+; AVR3: ldi r24, 0x3
+; AVR3-NEXT: ret
 
 define i8 @relax_to_jmp_backwards(i1 %a) {
 entry-block:

llvm/test/CodeGen/AVR/jmp.ll

@@ -15,10 +15,11 @@ bb2:
 
 declare i8 @bar(i8);
 
-; CHECK: rcall   .-2
+; CHECK: rcall   .+0
 ; CHECK-NEXT: 00000000: R_AVR_13_PCREL bar
 ; CHECK-NEXT: cpi     r24, 0x7b
-; CHECK-NEXT: brne    .+4
+; CHECK-NEXT: brne    .+0
+; CHECK-NEXT: 00000004: R_AVR_7_PCREL .text+0xa
 ; CHECK-NEXT: ldi     r24, 0x64
 ; CHECK-NEXT: ret
 ; CHECK-NEXT: ldi     r24, 0xc8