[AVR] Fix parsing & emitting relative jumps #102936
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-mc Author: Patryk Wychowaniec (Patryk27) ChangesAVR relative jumps are encoded relative to the end of the current instruction (i.e. (note that strictly speaking we have to offset just This wasn't a problem up until 6859685 (or rather 84428da), because the AVR backend used to encode all jumps (including those between basic blocks) using relocations, so the problematic code path simply wasn't triggered. Closes #102436. Patch is 83.55 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/102936.diff 108 Files Affected:
diff --git a/llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp b/llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp
index 383dfcc31117c1..c016b2dd91dc67 100644
--- a/llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp
+++ b/llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp
@@ -72,7 +72,7 @@ class AVRAsmParser : public MCTargetAsmParser {
int parseRegisterName();
int parseRegister(bool RestoreOnFailure = false);
bool tryParseRegisterOperand(OperandVector &Operands);
- bool tryParseExpression(OperandVector &Operands);
+ bool tryParseExpression(OperandVector &Operands, int64_t offset);
bool tryParseRelocExpression(OperandVector &Operands);
void eatComma();
@@ -418,7 +418,7 @@ bool AVRAsmParser::tryParseRegisterOperand(OperandVector &Operands) {
return false;
}
-bool AVRAsmParser::tryParseExpression(OperandVector &Operands) {
+bool AVRAsmParser::tryParseExpression(OperandVector &Operands, int64_t offset) {
SMLoc S = Parser.getTok().getLoc();
if (!tryParseRelocExpression(Operands))
@@ -437,6 +437,11 @@ bool AVRAsmParser::tryParseExpression(OperandVector &Operands) {
if (getParser().parseExpression(Expression))
return true;
+ if (offset) {
+ Expression = MCBinaryExpr::createAdd(
+ Expression, MCConstantExpr::create(offset, getContext()), getContext());
+ }
+
SMLoc E = SMLoc::getFromPointer(Parser.getTok().getLoc().getPointer() - 1);
Operands.push_back(AVROperand::CreateImm(Expression, S, E));
return false;
@@ -529,8 +534,9 @@ bool AVRAsmParser::parseOperand(OperandVector &Operands, bool maybeReg) {
[[fallthrough]];
case AsmToken::LParen:
case AsmToken::Integer:
+ return tryParseExpression(Operands, 0);
case AsmToken::Dot:
- return tryParseExpression(Operands);
+ return tryParseExpression(Operands, 2);
case AsmToken::Plus:
case AsmToken::Minus: {
// If the sign preceeds a number, parse the number,
@@ -540,7 +546,7 @@ bool AVRAsmParser::parseOperand(OperandVector &Operands, bool maybeReg) {
case AsmToken::BigNum:
case AsmToken::Identifier:
case AsmToken::Real:
- if (!tryParseExpression(Operands))
+ if (!tryParseExpression(Operands, 0))
return false;
break;
default:
@@ -643,6 +649,7 @@ bool AVRAsmParser::ParseInstruction(ParseInstructionInfo &Info,
// These specific operands should be treated as addresses/symbols/labels,
// other than registers.
bool maybeReg = true;
+
if (OperandNum == 1) {
std::array<StringRef, 8> Insts = {"lds", "adiw", "sbiw", "ldi"};
for (auto Inst : Insts) {
diff --git a/llvm/lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp b/llvm/lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp
index 0d29912bee2646..46bef75697fcb2 100644
--- a/llvm/lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp
+++ b/llvm/lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp
@@ -94,6 +94,9 @@ static void adjustRelativeBranch(unsigned Size, const MCFixup &Fixup,
// Rightshifts the value by one.
AVR::fixups::adjustBranchTarget(Value);
+
+ // Jumps are relative to the current instruction.
+ Value -= 1;
}
/// 22-bit absolute fixup.
@@ -513,14 +516,10 @@ bool AVRAsmBackend::shouldForceRelocation(const MCAssembler &Asm,
switch ((unsigned)Fixup.getKind()) {
default:
return Fixup.getKind() >= FirstLiteralRelocationKind;
- // Fixups which should always be recorded as relocations.
case AVR::fixup_7_pcrel:
case AVR::fixup_13_pcrel:
- // Do not force relocation for PC relative branch like 'rjmp .',
- // 'rcall . - off' and 'breq . + off'.
- if (const auto *SymA = Target.getSymA())
- if (SymA->getSymbol().getName().size() == 0)
- return false;
+ // Always resolve relocations for PC-relative branches
+ return false;
[[fallthrough]];
case AVR::fixup_call:
return true;
diff --git a/llvm/test/CodeGen/AVR/jmp.ll b/llvm/test/CodeGen/AVR/jmp.ll
new file mode 100644
index 00000000000000..30c7850b368442
--- /dev/null
+++ b/llvm/test/CodeGen/AVR/jmp.ll
@@ -0,0 +1,24 @@
+; RUN: llc -filetype=obj -mtriple=avr < %s | llvm-objdump -d --no-show-raw-insn - | FileCheck %s
+
+define i8 @foo(i8 %a) {
+bb0:
+ %0 = tail call i8 @bar(i8 %a)
+ %1 = icmp eq i8 %0, 123
+ br i1 %1, label %bb1, label %bb2
+
+bb1:
+ ret i8 100
+
+bb2:
+ ret i8 200
+}
+
+declare i8 @bar(i8);
+
+; CHECK: rcall .-2
+; CHECK-NEXT: cpi r24, 0x7b
+; CHECK-NEXT: brne .+4
+; CHECK-NEXT: ldi r24, 0x64
+; CHECK-NEXT: ret
+; CHECK-NEXT: ldi r24, 0xc8
+; CHECK-NEXT: ret
diff --git a/llvm/test/MC/AVR/inst-adc.s b/llvm/test/MC/AVR/inst-adc.s
index d1157bc7a9b3ba..33ce40849e021b 100644
--- a/llvm/test/MC/AVR/inst-adc.s
+++ b/llvm/test/MC/AVR/inst-adc.s
@@ -1,9 +1,7 @@
; RUN: llvm-mc -triple avr -show-encoding < %s | FileCheck %s
; RUN: llvm-mc -filetype=obj -triple avr < %s | llvm-objdump -d - | FileCheck --check-prefix=CHECK-INST %s
-
foo:
-
adc r0, r15
adc r15, r0
adc r16, r31
diff --git a/llvm/test/MC/AVR/inst-add.s b/llvm/test/MC/AVR/inst-add.s
index 49ad5de80c06b9..5120af12be7c41 100644
--- a/llvm/test/MC/AVR/inst-add.s
+++ b/llvm/test/MC/AVR/inst-add.s
@@ -1,9 +1,7 @@
; RUN: llvm-mc -triple avr -show-encoding < %s | FileCheck %s
; RUN: llvm-mc -filetype=obj -triple avr < %s | llvm-objdump -d - | FileCheck --check-prefix=CHECK-INST %s
-
foo:
-
add r0, r15
add r15, r0
add r16, r31
diff --git a/llvm/test/MC/AVR/inst-adiw.s b/llvm/test/MC/AVR/inst-adiw.s
index 7904965a51d68d..6b83027234c11c 100644
--- a/llvm/test/MC/AVR/inst-adiw.s
+++ b/llvm/test/MC/AVR/inst-adiw.s
@@ -1,9 +1,7 @@
; RUN: llvm-mc -triple avr -mattr=addsubiw -show-encoding < %s | FileCheck %s
; RUN: llvm-mc -filetype=obj -triple avr -mattr=addsubiw < %s | llvm-objdump --no-print-imm-hex -dr --mattr=addsubiw - | FileCheck --check-prefix=CHECK-INST %s
-
foo:
-
adiw r26, 12
adiw r26, 63
diff --git a/llvm/test/MC/AVR/inst-and.s b/llvm/test/MC/AVR/inst-and.s
index c4d90bfba37477..19d8a16862dd93 100644
--- a/llvm/test/MC/AVR/inst-and.s
+++ b/llvm/test/MC/AVR/inst-and.s
@@ -1,9 +1,7 @@
; RUN: llvm-mc -triple avr -show-encoding < %s | FileCheck %s
; RUN: llvm-mc -filetype=obj -triple avr < %s | llvm-objdump -d - | FileCheck --check-prefix=CHECK-INST %s
-
foo:
-
and r0, r15
and r15, r0
and r16, r31
diff --git a/llvm/test/MC/AVR/inst-andi.s b/llvm/test/MC/AVR/inst-andi.s
index 96a090173bd786..a68eb66921bc25 100644
--- a/llvm/test/MC/AVR/inst-andi.s
+++ b/llvm/test/MC/AVR/inst-andi.s
@@ -1,9 +1,7 @@
; RUN: llvm-mc -triple avr -show-encoding < %s | FileCheck %s
; RUN: llvm-mc -filetype=obj -triple avr < %s | llvm-objdump --no-print-imm-hex -d - | FileCheck --check-prefix=CHECK-INST %s
-
foo:
-
andi r16, 255
andi r29, 190
andi r22, 172
diff --git a/llvm/test/MC/AVR/inst-asr.s b/llvm/test/MC/AVR/inst-asr.s
index 1b59d027dc2bcc..265f8646d4976d 100644
--- a/llvm/test/MC/AVR/inst-asr.s
+++ b/llvm/test/MC/AVR/inst-asr.s
@@ -1,9 +1,7 @@
; RUN: llvm-mc -triple avr -show-encoding < %s | FileCheck %s
; RUN: llvm-mc -filetype=obj -triple avr < %s | llvm-objdump -d - | FileCheck -check-prefix=CHECK-INST %s
-
foo:
-
asr r31
asr r25
asr r5
diff --git a/llvm/test/MC/AVR/inst-bld.s b/llvm/test/MC/AVR/inst-bld.s
index 71352c5c0abd80..01a1b8b6973290 100644
--- a/llvm/test/MC/AVR/inst-bld.s
+++ b/llvm/test/MC/AVR/inst-bld.s
@@ -1,9 +1,7 @@
; RUN: llvm-mc -triple avr -show-encoding < %s | FileCheck %s
; RUN: llvm-mc -filetype=obj -triple avr < %s | llvm-objdump --no-print-imm-hex -d - | FileCheck --check-prefix=CHECK-INST %s
-
foo:
-
bld r3, 5
bld r1, 1
bld r0, 0
diff --git a/llvm/test/MC/AVR/inst-brbc.s b/llvm/test/MC/AVR/inst-brbc.s
index 4d7d684da4468a..3ef3664cf07bfc 100644
--- a/llvm/test/MC/AVR/inst-brbc.s
+++ b/llvm/test/MC/AVR/inst-brbc.s
@@ -3,7 +3,6 @@
; RUN: | llvm-objdump -d - | FileCheck --check-prefix=INST %s
foo:
-
brbc 3, .+8
brbc 0, .-16
.short 0xf759
@@ -11,14 +10,16 @@ foo:
.short 0xf74c
.short 0xf4c7
-; CHECK: brvc .Ltmp0+8 ; encoding: [0bAAAAA011,0b111101AA]
-; CHECK: ; fixup A - offset: 0, value: .Ltmp0+8, kind: fixup_7_pcrel
-; CHECK: brcc .Ltmp1-16 ; encoding: [0bAAAAA000,0b111101AA]
-; CHECK: ; fixup A - offset: 0, value: .Ltmp1-16, kind: fixup_7_pcrel
+; CHECK: brvc (.Ltmp0+8)+2 ; encoding: [0bAAAAA011,0b111101AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: (.Ltmp0+8)+2, kind: fixup_7_pcrel
+;
+; CHECK: brcc (.Ltmp1-16)+2 ; encoding: [0bAAAAA000,0b111101AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: (.Ltmp1-16)+2, kind: fixup_7_pcrel
-; INST: 23 f4 brvc .+8
-; INST: c0 f7 brsh .-16
-; INST: 59 f7 brne .-42
-; INST: 52 f7 brpl .-44
-; INST: 4c f7 brge .-46
-; INST: c7 f4 brid .+48
+; INST-LABEL: <foo>:
+; INST-NEXT: 23 f4 brvc .+8
+; INST-NEXT: c0 f7 brsh .-16
+; INST-NEXT: 59 f7 brne .-42
+; INST-NEXT: 52 f7 brpl .-44
+; INST-NEXT: 4c f7 brge .-46
+; INST-NEXT: c7 f4 brid .+48
diff --git a/llvm/test/MC/AVR/inst-brbs.s b/llvm/test/MC/AVR/inst-brbs.s
index 7987feeec654a1..58db423aae50fa 100644
--- a/llvm/test/MC/AVR/inst-brbs.s
+++ b/llvm/test/MC/AVR/inst-brbs.s
@@ -3,7 +3,6 @@
; RUN: | llvm-objdump -d - | FileCheck --check-prefix=INST %s
foo:
-
brbs 3, .+8
brbs 0, .-12
.short 0xf359
@@ -11,14 +10,16 @@ foo:
.short 0xf34c
.short 0xf077
-; CHECK: brvs .Ltmp0+8 ; encoding: [0bAAAAA011,0b111100AA]
-; CHECK: ; fixup A - offset: 0, value: .Ltmp0+8, kind: fixup_7_pcrel
-; CHECK: brcs .Ltmp1-12 ; encoding: [0bAAAAA000,0b111100AA]
-; CHECK: ; fixup A - offset: 0, value: .Ltmp1-12, kind: fixup_7_pcrel
+; CHECK: brvs (.Ltmp0+8)+2 ; encoding: [0bAAAAA011,0b111100AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: (.Ltmp0+8)+2, kind: fixup_7_pcrel
+;
+; CHECK: brcs (.Ltmp1-12)+2 ; encoding: [0bAAAAA000,0b111100AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: (.Ltmp1-12)+2, kind: fixup_7_pcrel
-; INST: 23 f0 brvs .+8
-; INST: d0 f3 brlo .-12
-; INST: 59 f3 breq .-42
-; INST: 52 f3 brmi .-44
-; INST: 4c f3 brlt .-46
-; INST: 77 f0 brie .+28
+; INST-LABEL: <foo>:
+; INST-NEXT: 23 f0 brvs .+8
+; INST-NEXT: d0 f3 brlo .-12
+; INST-NEXT: 59 f3 breq .-42
+; INST-NEXT: 52 f3 brmi .-44
+; INST-NEXT: 4c f3 brlt .-46
+; INST-NEXT: 77 f0 brie .+28
diff --git a/llvm/test/MC/AVR/inst-brcc.s b/llvm/test/MC/AVR/inst-brcc.s
new file mode 100644
index 00000000000000..d9218bc61e787f
--- /dev/null
+++ b/llvm/test/MC/AVR/inst-brcc.s
@@ -0,0 +1,28 @@
+; RUN: llvm-mc -triple avr -show-encoding < %s | FileCheck %s
+;
+; RUN: llvm-mc -filetype=obj -triple avr < %s \
+; RUN: | llvm-objdump -d - \
+; RUN: | FileCheck --check-prefix=INST %s
+
+foo:
+ brcc .+66
+ brcc .-22
+ brbc 0, .+66
+ brbc 0, bar
+
+bar:
+
+; CHECK: brcc (.Ltmp0+66)+2 ; encoding: [0bAAAAA000,0b111101AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: (.Ltmp0+66)+2, kind: fixup_7_pcrel
+; CHECK: brcc (.Ltmp1-22)+2 ; encoding: [0bAAAAA000,0b111101AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: (.Ltmp1-22)+2, kind: fixup_7_pcrel
+; CHECK: brcc (.Ltmp2+66)+2 ; encoding: [0bAAAAA000,0b111101AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: (.Ltmp2+66)+2, kind: fixup_7_pcrel
+; CHECK: brcc bar ; encoding: [0bAAAAA000,0b111101AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: bar, kind: fixup_7_pcrel
+
+; INST-LABEL: <foo>:
+; INST-NEXT: 08 f5 brsh .+66
+; INST-NEXT: a8 f7 brsh .-22
+; INST-NEXT: 08 f5 brsh .+66
+; INST-NEXT: 00 f4 brsh .+0
diff --git a/llvm/test/MC/AVR/inst-brcs.s b/llvm/test/MC/AVR/inst-brcs.s
new file mode 100644
index 00000000000000..0012cb31f61269
--- /dev/null
+++ b/llvm/test/MC/AVR/inst-brcs.s
@@ -0,0 +1,28 @@
+; RUN: llvm-mc -triple avr -show-encoding < %s | FileCheck %s
+;
+; RUN: llvm-mc -filetype=obj -triple avr < %s \
+; RUN: | llvm-objdump -d - \
+; RUN: | FileCheck --check-prefix=INST %s
+
+foo:
+ brcs .+8
+ brcs .+4
+ brbs 0, .+8
+ brbs 0, bar
+
+bar:
+
+; CHECK: brcs (.Ltmp0+8)+2 ; encoding: [0bAAAAA000,0b111100AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: (.Ltmp0+8)+2, kind: fixup_7_pcrel
+; CHECK: brcs (.Ltmp1+4)+2 ; encoding: [0bAAAAA000,0b111100AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: (.Ltmp1+4)+2, kind: fixup_7_pcrel
+; CHECK: brcs (.Ltmp2+8)+2 ; encoding: [0bAAAAA000,0b111100AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: (.Ltmp2+8)+2, kind: fixup_7_pcrel
+; CHECK: brcs bar ; encoding: [0bAAAAA000,0b111100AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: bar, kind: fixup_7_pcrel
+
+; INST-LABEL: <foo>:
+; INST-NEXT: 20 f0 brlo .+8
+; INST-NEXT: 10 f0 brlo .+4
+; INST-NEXT: 20 f0 brlo .+8
+; INST-NEXT: 00 f0 brlo .+0
diff --git a/llvm/test/MC/AVR/inst-break.s b/llvm/test/MC/AVR/inst-break.s
index a1bfde93c5a0d7..bb3abc651a1511 100644
--- a/llvm/test/MC/AVR/inst-break.s
+++ b/llvm/test/MC/AVR/inst-break.s
@@ -1,9 +1,7 @@
; RUN: llvm-mc -triple avr -mattr=break -show-encoding < %s | FileCheck %s
; RUN: llvm-mc -filetype=obj -triple avr -mattr=break < %s | llvm-objdump -d --mattr=break - | FileCheck --check-prefix=CHECK-INST %s
-
foo:
-
break
; CHECK: break ; encoding: [0x98,0x95]
diff --git a/llvm/test/MC/AVR/inst-breq.s b/llvm/test/MC/AVR/inst-breq.s
new file mode 100644
index 00000000000000..f82010f02ba617
--- /dev/null
+++ b/llvm/test/MC/AVR/inst-breq.s
@@ -0,0 +1,28 @@
+; RUN: llvm-mc -triple avr -show-encoding < %s | FileCheck %s
+;
+; RUN: llvm-mc -filetype=obj -triple avr < %s \
+; RUN: | llvm-objdump -d - \
+; RUN: | FileCheck --check-prefix=INST %s
+
+foo:
+ breq .-18
+ breq .-12
+ brbs 1, .-18
+ brbs 1, bar
+
+bar:
+
+; CHECK: breq (.Ltmp0-18)+2 ; encoding: [0bAAAAA001,0b111100AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: (.Ltmp0-18)+2, kind: fixup_7_pcrel
+; CHECK: breq (.Ltmp1-12)+2 ; encoding: [0bAAAAA001,0b111100AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: (.Ltmp1-12)+2, kind: fixup_7_pcrel
+; CHECK: brbs 1, (.Ltmp2-18)+2 ; encoding: [0bAAAAA001,0b111100AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: (.Ltmp2-18)+2, kind: fixup_7_pcrel
+; CHECK: brbs 1, bar ; encoding: [0bAAAAA001,0b111100AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: bar, kind: fixup_7_pcrel
+
+; INST-LABEL: <foo>:
+; INST-NEXT: b9 f3 breq .-18
+; INST-NEXT: d1 f3 breq .-12
+; INST-NEXT: b9 f3 breq .-18
+; INST-NEXT: 01 f0 breq .+0
diff --git a/llvm/test/MC/AVR/inst-brge.s b/llvm/test/MC/AVR/inst-brge.s
new file mode 100644
index 00000000000000..1121284a114689
--- /dev/null
+++ b/llvm/test/MC/AVR/inst-brge.s
@@ -0,0 +1,24 @@
+; RUN: llvm-mc -triple avr -show-encoding < %s | FileCheck %s
+;
+; RUN: llvm-mc -filetype=obj -triple avr < %s \
+; RUN: | llvm-objdump -d - \
+; RUN: | FileCheck --check-prefix=INST %s
+
+foo:
+ brge .+50
+ brge .+42
+ brge bar
+
+bar:
+
+; CHECK: brge (.Ltmp0+50)+2 ; encoding: [0bAAAAA100,0b111101AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: (.Ltmp0+50)+2, kind: fixup_7_pcrel
+; CHECK: brge (.Ltmp1+42)+2 ; encoding: [0bAAAAA100,0b111101AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: (.Ltmp1+42)+2, kind: fixup_7_pcrel
+; CHECK: brge bar ; encoding: [0bAAAAA100,0b111101AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: bar, kind: fixup_7_pcrel
+
+; INST-LABEL: <foo>:
+; INST-NEXT: cc f4 brge .+50
+; INST-NEXT: ac f4 brge .+42
+; INST-NEXT: 04 f4 brge .+0
diff --git a/llvm/test/MC/AVR/inst-brhc.s b/llvm/test/MC/AVR/inst-brhc.s
new file mode 100644
index 00000000000000..eb16ac2ef7a64e
--- /dev/null
+++ b/llvm/test/MC/AVR/inst-brhc.s
@@ -0,0 +1,24 @@
+; RUN: llvm-mc -triple avr -show-encoding < %s | FileCheck %s
+;
+; RUN: llvm-mc -filetype=obj -triple avr < %s \
+; RUN: | llvm-objdump -d - \
+; RUN: | FileCheck --check-prefix=INST %s
+
+foo:
+ brhc .+12
+ brhc .+14
+ brhc bar
+
+bar:
+
+; CHECK: brhc (.Ltmp0+12)+2 ; encoding: [0bAAAAA101,0b111101AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: (.Ltmp0+12)+2, kind: fixup_7_pcrel
+; CHECK: brhc (.Ltmp1+14)+2 ; encoding: [0bAAAAA101,0b111101AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: (.Ltmp1+14)+2, kind: fixup_7_pcrel
+; CHECK: brhc bar ; encoding: [0bAAAAA101,0b111101AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: bar, kind: fixup_7_pcrel
+
+; INST-LABEL: <foo>:
+; INST-NEXT: 35 f4 brhc .+12
+; INST-NEXT: 3d f4 brhc .+14
+; INST-NEXT: 05 f4 brhc .+0
diff --git a/llvm/test/MC/AVR/inst-brhs.s b/llvm/test/MC/AVR/inst-brhs.s
new file mode 100644
index 00000000000000..77c49596b3b0b8
--- /dev/null
+++ b/llvm/test/MC/AVR/inst-brhs.s
@@ -0,0 +1,24 @@
+; RUN: llvm-mc -triple avr -show-encoding < %s | FileCheck %s
+;
+; RUN: llvm-mc -filetype=obj -triple avr < %s \
+; RUN: | llvm-objdump -d - \
+; RUN: | FileCheck --check-prefix=INST %s
+
+foo:
+ brhs .-66
+ brhs .+14
+ brhs bar
+
+bar:
+
+; CHECK: brhs (.Ltmp0-66)+2 ; encoding: [0bAAAAA101,0b111100AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: (.Ltmp0-66)+2, kind: fixup_7_pcrel
+; CHECK: brhs (.Ltmp1+14)+2 ; encoding: [0bAAAAA101,0b111100AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: (.Ltmp1+14)+2, kind: fixup_7_pcrel
+; CHECK: brhs bar ; encoding: [0bAAAAA101,0b111100AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: bar, kind: fixup_7_pcrel
+
+; INST-LABEL: <foo>:
+; INST-NEXT: fd f2 brhs .-66
+; INST-NEXT: 3d f0 brhs .+14
+; INST-NEXT: 05 f0 brhs .+0
diff --git a/llvm/test/MC/AVR/inst-brid.s b/llvm/test/MC/AVR/inst-brid.s
new file mode 100644
index 00000000000000..70d0ea83c49b2a
--- /dev/null
+++ b/llvm/test/MC/AVR/inst-brid.s
@@ -0,0 +1,24 @@
+; RUN: llvm-mc -triple avr -show-encoding < %s | FileCheck %s
+;
+; RUN: llvm-mc -filetype=obj -triple avr < %s \
+; RUN: | llvm-objdump -d - \
+; RUN: | FileCheck --check-prefix=INST %s
+
+foo:
+ brid .+42
+ brid .+62
+ brid bar
+
+bar:
+
+; CHECK: brid (.Ltmp0+42)+2 ; encoding: [0bAAAAA111,0b111101AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: (.Ltmp0+42)+2, kind: fixup_7_pcrel
+; CHECK: brid (.Ltmp1+62)+2 ; encoding: [0bAAAAA111,0b111101AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: (.Ltmp1+62)+2, kind: fixup_7_pcrel
+; CHECK: brid bar ; encoding: [0bAAAAA111,0b111101AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: bar, kind: fixup_7_pcrel
+
+; INST-LABEL: <foo>:
+; INST-NEXT: af f4 brid .+42
+; INST-NEXT: ff f4 brid .+62
+; INST-NEXT: 07 f4 brid .+0
diff --git a/llvm/test/MC/AVR/inst-brie.s b/llvm/test/MC/AVR/inst-brie.s
new file mode 100644
index 00000000000000..717c686e2ed44e
--- /dev/null
+++ b/llvm/test/MC/AVR/inst-brie.s
@@ -0,0 +1,24 @@
+; RUN: llvm-mc -triple avr -show-encoding < %s | FileCheck %s
+;
+; RUN: llvm-mc -filetype=obj -triple avr < %s \
+; RUN: | llvm-objdump -d - \
+; RUN: | FileCheck --check-prefix=INST %s
+
+foo:
+ brie .+20
+ brie .+40
+ brie bar
+
+bar:
+
+; CHECK: brie (.Ltmp0+20)+2 ; encoding: [0bAAAAA111,0b111100AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: (.Ltmp0+20)+2, kind: fixup_7_pcrel
+; CHECK: brie (.Ltmp1+40)+2 ; encoding: [0bAAAAA111,0b111100AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: (.Ltmp1+40)+2, kind: fixup_7_pcrel
+; CHECK: brie bar ; encoding: [0bAAAAA111,0b111100AA]
+; CHECK-NEXT: ; fixup A - offset: 0, value: bar, kind: fixup_7_pcrel
+
+; INST-LABEL: <foo>:
+; INST-NEXT: 57 f0 brie .+20
+; INST-NEXT: a7 f0 brie .+40
+; INST-NEXT: 07 f0 brie .+0
diff --git a/llvm/test/MC/AVR/inst-brlo.s b/llvm/test/MC/AVR/inst-brlo.s
new file mode 100644
index 00000000000000..4b56d66ffdfe00
--- /dev/null
+++ b/llvm/test/MC/AVR/inst-brlo.s
@@ -0,0 +1,24 @@
+; RUN: llvm-mc -triple avr -show-encoding < %s | FileCheck %s
+;
+; RUN: llvm-mc -filetype=obj -triple avr < %s \
+; RUN: | llvm-objdump -d - \
+; RUN: | FileCheck --check-prefix=INST %s
+
+foo:
...
[truncated]
|
Patryk27
changed the title
|
cc @benshi001 🙂 |
|
Testing this change now. It does improve things, but it doesn't fix all issues. Still investigating... |
|
Nevermind, that was a stale build cache (should really fix that some day). I can confirm that with this fix, all my TinyGo AVR tests start working again on LLVM 19 (when cherry-picking the first commit on top of llvm19-rc3). I'll review the code more in-depth. |
|
This patch makes it so that relative branches are typically resolved at compile time, with no emitted relocations. void asdf(void);
void foobar(char x) {
while (x != 0) {
asdf();
x--;
}
}I get the following output for avr-gcc 5.4.0 (Debian), which includes relocations: And the following for avr-gcc 14.1.0 in Fedora 40, which also includes relocations: But Clang outputs the following, with no relocations: (In all cases, I compiled to an object file and disassembled using I don't know why this is, but my guess would be to support linker relaxation. In any case:
I think that would in fact be a reasonable solution: to let the linker deal with relative branches in all cases. |
|
This small patch also gets all my AVR tests to pass: --- a/llvm/lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp
+++ b/llvm/lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp
@@ -516,12 +516,6 @@ bool AVRAsmBackend::shouldForceRelocation(const MCAssembler &Asm,
// Fixups which should always be recorded as relocations.
case AVR::fixup_7_pcrel:
case AVR::fixup_13_pcrel:
- // Do not force relocation for PC relative branch like 'rjmp .',
- // 'rcall . - off' and 'breq . + off'.
- if (const auto *SymA = Target.getSymA())
- if (SymA->getSymbol().getName().size() == 0)
- return false;
- [[fallthrough]];
case AVR::fixup_call:
return true;
}EDIT: nevermind, it doesn't result in the same assembly when checking with your inline assembly example. So it's only mostly a fix, not entirely. |
aykevl
approved these changes
Aug 26, 2024
|
Also note that you may want to create two separate PRs, one for each commit, unless you want to squash then ("rebase and merge" has been disabled). |
|
Sorry for my delay, I will take a look soon. |
|
Should we force relocations for calls and pc-relative jumps, then? 👀 (cc @aykevl) I'm leaning towards resolving the relocations, because the current behavior seems to confuse people:
... but I don't have a strong opinion here - linker relaxation seems like a good argument (couldn't llvm just do it on its own, though?) and emiting relocations would certainly allow to remove some pieces of code, so there's pros and cons to both approaches. |
|
I'm mainly in favor of merging any reasonable fix in time for the LLVM 19 deadline (coming Tuesday) so that the AVR won't be totally broken in there. Whichever fix it is, I'm less concerned about. The PR as-is seems reasonable to me.
I'm not sure I agree with "it confuses people". Yes, it may confuse people, but that's just how relocations work. void foo(void);
void bar(void) {
foo();
}and I compile it to an ARM object file, I get the following: The same issue is present here: the @MaskRay also has a good explanation here: #104853 (comment) We could also do both and have all branches point to the destination, and insert relocations. Anyway, I hope this doesn't distract too much. I just hope we can get a fix in for LLVM 19 because having working code is better than having a totally broken AVR backend. We can think of the most proper fix afterwards. |
Just to clarify: the whole point of linker relaxation is that it can only be done at link time. A typical example would be to convert a ...but as long as we don't enable linker relaxation, there's nothing to worry about here. IIRC (but I'm not sure) there's a flag in the object file that tells the linker whether linker relaxation is supported, and I assume we don't set that flag. |
|
Makes sense, one more thing:
I'm not sure I follow - what does |
|
GitHub has several ways to merge a PR from the web UI:
Of these, only the 2nd is enabled, "squash and merge". That means that from the web UI, it's only possible to squash the two commits together into one and merge that. Which is probably not what you intended! I've noticed that (apparently) I can squash+merge a PR from the web UI. Didn't know that. |
|
Okie, on it! |
|
Okie, after that other PR is merged, I'll rebase this one and adjust it to contain only the test-refactoring commit. |
benshi001
reviewed
Sep 3, 2024
| ; CHECK: ; fixup A - offset: 0, value: .Ltmp0+8, kind: fixup_7_pcrel | ||
| ; CHECK: brcc .Ltmp1-16 ; encoding: [0bAAAAA000,0b111101AA] | ||
| ; CHECK: ; fixup A - offset: 0, value: .Ltmp1-16, kind: fixup_7_pcrel | ||
| ; CHECK: brvc (.Ltmp0+8)+2 ; encoding: [0bAAAAA011,0b111101AA] |
There was a problem hiding this comment.
Though the generated binary is correct, which makes TinyGo fully work. The disassembly of llvm-objdump is not in accordance with avr-objdump. avr-objdump will output 1a: 23 f4 brvc .+8, without an extra +2.
So can we also fix llvm-objdump ? BTW, is it possible to fix this issue by modification in adjustRelativeBranch. (Sorry I have not enough time to investigate my suggestion)
There was a problem hiding this comment.
I see, yes - we probably have to approach the problem differently, since treating . as +2 leaks into the objdump's output.
We've already got this commit merged, but I'll prepare a better fix in the coming days.
|
I'm cleaning up the pull requests - this change has already been merged, we're missing just the second commit, which I'll extract into a separate pull request now. I'm also noting both your suggestions and I'll try to prepare a fix where |
This was referenced Sep 3, 2024
This was referenced Sep 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ever since 6859685 (or, precisely, 84428da) relative jumps emitted by the AVR codegen are off by two bytes - this pull request fixes it.
Abstract
As compared to absolute jumps, relative jumps - such as rjmp, rcall or brsh - have an implied
pc+2behavior; that is,jmp 100ispc = 100, butrjmp 100gets understood aspc = pc + 100 + 2.This is not reflected in the AVR codegen:
llvm-project/llvm/lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp
Line 89 in f95026d
... which always emits relative jumps that are two bytes too far - or rather it would emit such jumps if not for this check:
llvm-project/llvm/lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp
Line 517 in f95026d
... which causes most of the relative jumps to be actually resolved late, by the linker, which applies the offsetting logic on its own, hiding the issue within LLVM.
Some time ago we've had a similar "jumps are off" problem that got solved by touching
shouldForceRelocation(), but I think that has worked only by accident. It's exploited the fact that absolute vs relative jumps in the parsed assembly can be distinguished through a "side channel" check relying on the existence of labels (i.e. absolute jumps happen to named labels, but relative jumps are anonymous, so to say). This was an alright idea back then, but it got broken by 6859685.I propose a different approach:
-2(well,-1, strictly speaking, because those instructions rely on right-shifted offset),.as+2and readrjmp .+1234asrjmp (1234 + 2).This approach seems to be sound and now we generate the same assembly as avr-gcc, which can be confirmed with:
avr-gcc is also how I got the opcodes for all new tests like
inst-brbc.s, so we should be good.Seizing the day, I've reformatted a couple of other tests (this change got extracted into its own commit).
Closes #102436.