Skip to content

Commit

Permalink
[AVR] Fix parsing & emitting relative jumps (llvm#106722)
Browse files Browse the repository at this point in the history
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+2` behavior; that is, `jmp 100` is `pc =
100`, but `rjmp 100` gets understood as `pc = pc + 100 + 2`.

This is not reflected in the AVR codegen:

https://github.com/llvm/llvm-project/blob/f95026dbf66e353128a3a3d7b55f3e52d5985535/llvm/lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp#L89

... which always emits relative jumps that are two bytes too far - or
rather it _would_ emit such jumps if not for this check:

https://github.com/llvm/llvm-project/blob/f95026dbf66e353128a3a3d7b55f3e52d5985535/llvm/lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp#L517

... 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](llvm@697a162)
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:
- when emitting relative jumps, offset them by `-2` (well, `-1`,
strictly speaking, because those instructions rely on right-shifted
offset),
- when parsing relative jumps, treat `.` as `+2` and read `rjmp .+1234`
as `rjmp (1234 + 2)`.

This approach seems to be sound and now we generate the same assembly as
avr-gcc, which can be confirmed with:

```cpp
// avr-gcc test.c -O3 && avr-objdump -d a.out

int main() {
    asm(
"      foo:\n\t"
"        rjmp  .+2\n\t"
"        rjmp  .-2\n\t"
"        rjmp  foo\n\t"
"        rjmp  .+8\n\t"
"        rjmp  end\n\t"
"        rjmp  .+0\n\t"
"      end:\n\t"
"        rjmp .-4\n\t"
"        rjmp .-6\n\t"
"      x:\n\t"
"        rjmp x\n\t"
"        .short 0xc00f\n\t"
);
}
```

avr-gcc is also how I got the opcodes for all new tests like `inst-brbc.s`, so we should be good.

(cherry picked from commit 86a60e7)
  • Loading branch information
Patryk27 authored and tru committed Sep 3, 2024
1 parent f3da9af commit 830b7eb
Show file tree
Hide file tree
Showing 26 changed files with 567 additions and 401 deletions.
15 changes: 11 additions & 4 deletions llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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))
Expand All @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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:
Expand Down Expand Up @@ -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) {
Expand Down
12 changes: 5 additions & 7 deletions llvm/lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -513,15 +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;
[[fallthrough]];
// Always resolve relocations for PC-relative branches
return false;
case AVR::fixup_call:
return true;
}
Expand Down
25 changes: 25 additions & 0 deletions llvm/test/CodeGen/AVR/jmp.ll
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
; RUN: llc -filetype=obj -mtriple=avr < %s | llvm-objdump -dr --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: 00000000: R_AVR_13_PCREL bar
; 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
23 changes: 12 additions & 11 deletions llvm/test/MC/AVR/inst-brbc.s
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,23 @@
; RUN: | llvm-objdump -d - | FileCheck --check-prefix=INST %s

foo:

brbc 3, .+8
brbc 0, .-16
.short 0xf759
.short 0xf752
.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
22 changes: 11 additions & 11 deletions llvm/test/MC/AVR/inst-brbs.s
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,22 @@
; RUN: | llvm-objdump -d - | FileCheck --check-prefix=INST %s

foo:

brbs 3, .+8
brbs 0, .-12
.short 0xf359
.short 0xf352
.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
28 changes: 28 additions & 0 deletions llvm/test/MC/AVR/inst-brcc.s
Original file line number Diff line number Diff line change
@@ -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
28 changes: 28 additions & 0 deletions llvm/test/MC/AVR/inst-brcs.s
Original file line number Diff line number Diff line change
@@ -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
28 changes: 28 additions & 0 deletions llvm/test/MC/AVR/inst-breq.s
Original file line number Diff line number Diff line change
@@ -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
24 changes: 24 additions & 0 deletions llvm/test/MC/AVR/inst-brge.s
Original file line number Diff line number Diff line change
@@ -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
24 changes: 24 additions & 0 deletions llvm/test/MC/AVR/inst-brhc.s
Original file line number Diff line number Diff line change
@@ -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
24 changes: 24 additions & 0 deletions llvm/test/MC/AVR/inst-brhs.s
Original file line number Diff line number Diff line change
@@ -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
24 changes: 24 additions & 0 deletions llvm/test/MC/AVR/inst-brid.s
Original file line number Diff line number Diff line change
@@ -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
24 changes: 24 additions & 0 deletions llvm/test/MC/AVR/inst-brie.s
Original file line number Diff line number Diff line change
@@ -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
Loading

0 comments on commit 830b7eb

Please sign in to comment.