[AVR] Fix a crash in AVRInstrInfo::insertIndirectBranch

[benshi001]

Fixes #67042

[aykevl]

To make this test easier to read (and less likely to not work anymore in the future due to unrelated changes), I think it would be better to write the test like branch-relaxation.ll and jmp-long.ll (lots of "nop" instructions between from the jump instruction to the branch target).

[benshi001]

To make this test easier to read (and less likely to not work anymore in the future due to unrelated changes), I think it would be better to write the test like branch-relaxation.ll and jmp-long.ll (lots of "nop" instructions between from the jump instruction to the branch target).

Done. I have modified branch-relaxation.ll and branch-relaxation-long.ll as you suggested.

[benshi001]

This patch just fixes the crash, but incorrect code is still generated, and the assembler or the linker will report "out of range".

A full solution would be introducing "code model", like riscv did
https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/RISCV/jumptable.ll#L4

when --model-mode=medium is specified, we use indirect branch ijmp instead of short direct rjmp, for AVR devices without long direct jump.

[jacquesguan]

LGTM

[aykevl]

A full solution would be introducing "code model", like riscv did https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/RISCV/jumptable.ll#L4

when --model-mode=medium is specified, we use indirect branch ijmp instead of short direct rjmp, for AVR devices without long direct jump.

Probably unnecessary. I think the only chips that are limited to rjmp are the chips that don't have enough flash to address more memory anyway. For example, the attiny85 has only 8kB flash so the entire flash area can be addressed using rjmp. From the ISA documentation:

Relative jump to an address within PC - 2K +1 and PC + 2K (words). For AVR microcontrollers with pogram memory not exceeding 4K words (8 KB), this instruction can address the entire memory from every address location. See also JMP.

As for the current fix:

This patch just fixes the crash, but incorrect code is still generated, and the assembler or the linker will report "out of range".

That seems like a reasonable solution to me. I hit this while compiling parts of the libc that will be too large anyway for these small chips, but it's nice to be able to compile those parts anyway. If those large functions are linked, they will result in a linker error anyway because they don't fit the flash space.

[aykevl]

This seems like a reasonable solution. The only thing I'm worried about is what the linker will actually do. Will it report an out of range error? Do we have tests for this?

[benshi001]

This seems like a reasonable solution. The only thing I'm worried about is what the linker will actually do. Will it report an out of range error? Do we have tests for this?

With the attached b.s, an linker error is reported as

benshi@BENNSHI-MB0 /tmp % /Applications/Arduino.app/Contents/Java/hardware/tools/avr/bin/avr-gcc b.s -O3 -Wall -mmcu=attiny85
/Applications/Arduino.app/Contents/Java/hardware/tools/avr/bin/../lib/gcc/avr/7.3.0/../../../../avr/bin/ld: a.out section `.text' will not fit in region `text'
/Applications/Arduino.app/Contents/Java/hardware/tools/avr/bin/../lib/gcc/avr/7.3.0/../../../../avr/bin/ld: region `text' overflowed by 614 bytes
collect2: error: ld returned 1 exit status

but it is not easy to add such a test into my patch.

[benshi001]

The b.s is attached here.
b.s.zip

[aykevl]

With the attached b.s, an linker error is reported as

And what about the LLVM linker (lld)? Does it report a similar error?
(I would assume so, because LLD normally generates such "out of space" errors).

but it is not easy to add such a test into my patch.

No I don't think it should be part of the test, just something to verify before committing. If we add a test it would be a separate test to LLD that checks whether an error is generated as we would expect.

In any case, I think this patch is ready to go from my POV, but perhaps others will like to take a look as well.

[aykevl]

Checked avr-gcc, and it behaves the same as this patch: it emits an rjmp if jmp isn't available.
Source: https://godbolt.org/z/dao8sq9hd

[benshi001]

With the attached b.s, an linker error is reported as

And what about the LLVM linker (lld)? Does it report a similar error? (I would assume so, because LLD normally generates such "out of space" errors).

but it is not easy to add such a test into my patch.

No I don't think it should be part of the test, just something to verify before committing. If we add a test it would be a separate test to LLD that checks whether an error is generated as we would expect.

In any case, I think this patch is ready to go from my POV, but perhaps others will like to take a look as well.

LLD checks range for conditional branch: https://github.com/llvm/llvm-project/blob/main/lld/ELF/Arch/AVR.cpp#L233,

but does not for RJMP: https://github.com/llvm/llvm-project/blob/main/lld/ELF/Arch/AVR.cpp#L240C8-L240C22

I will fix that in another patch.