Skip to content

Potential bug in shl operator implementation #52

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
carlos4242 opened this issue May 10, 2017 · 4 comments
Closed

Potential bug in shl operator implementation #52

carlos4242 opened this issue May 10, 2017 · 4 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

@carlos4242
Copy link

carlos4242 commented May 10, 2017

avrio.c.txt
avrio.ll.txt
blink.s.txt

I notice that this IL...

pinMask.exit:                                     ; preds = %entry, %if.then7.i
  %conv.i.sink = phi i16 [ %sub.i, %if.then7.i ], [ %conv.i, %entry ]
  %shl.i = shl i16 1, %conv.i.sink
  %conv3.i = trunc i16 %shl.i to i8
  %tobool = icmp eq i8 %conv3.i, 0
  br i1 %tobool, label %cleanup, label %if.end

There is what looks like an error with the shl operator implementation.

This seems to be the assembly language created...

00000162 <LBB2_3>:
 162:	21 e0       	ldi	r18, 0x01	; 1
 164:	30 e0       	ldi	r19, 0x00	; 0
 166:	e0 15       	cp	r30, r0
 168:	21 f0       	breq	.+8      	; 0x172 <LBB2_5>

0000016a <LBB2_4>:
 16a:	22 0f       	add	r18, r18
 16c:	33 1f       	adc	r19, r19
 16e:	e1 50       	subi	r30, 0x01	; 1
 170:	e1 f7       	brne	.-8      	; 0x16a <LBB2_4>

00000172 <LBB2_5>:

I think the issue is with cp r30, r0. I don't see anywhere in the full assembly that r0 is set, meaning that it could be any value. After a power on it should probably be 0 so this may work in most cases but it looks like a bug. Should it not instead be cpi r30, 00?

It's pretty weird that this IL resulted from compiling the attached c file as I've tried very hard to force unsigned char as the data type everywhere but that's a front end issue rather than an IL -> AVR ASM issue.

@shepmaster
Copy link
Member

Hello! This is the AVR-Rust repository; could you please show the corresponding Rust code that generates the issue? Otherwise, you are probably better off opening an issue on the LLVM issue tracker

@shepmaster shepmaster added the A-llvm Affects the LLVM AVR backend label May 10, 2017
@gergoerdi
Copy link
Collaborator

I have a fix for this at avr-rust/llvm@910429c, looks like I forgot to file the bug.

@dylanmckay dylanmckay added has-llvm-commit This issue should be fixed in upstream LLVM has-reduced-testcase A small LLVM IR file exists that demonstrates the problem labels May 31, 2017
@dylanmckay
Copy link
Member

Fix upstreamed in r304284

@dylanmckay
Copy link
Member

Fix has been cherry picked into avr-support branch in 301cc4e.

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

4 participants