Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Relocation truncated to fit" from branches in long functions #44

Closed
gergoerdi opened this issue May 1, 2017 · 25 comments
Closed

"Relocation truncated to fit" from branches in long functions #44

gergoerdi opened this issue May 1, 2017 · 25 comments
Labels
A-llvm Affects the LLVM AVR backend has-llvm-commit This issue should be fixed in upstream LLVM has-reduced-testcase A small LLVM IR file exists that demonstrates the problem

Comments

@gergoerdi
Copy link
Collaborator

As mentioned in #36 (comment), I got around #36 by inlining lots of functions. However, the resulting code cannot be linked because, I quote avr-gcc:

target/avr-atmega328p/release/deps/chip8_avr-41c427b8d446a439.o: In function `LBB5_18':
chip8_avr.cgu-0.rs:(.text.main+0x432): relocation truncated to fit: R_AVR_7_PCREL against `no symbol'
target/avr-atmega328p/release/deps/chip8_avr-41c427b8d446a439.o: In function `LBB5_23':
chip8_avr.cgu-0.rs:(.text.main+0x45c): relocation truncated to fit: R_AVR_7_PCREL against `no symbol'
target/avr-atmega328p/release/deps/chip8_avr-41c427b8d446a439.o: In function `LBB5_31':
chip8_avr.cgu-0.rs:(.text.main+0x4ae): relocation truncated to fit: R_AVR_7_PCREL against `no symbol'
target/avr-atmega328p/release/deps/chip8_avr-41c427b8d446a439.o: In function `LBB5_34':
chip8_avr.cgu-0.rs:(.text.main+0x4d2): relocation truncated to fit: R_AVR_7_PCREL against `no symbol'
target/avr-atmega328p/release/deps/chip8_avr-41c427b8d446a439.o: In function `LBB5_146':
chip8_avr.cgu-0.rs:(.text.main+0x58a): relocation truncated to fit: R_AVR_7_PCREL against `no symbol'
chip8_avr.cgu-0.rs:(.text.main+0x58e): relocation truncated to fit: R_AVR_7_PCREL against `no symbol'
chip8_avr.cgu-0.rs:(.text.main+0x592): relocation truncated to fit: R_AVR_7_PCREL against `no symbol'
target/avr-atmega328p/release/deps/chip8_avr-41c427b8d446a439.o: In function `LBB5_153':
chip8_avr.cgu-0.rs:(.text.main+0x59a): relocation truncated to fit: R_AVR_7_PCREL against `no symbol'
chip8_avr.cgu-0.rs:(.text.main+0x59e): relocation truncated to fit: R_AVR_7_PCREL against `no symbol'
chip8_avr.cgu-0.rs:(.text.main+0x5a6): relocation truncated to fit: R_AVR_7_PCREL against `no symbol'
chip8_avr.cgu-0.rs:(.text.main+0x5aa): additional relocation overflows omitted from the output
collect2: error: ld returned 1 exit status

However, looking at those places, all those branches that the linker tries to rewrite to a long address, they look quite superfluous to me. For example, the first one at 0x432:

     432:       01 f4           brne    .+0             ; 0x434 <LBB5_18+0x8>

Isn't that a conditional branch to exactly the next instruction? A roundabout, two-byte NOP?

I looked at all the locations mentioned in the linker error messages, and they are all of this form (some brne, some brcs, some brge, but all jump to exactly the next instruction).

@gergoerdi
Copy link
Collaborator Author

Here's a full example IR file:

; ModuleID = 'chip8_engine.cgu-0.rs'
source_filename = "chip8_engine.cgu-0.rs"
target datalayout = "e-p:16:16:16-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-n8"
target triple = "avr-atmel-none"

%"libcore_mini::option::Option<opcodes::Op>" = type { i8, [0 x i8], [5 x i8] }
%"machine::Machine" = type { i16, [0 x i8], i16, [0 x i8], [16 x i8], [0 x i8] }

