Skip to content

Structure passed incorrectly to function #48

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

Closed
gergoerdi opened this issue May 7, 2017 · 5 comments
Closed

Structure passed incorrectly to function #48

gergoerdi opened this issue May 7, 2017 · 5 comments
Labels
A-llvm Affects the LLVM AVR backend has-reduced-testcase A small LLVM IR file exists that demonstrates the problem

Comments

@gergoerdi
Copy link
Collaborator

I have the following two functions, where one just calls the other after unpacking a struct:

pub struct S {
    pub op : u8,
    pub x : u8,
    pub addr : u16,
    pub nn : u8,
}

pub enum R {
    A,
    B(u16),
    C(u8),
}

#[inline(never)]
pub fn indirect(raw: S) -> Option<R> {
    direct(raw.op, raw.nn, raw.x, raw.addr)
}

#[inline(never)]
pub fn direct(op: u8, nn: u8, x: u8, addr: u16) -> Option<R> {
    match op {
        0x0 => Some(R::A),
        0x7 => Some(R::B(addr)),
        0xb => Some(R::B(addr)),
        0xf => match nn {
            0x0a => Some(R::C(x)),
            _ => None
        },
        _ => None
    }
}

I am calling it twice, once directly and once via the struct unpacker:

    spi::sync(if direct(0x0f, 0x0a, 0x05, 0x1234).is_some() { 0x42 } else { 0xde });
    spi::sync(if indirect(S{op: 0x0f, nn: 0x0a, x: 0x05, addr: 0x1234}).is_some() { 0x42 } else { 0xde });

However, only the first (direct) one gives the correct result.

@gergoerdi
Copy link
Collaborator Author

Actually, this might be because of my proposed fix for #47 ... there's a suspicious lpm instruction in the assembly of indirect.

define void @_ZN8external8indirect17had629ddc775c421cE(%"libcore_mini::option::Option<R>"* 
noalias nocapture sret dereferenceable(6), %S* noalias nocapture readonly dereferenceable(6
)) unnamed_addr #0 {
start:
  %raw.sroa.0.0..sroa_idx = getelementptr inbounds %S, %S* %1, i16 0, i32 0
  %raw.sroa.0.0.copyload = load i16, i16* %raw.sroa.0.0..sroa_idx, align 2
  %raw.sroa.4.0..sroa_idx = getelementptr inbounds %S, %S* %1, i16 0, i32 2
  %raw.sroa.4.0.copyload = load i8, i8* %raw.sroa.4.0..sroa_idx, align 2
  %raw.sroa.5.0..sroa_idx = getelementptr inbounds %S, %S* %1, i16 0, i32 4
  %raw.sroa.5.0.copyload = load i8, i8* %raw.sroa.5.0..sroa_idx, align 1
  %raw.sroa.6.0..sroa_idx = getelementptr inbounds %S, %S* %1, i16 0, i32 6
  %raw.sroa.6.0.copyload = load i8, i8* %raw.sroa.6.0..sroa_idx, align 2
  tail call void @_ZN8external6direct17h8a2c1e60d46790c8E(%"libcore_mini::option::Option<R>"* noalias nocapture nonnull sret dereferenceable(6) %0, i8 %raw.sroa.4.0.copyload, i8 %raw.sroa.6.0.copyload, i8 %raw.sroa.5.0.copyload, i16 %raw.sroa.0.0.copyload)
  ret void
}
00000000 <_ZN8external8indirect17had629ddc775c421cE>:
   0:   0f 93           push    r16
   2:   1f 93           push    r17
   4:   fb 01           movw    r30, r22
   6:   44 81           ldd     r20, Z+4        ; 0x04
   8:   23 81           ldd     r18, Z+3        ; 0x03
   a:   01 91           ld      r16, Z+
   c:   11 91           ld      r17, Z+
   e:   64 91           lpm     r22, Z
  10:   0e 94 00 00     call    0       ; 0x0 <_ZN8external8indirect17had629ddc775c421cE>
  14:   1f 91           pop     r17
  16:   0f 91           pop     r16
  18:   08 95           ret

@gergoerdi
Copy link
Collaborator Author

Yep, going back to e532848b145f3fc404212383e79f118f354f3d0f, i.e. just before my change to select lpm for load, fixes this.

