[all-commits] [llvm/llvm-project] 830b7e: [AVR] Fix parsing & emitting relative jumps (#106722)

Tue Sep 3 07:08:10 PDT 2024

  Branch: refs/heads/release/19.x
  Home:   https://github.com/llvm/llvm-project
  Commit: 830b7ebac09ebef91671f0863986aee1a1d60e5e
      https://github.com/llvm/llvm-project/commit/830b7ebac09ebef91671f0863986aee1a1d60e5e
  Author: Patryk Wychowaniec <pwychowaniec at pm.me>
  Date:   2024-09-03 (Tue, 03 Sep 2024)

  Changed paths:
    M llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp
    M llvm/lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp
    A llvm/test/CodeGen/AVR/jmp.ll
    M llvm/test/MC/AVR/inst-brbc.s
    M llvm/test/MC/AVR/inst-brbs.s
    A llvm/test/MC/AVR/inst-brcc.s
    A llvm/test/MC/AVR/inst-brcs.s
    A llvm/test/MC/AVR/inst-breq.s
    A llvm/test/MC/AVR/inst-brge.s
    A llvm/test/MC/AVR/inst-brhc.s
    A llvm/test/MC/AVR/inst-brhs.s
    A llvm/test/MC/AVR/inst-brid.s
    A llvm/test/MC/AVR/inst-brie.s
    A llvm/test/MC/AVR/inst-brlo.s
    A llvm/test/MC/AVR/inst-brlt.s
    A llvm/test/MC/AVR/inst-brmi.s
    A llvm/test/MC/AVR/inst-brne.s
    A llvm/test/MC/AVR/inst-brpl.s
    A llvm/test/MC/AVR/inst-brsh.s
    A llvm/test/MC/AVR/inst-brtc.s
    A llvm/test/MC/AVR/inst-brts.s
    A llvm/test/MC/AVR/inst-brvc.s
    A llvm/test/MC/AVR/inst-brvs.s
    R llvm/test/MC/AVR/inst-family-cond-branch.s
    M llvm/test/MC/AVR/inst-rcall.s
    M llvm/test/MC/AVR/inst-rjmp.s

  Log Message:
  -----------
  [AVR] Fix parsing & emitting relative jumps (#106722)

Ever since 6859685a87ad093d60c8bed60b116143c0a684c7 (or, precisely,
84428dafc0941e3a31303fa1b286835ab2b8e234) 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](https://github.com/llvm/llvm-project/commit/697a162fa63df328ec9ca334636c5e85390b2bf0)
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 6859685a87ad093d60c8bed60b116143c0a684c7.

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 86a60e7f1e8f361f84ccb6e656e848dd4fbaa713)

  Commit: a01d631a1c2c3902b383b6491f27b72d63f6257b
      https://github.com/llvm/llvm-project/commit/a01d631a1c2c3902b383b6491f27b72d63f6257b
  Author: Patryk Wychowaniec <pwychowaniec at pm.me>
  Date:   2024-09-03 (Tue, 03 Sep 2024)

  Changed paths:
    M lld/test/ELF/avr-reloc.s

  Log Message:
  -----------
  [AVR] Fix LLD test (#106739)

Since we don't generate relocations for those, it doesn't make sense to
assert them here; fallout of
https://github.com/llvm/llvm-project/pull/106722.

(cherry picked from commit a3816b5a573dbf57ba3082a919ca2de6b47257e9)

Compare: https://github.com/llvm/llvm-project/compare/f3da9af3fd26...a01d631a1c2c

To unsubscribe from these emails, change your notification settings at https://github.com/llvm/llvm-project/settings/notifications