; Function Attrs: noinline nounwind uwtable
define void @_ZN12chip8_engine7opcodes6decode17haab3c6c935229a6aE(%"libcore_mini::option::Option<opcodes::Op>"* noalias nocapture sret dereferenceable(6), i8) unnamed_addr #0 {
start:
  switch i8 %1, label %bb7 [
    i8 1, label %bb1
    i8 2, label %bb2
    i8 3, label %bb3
    i8 4, label %bb4
    i8 5, label %bb5
    i8 6, label %bb6
  ]

bb1:                                              ; preds = %start
  %2 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 0
  store i8 1, i8* %2, align 1
  %3 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 2, i16 0
  store i8 0, i8* %3, align 1
  %4 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 2, i16 1
  store i8 0, i8* %4, align 1
  br label %bb8

bb2:                                              ; preds = %start
  %5 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 0
  store i8 1, i8* %5, align 1
  %6 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 2, i16 0
  store i8 1, i8* %6, align 1
  %7 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 2, i16 1
  store i8 0, i8* %7, align 1
  br label %bb8

bb3:                                              ; preds = %start
  %8 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 0
  store i8 1, i8* %8, align 1
  %9 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 2, i16 0
  store i8 2, i8* %9, align 1
  %10 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 2, i16 1
  store i8 0, i8* %10, align 1
  %11 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 2, i16 2
  store i8 0, i8* %11, align 1
  %12 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 2, i16 3
  store i8 1, i8* %12, align 1
  %13 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 2, i16 4
  store i8 0, i8* %13, align 1
  br label %bb8

bb4:                                              ; preds = %start
  %14 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 0
  store i8 1, i8* %14, align 1
  %15 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 2, i16 0
  store i8 2, i8* %15, align 1
  %16 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 2, i16 1
  store i8 1, i8* %16, align 1
  %17 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 2, i16 2
  store i8 0, i8* %17, align 1
  %18 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 2, i16 3
  store i8 1, i8* %18, align 1
  %19 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 2, i16 4
  store i8 0, i8* %19, align 1
  br label %bb8

bb5:                                              ; preds = %start
  %20 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 0
  store i8 1, i8* %20, align 1
  %21 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 2, i16 0
  store i8 2, i8* %21, align 1
  %22 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 2, i16 1
  %23 = bitcast i8* %22 to i32*
  store i32 0, i32* %23, align 1
  br label %bb8

bb6:                                              ; preds = %start
  %24 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 0
  store i8 1, i8* %24, align 1
  %25 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 2, i16 0
  store i8 3, i8* %25, align 1
  %26 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 2, i16 1
  store i8 0, i8* %26, align 1
  %27 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 2, i16 2
  store i8 0, i8* %27, align 1
  br label %bb8

bb7:                                              ; preds = %start
  %28 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 0
  store i8 0, i8* %28, align 1
  br label %bb8

bb8:                                              ; preds = %bb1, %bb2, %bb3, %bb4, %bb5, %bb6, %bb7
  ret void
}

; Function Attrs: nounwind uwtable
define void @_ZN12chip8_engine7machine7Machine3new17hf50262d138e2457eE(%"machine::Machine"* noalias nocapture sret dereferenceable(20)) unnamed_addr #1 {
start:
  %1 = getelementptr inbounds %"machine::Machine", %"machine::Machine"* %0, i16 0, i32 4, i16 0
  call void @llvm.memset.p0i8.i16(i8* %1, i8 0, i16 16, i32 2, i1 false)
  %2 = getelementptr inbounds %"machine::Machine", %"machine::Machine"* %0, i16 0, i32 0
  store i16 0, i16* %2, align 2
  %3 = getelementptr inbounds %"machine::Machine", %"machine::Machine"* %0, i16 0, i32 2
  store i16 512, i16* %3, align 2
  ret void
}

; Function Attrs: argmemonly nounwind
declare void @llvm.memset.p0i8.i16(i8* nocapture writeonly, i8, i16, i32, i1) #2