So I guess we'll need to be smart about either using lpm or ld when selecting for a load, based on where the pointer is pointing...

@gergoerdi
Copy link
Collaborator Author

gergoerdi commented May 7, 2017

Simplified example:

pub struct S {
    pub x : u8,
    pub y : u16,
}

#[inline(never)]
pub fn indirect(raw: S) -> u8 {
    direct(raw.x, raw.y)
}

#[inline(never)]
pub fn direct(x: u8, y: u16) -> u8 {
    x + (y as u8)
}
; ModuleID = 'external.cgu-0.rs'
source_filename = "external.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"

%S = type { i8, [1 x i8], i16, [0 x i8] }

; Function Attrs: noinline norecurse nounwind readonly uwtable
define i8 @_ZN8external8indirect17h3cb4d883dc7f13c3E(%S* noalias nocapture readonly dereferenceable(4)) unnamed_addr #0 {
start:
  %1 = getelementptr inbounds %S, %S* %0, i16 0, i32 0
  %2 = getelementptr inbounds %S, %S* %0, i16 0, i32 2
  %3 = load i8, i8* %1, align 1
  %4 = load i16, i16* %2, align 2
  %5 = tail call i8 @_ZN8external6direct17h0ed4e1d916080c81E(i8 %3, i16 %4)
  ret i8 %5
}

; Function Attrs: noinline norecurse nounwind readnone uwtable
define i8 @_ZN8external6direct17h0ed4e1d916080c81E(i8, i16) unnamed_addr #1 {
start:
  %2 = trunc i16 %1 to i8
  %3 = add i8 %2, %0
  ret i8 %3
}

attributes #0 = { noinline norecurse nounwind readonly uwtable }
attributes #1 = { noinline norecurse nounwind readnone uwtable }

Compiled indirect:

_ZN8external8indirect17h3cb4d883dc7f13c3E: ; @_ZN8external8indirect17h3cb4d883dc7f13c3E
; BB#0:                                 ; %start
        movw    r30, r24
        ldd     r22, Z+2
        ldd     r23, Z+3
        lpm     r24, Z
        call    _ZN8external6direct17h0ed4e1d916080c81E
        ret

Before my changes:

_ZN8external8indirect17h3cb4d883dc7f13c3E: ; @_ZN8external8indirect17h3cb4d883dc7f13c3E
; BB#0:                                 ; %start
        movw    r30, r24
        ldd     r22, Z+2
        ldd     r23, Z+3
        ld      r24, Z
        call    _ZN8external6direct17h0ed4e1d916080c81E
        ret

So the question, then, is how to dispatch to ld or lpm depending in the load's argument.

@gergoerdi
Copy link
Collaborator Author

Truly minimal:

; ModuleID = 'external.cgu-0.rs'
source_filename = "external.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"

%S = type { i8 }

; Function Attrs: noinline norecurse nounwind readonly uwtable
define i8 @_ZN8external8indirect17h3cb4d883dc7f13c3E(%S* noalias nocapture readonly dereferenceable(4)) unnamed_addr #0 {
start:
  %1 = getelementptr inbounds %S, %S* %0, i16 0, i32 0
  %2 = load i8, i8* %1, align 1
  ret i8 %2
}

attributes #0 = { noinline norecurse nounwind readonly uwtable }

Instruction selection trace: (I hope to find out more about interpreting this output):

ISEL: Starting pattern match on root node: t5: i8,ch = load<LD1[%1](dereferenceable)> t0, t2, undef:i16

  Initial Opcode index to 581
  TypeSwitch[i8] from 590 to 593
  Match failed at index 595
  Continuing at 624
  Match failed at index 626
  Continuing at 662
  Match failed at index 667
  Continuing at 754
  TypeSwitch[i8] from 761 to 764
  Morphed node: t5: i8,ch = LPMRdZ<Mem:LD1[%1](dereferenceable)> t2, t0

gergoerdi added a commit to gergoerdi/llvm-avr that referenced this issue May 9, 2017

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@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

Closing this as it's caused by a hack on your branch that isn't in avr-rust's llvm fork.

The hack won't be necessary once #47 is fixed.

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-reduced-testcase A small LLVM IR file exists that demonstrates the problem
Projects
None yet
Development

No branches or pull requests

3 participants