attributes #0 = { noinline nounwind uwtable }
attributes #1 = { nounwind uwtable }
attributes #2 = { argmemonly nounwind }

The corresponding .o file has the following nop branch:

  10:   63 30           cpi     r22, 0x03       ; 3
  12:   01 f4           brne    .+0             ; 0x14 <_ZN12chip8_engine7opcode
s6decode17haab3c6c935229a6aE+0x14>
  14:   81 e0           ldi     r24, 0x01       ; 1

leading to, when linking it with more stuff:

chip8_engine.cgu-0.rs:(.text._ZN12chip8_engine7opcodes6decode17h793b52fb996d0b7aE+0x12): relocation truncated to fit: R_AVR_7_PCREL against `no symbol'

@gergoerdi
Copy link
Collaborator Author

If I cut down on the IR file, the linking error can go away because it is the interaction of these branch-by-0 ops and overall function sizes that causes them. However, just to trigger these branch-by-0 ops, the following IR is enough:

; ModuleID = 'chip8_engine.cgu-0.rs'
source_filename = "chip8_engine.cgu-0.rs"
target datalayout = "e-p:16:16:16-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-n8"
target triple = "avr-atmel-none"

%"libcore_mini::option::Option<opcodes::Op>" = type { i8, [0 x i8], [5 x i8] }

; Function Attrs: noinline nounwind uwtable
define void @_ZN12chip8_engine7opcodes6decode17haab3c6c935229a6aE(%"libcore_mini::option::Option<opcodes::Op>"* noalias nocapture sret dereferenceable(6), i8) unnamed_addr #0 {
start:
  %cond = icmp eq i8 %1, 1
  br i1 %cond, label %bb1, label %bb3

bb1:                                              ; preds = %start
  %2 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 2, i16 0
  store i8 0, i8* %2, align 1
  %3 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 2, i16 1
  store i8 0, i8* %3, align 1
  br label %bb3

bb3:                                              ; preds = %start, %bb1
  %.sink = phi i8 [ 1, %bb1 ], [ 0, %start ]
  %4 = getelementptr inbounds %"libcore_mini::option::Option<opcodes::Op>", %"libcore_mini::option::Option<opcodes::Op>"* %0, i16 0, i32 0
  store i8 %.sink, i8* %4, align 1
  ret void
}

attributes #0 = { noinline nounwind uwtable }

Note the instruction at 0x08:

00000000 <_ZN12chip8_engine7opcodes6decode17haab3c6c935229a6aE>:
   0:   e8 2f           mov     r30, r24
   2:   f9 2f           mov     r31, r25
   4:   80 e0           ldi     r24, 0x00       ; 0
   6:   61 30           cpi     r22, 0x01       ; 1
   8:   01 f4           brne    .+0             ; 0xa <_ZN12chip8_engine7opcodes6decode17haab3c6c935229a6aE+0xa>
   a:   81 83           std     Z+1, r24        ; 0x01
   c:   82 83           std     Z+2, r24        ; 0x02
   e:   81 e0           ldi     r24, 0x01       ; 1

00000010 <LBB0_2>:
  10:   80 83           st      Z, r24
  12:   08 95           ret

@gergoerdi
Copy link
Collaborator Author

I looked at this last example where linking succeeds, and it looks like those branch-by-0 instructions are actually just placeholders to be filled in by the linker; for example, with the code in #44 (comment), the linked .elf file has

000000d0 <_ZN12chip8_engine7opcodes6decode17h793b52fb996d0b7aE>:
  d0:   e8 2f           mov     r30, r24
  d2:   f9 2f           mov     r31, r25
  d4:   80 e0           ldi     r24, 0x00       ; 0
  d6:   61 30           cpi     r22, 0x01       ; 1
  d8:   19 f4           brne    .+6             ; 0xe0 <LBB0_2>
  da:   81 83           std     Z+1, r24        ; 0x01
  dc:   82 83           std     Z+2, r24        ; 0x02
  de:   81 e0           ldi     r24, 0x01       ; 1

000000e0 <LBB0_2>:
  e0:   80 83           st      Z, r24
  e2:   08 95           ret

(note the branch at 0xd8)

So I guess these are not NOP branches, they are just to be filled in by the linker. The problem still remains though: the offset to fill in is sometimes too large.

@gergoerdi gergoerdi changed the title "Relocation truncated to fit" from superfluous(?) branches "Relocation truncated to fit" from branches in long functions May 1, 2017
@dylanmckay
Copy link
Member

I would've assumed that the linker would see that the branch is out of range and automatically inserted a trampoline with a large branch instruction.

It's difficult to do that pre-link time because it's unclear where each section will be placed and so we cannot really deduce the final target offset of a lot of the branches.

We should investigate what GCC does here - we may need to implement some sort of trampolining ourselves.

@gergoerdi
Copy link
Collaborator Author

I'll look into reproducing a similar function in GCC and checking the resulting code later, as time permits.

However, given these are intra-function branches represented by relative addresses, why do they need any link-time fixup at all? Is it just to implement a poor-man's-two stage assembler? Or does linking sometimes move parts of a function around, relative to its other parts?

@dylanmckay
Copy link
Member

However, given these are intra-function branches represented by relative addresses, why do they need any link-time fixup at all? Is it just to implement a poor-man's-two stage assembler? Or does linking sometimes move parts of a function around, relative to its other parts?

For this specific case, I'm not entirely sure. In the past, I made the backend very conservative on what it promoted to relocations, leading to us perming most fixups at compile time.

I changed it a while ago so that the compiler output is more like GCC, which lowers almost all fixups to relocations. This allowed me to run a script to compile all of our machine code tests and compare the generated machine code to GCC.

@gergoerdi
Copy link
Collaborator Author

GCC generates a two-step branch exactly when needed. The following code compiles to a single breq, but if the last assignment is uncommented, it compiles to a two-step brne + rjmp:

#include <avr/io.h>

int main ()
{
    DDRB |= _BV(DDB5);

    while (PINC != 0)
    {
        PORTC = 1;
        PORTC = 2;
        PORTC = 3;
        PORTC = 4;
        PORTC = 1;
        PORTC = 2;
        PORTC = 3;
        PORTC = 4;
        PORTC = 1;
        PORTC = 2;
        PORTC = 3;
        PORTC = 4;
        PORTC = 1;
        PORTC = 2;
        PORTC = 3;
        PORTC = 4;
        PORTC = 1;
        PORTC = 2;
        PORTC = 3;
        PORTC = 4;
        PORTC = 1;
        PORTC = 2;
        PORTC = 3;
        PORTC = 4;
        PORTC = 1;
        PORTC = 2;
        PORTC = 3;
        PORTC = 4;
        PORTC = 1;
        PORTC = 2;
        PORTC = 3;
        PORTC = 4;
        PORTC = 1;
        PORTC = 2;
        PORTC = 3;
        PORTC = 4;
        PORTC = 1;
        PORTC = 2;
        PORTC = 3;
        PORTC = 4;
        PORTC = 1;
        PORTC = 2;
        PORTC = 3;
        PORTC = 4;
        PORTC = 1;
        PORTC = 2;
        PORTC = 3;
        PORTC = 4;
        PORTC = 1;
        PORTC = 2;
        PORTC = 3;
        PORTC = 4;
        PORTC = 1;
        PORTC = 2;
        PORTC = 3;
        /* PORTC = 4; */
    }
}

@dylanmckay
Copy link
Member

That's interesting, it looks like we'll have to implement that behaviour too.

Now that I think about it, we don't do any kind of branch relaxation. I think we used to have a custom pass that did it, but I couldn't upstream it because the reviewer wanted us to use the 'generic LLVM branch relaxation' pass instead.

@dylanmckay
Copy link
Member

Here's the patch where we decided to use the generic relaxation pass.

@gergoerdi
Copy link
Collaborator Author

Yeah, I'm looking at that right now. Being the total LLVM noob, I'm tripping up on how to define a "virtual" / "synthetic" AVR opcode that consists of a branch + a rjmp, just so I have something to put in Res in relaxInstruction.

@dylanmckay
Copy link
Member

I'm not too sure of the background but from my understanding, you want to define a synthetic instruction which gets expanded into a branch plus a jump?

If so, the LLVM formally calls these 'pseudo' instructions.

In order to define them, you can add new entries to AVRInstrInfo.td. You'll notice that all of the instruction definitions look like this.

// Normal instruction
def MyInst : FRdRr<1, 213, 1231231, <ins>);

// Pseudo instruction
def Foo : Pseudo<....>;

In order to write code to expand these pseudos, you can add some handling code to AVRExpandPseudoInsts.cpp.

@gergoerdi
Copy link
Collaborator Author

Yes I think that's exactly what I was looking for.

@gergoerdi
Copy link
Collaborator Author

Aaaaaaaargh! It seems AVRExpandPseudo runs before AsmBackend::relaxInstruction, so relaxInstructioncan't emit pseudo-instructions... does that mean I have to put this in AVRRelaxMemOperations instead of AVRAsmBackend?

@dylanmckay
Copy link
Member

There are a few places you could put it. One thing I am curious about though - why do you need a pseudo instruction in this case? Normally you'd want one if you needed something that the instruction selector could pattern match during instruction selection so you can write a custom pass to expand it into MachineInstrs later.

In this specific case, you're free build MachineInstrs anyway.

@gergoerdi
Copy link
Collaborator Author

Yes, but the type of relaxInstruction is such that it returns a single MCInstr, unless I'm misreading it (which is entirely possible). So I need some single instruction to represent a relaxed branch (which is 2 real instructions).

@dylanmckay
Copy link
Member

I'm looking into relaxInstruction now and it looks like it may actually be too restrictive for our needs.

For context, here is how operations/instructions are represented at different parts of the pipeline.

llvm::Value -> llvm::SDNode -> llvm::MachineInstr -> llvm::MCInst -> output file

The further along the pipeline you go, the less information you have.

Pseudo instructions exist during the MachineInstr part of the pipeline - a MachineInstr is basically a generalised target instruction, but it also supports pseudos.

After pseudos are expanded and just before we're actually done generating the output, everything is lowered to MCInstr objects, which are basically just opcodes and a list of operands. Nothing fancy is supported here - all registers must be physical, we cannot have pseudo instructions, etc.

It looks like the relaxInstruction function only supports having a 1:1 branch relaxation mapping. We can only relax a single instruction into one other instruction.

I haven't looked into it yet, but that doesn't sound like enough for the AVR target. I think we will always need to insert a trampoline in these cases, which is a minimum two instructions. It also doesn't really fit inside what relaxInstruction supports.

Given that this looks to be the case, I don't see any problem with reinstating the old branch selection pass.

Given that you've been looking at this problem @gergoerdi, does that sound correct to you?

@gergoerdi
Copy link
Collaborator Author

Yes exactly, that's what I was alluding to with "the type of relaxInstruction is such that it returns a single MCInstr". I made a hacked-up version of LLVM's AVR target which doesn't use the relaxation framework and instead just unconditionally uses a two-step branch everywhere.

This allowed me to make further progress on what I'm doing; however, I'll need to clean it up a bit before posting it. Also, I'll need to convince myself first that my patch is correct -- now that I can compile and link it fully, I'm seeing some strange behaviour from my program that I'm not sure yet if it is because of something I screwed up in the relaxation code, or some other, unrelated LLVM AVR or Rust AVR bug.

gergoerdi added a commit to gergoerdi/llvm-avr that referenced this issue May 7, 2017
For now, these relaxed branches are used unconditionally, even when
the branch target is nearby.
avr-rust/rust-legacy-fork#44
gergoerdi added a commit to gergoerdi/llvm-avr that referenced this issue May 7, 2017
For now, these relaxed branches are used unconditionally, even when
the branch target is nearby.
avr-rust/rust-legacy-fork#44
gergoerdi added a commit to gergoerdi/llvm-avr that referenced this issue May 7, 2017
For now, these relaxed branches are used unconditionally, even when
the branch target is nearby.
avr-rust/rust-legacy-fork#44
@shepmaster shepmaster added A-llvm Affects the LLVM AVR backend has-reduced-testcase A small LLVM IR file exists that demonstrates the problem labels May 10, 2017
@dylanmckay
Copy link
Member

I'm looking at this again

I'm not sure why we need pseudo instructions for this. Here is the original patch which partly added the relaxation framework.

The main part is this function

 virtual unsigned insertUnconditionalBranch(MachineBasicBlock &MBB,
                                             MachineBasicBlock &NewDestBB,
                                             const DebugLoc &DL,
                                             int64_t BrOffset = 0,
                                             RegScavenger *RS = nullptr) const {
    llvm_unreachable("target did not implement"); 

Because we have access to the basic block, we should be able to insert as many instructions as we need.

Also, here is a WIP implementation of it that I wrote a year ago link.

@dylanmckay
Copy link
Member

I can't seem to properly replicate this.

I believe I can see the out of range relocations in the executable

00000006 R_AVR_7_PCREL     .text+0x00000028
0000000a R_AVR_7_PCREL     .text+0x00000044
0000000e R_AVR_7_PCREL     .text+0x00000050
00000012 R_AVR_7_PCREL     .text+0x00000090
0000002a R_AVR_7_PCREL     .text+0x0000005c
0000002e R_AVR_7_PCREL     .text+0x00000070
00000032 R_AVR_7_PCREL     .text+0x00000090

But avr-gcc doesn't warn about anything

avr-gcc example.old -o example.old.linked

gergoerdi added a commit to gergoerdi/llvm-avr that referenced this issue Jun 17, 2017
For now, these relaxed branches are used unconditionally, even when
the branch target is nearby.
avr-rust/rust-legacy-fork#44
gergoerdi added a commit to gergoerdi/llvm-avr that referenced this issue Jun 18, 2017
For now, these relaxed branches are used unconditionally, even when
the branch target is nearby.
avr-rust/rust-legacy-fork#44
@dylanmckay
Copy link
Member

I'm going to reinstate the AVRBSel pass that did this from the old avr-llvm/llvm GitHub repository.

@dylanmckay
Copy link
Member

Upstreamed in r307109 and cherry-picked in cd29cc0e8d264f3000bbbe39465e7a44bb78dbf4.

@dylanmckay
Copy link
Member

That didn't seem to fix it, so I will revert.

./bin/llc -march=avr test.ll -o test.o -filetype=obj && avr-objdump -r test.o

test.o:     file format elf32-avr

RELOCATION RECORDS FOR [.text]:
OFFSET   TYPE              VALUE
00000006 R_AVR_7_PCREL     .text+0x00000028
0000000a R_AVR_7_PCREL     .text+0x00000044
0000000e R_AVR_7_PCREL     .text+0x00000050
00000012 R_AVR_7_PCREL     .text+0x00000090
0000002a R_AVR_7_PCREL     .text+0x0000005c
0000002e R_AVR_7_PCREL     .text+0x00000070
00000032 R_AVR_7_PCREL     .text+0x00000090

@dylanmckay
Copy link
Member

@gergoerdi Can you please comment the output of avr-gcc --version and avr-ld --version so I can compare?

@dylanmckay
Copy link
Member

I've got a patch which fully implements the target-independent branch relaxer for AVR.

@dylanmckay
Copy link
Member

Fixed in r307617 and cherry-picked in 480ebd5.

@dylanmckay dylanmckay added the has-llvm-commit This issue should be fixed in upstream LLVM label Jul 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-llvm Affects the LLVM AVR backend has-llvm-commit This issue should be fixed in upstream LLVM has-reduced-testcase A small LLVM IR file exists that demonstrates the problem
Projects
None yet
Development

No branches or pull requests

3 participants