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

CPI, and who knows what other instructions, are defined with wrong operand registers #50

Closed
gergoerdi opened this issue May 10, 2017 · 6 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

LLVM IR:

target triple = "avr-atmel-none"

define internal fastcc void @loopy() unnamed_addr {
start:
  br label %bb7.preheader

bb2.loopexit:                                     ; preds = %bb10
  %0 = icmp ult i8 %.iter.sroa.0.020, 20
  %1 = zext i1 %0 to i8
  %.iter.sroa.0.0 = add i8 %.iter.sroa.0.020, %1
  br i1 %0, label %bb7.preheader, label %bb4

bb7.preheader:                                    ; preds = %bb2.loopexit, %start
  %iter.sroa.0.0.21 = phi i8 [ 0, %start ], [ %.iter.sroa.0.020, %bb2.loopexit ]
  %.iter.sroa.0.020 = phi i8 [ 1, %start ], [ %.iter.sroa.0.0, %bb2.loopexit ]
  %2 = add i8 %iter.sroa.0.0.21, 10
  br label %bb10

bb4:                                              ; preds = %bb2.loopexit
  ret void

bb10:                                             ; preds = %bb10, %bb7.preheader
  %iter1.sroa.0.0.19 = phi i8 [ 0, %bb7.preheader ], [ %.iter1.sroa.0.018, %bb10 ]
  %.iter1.sroa.0.018 = phi i8 [ 1, %bb7.preheader ], [ %.iter1.sroa.0.0, %bb10 ]
  tail call fastcc void @observe(i8 %2, i8 %iter1.sroa.0.0.19)
  %3 = icmp ult i8 %.iter1.sroa.0.018, 48
  %4 = zext i1 %3 to i8
  %.iter1.sroa.0.0 = add i8 %.iter1.sroa.0.018, %4
  br i1 %3, label %bb10, label %bb2.loopexit
}

declare void @observe(i8, i8) unnamed_addr #2

Generated assembly with --filetype=asm:

	.text
	.file	"a.ll"
	.p2align	1
	.type	loopy,@function
loopy:                                  ; @loopy
; BB#0:                                 ; %start
	push	r15
	push	r16
	push	r17
	ldi	r24, 1
	ldi	r17, 0
LBB0_3:                                 ; %bb7.preheader
                                        ; =>This Loop Header: Depth=1
                                        ;     Child Loop BB0_4 Depth 2
	mov	r15, r24
	subi	r17, -10
	ldi	r16, 1
	ldi	r22, 0
LBB0_4:                                 ; %bb10
                                        ;   Parent Loop BB0_3 Depth=1
                                        ; =>  This Inner Loop Header: Depth=2
	mov	r24, r17
	call	observe
	cpi	r16, 48
	ldi	r24, 1
	brsh	.+2
	rjmp	LBB0_5
; BB#6:                                 ; %bb10
                                        ;   in Loop: Header=BB0_4 Depth=2
	ldi	r24, 0
LBB0_5:                                 ; %bb10
                                        ;   in Loop: Header=BB0_4 Depth=2
	add	r24, r16
	cpi	r16, 48
	mov	r22, r16
	mov	r16, r24
	brsh	.+2
	rjmp	LBB0_4
; BB#1:                                 ; %bb2.loopexit
                                        ;   in Loop: Header=BB0_3 Depth=1
	ldi	r24, 1
	cpi	r15, 20
	brsh	.+2
	rjmp	LBB0_2
; BB#7:                                 ; %bb2.loopexit
                                        ;   in Loop: Header=BB0_3 Depth=1
	ldi	r24, 0
LBB0_2:                                 ; %bb2.loopexit
                                        ;   in Loop: Header=BB0_3 Depth=1
	add	r24, r15
	cpi	r15, 20
	mov	r17, r15
	brsh	.+2
	rjmp	LBB0_3
; BB#8:                                 ; %bb4
	pop	r17
	pop	r16
	pop	r15
	ret
.Lfunc_end0:
	.size	loopy, .Lfunc_end0-loopy

Disassembly of output generated with --filetype=obj:


a.o:     file format elf32-avr


Disassembly of section .text:

00000000 <loopy>:
   0:	ff 92       	push	r15
   2:	0f 93       	push	r16
   4:	1f 93       	push	r17
   6:	81 e0       	ldi	r24, 0x01	; 1
   8:	10 e0       	ldi	r17, 0x00	; 0

0000000a <LBB0_3>:
   a:	f8 2e       	mov	r15, r24
   c:	16 5f       	subi	r17, 0xF6	; 246
   e:	01 e0       	ldi	r16, 0x01	; 1
  10:	60 e0       	ldi	r22, 0x00	; 0

00000012 <LBB0_4>:
  12:	81 2f       	mov	r24, r17
  14:	0e 94 00 00 	call	0	; 0x0 <loopy>
  18:	00 33       	cpi	r16, 0x30	; 48
  1a:	81 e0       	ldi	r24, 0x01	; 1
  1c:	08 f4       	brcc	.+2      	; 0x20 <LBB0_4+0xe>
  1e:	00 c0       	rjmp	.+0      	; 0x20 <LBB0_4+0xe>
  20:	80 e0       	ldi	r24, 0x00	; 0

00000022 <LBB0_5>:
  22:	80 0f       	add	r24, r16
  24:	00 33       	cpi	r16, 0x30	; 48
  26:	60 2f       	mov	r22, r16
  28:	08 2f       	mov	r16, r24
  2a:	08 f4       	brcc	.+2      	; 0x2e <LBB0_5+0xc>
  2c:	00 c0       	rjmp	.+0      	; 0x2e <LBB0_5+0xc>
  2e:	81 e0       	ldi	r24, 0x01	; 1
  30:	f4 31       	cpi	r31, 0x14	; 20
  32:	08 f4       	brcc	.+2      	; 0x36 <LBB0_5+0x14>
  34:	00 c0       	rjmp	.+0      	; 0x36 <LBB0_5+0x14>
  36:	80 e0       	ldi	r24, 0x00	; 0

00000038 <LBB0_2>:
  38:	8f 0d       	add	r24, r15
  3a:	f4 31       	cpi	r31, 0x14	; 20
  3c:	1f 2d       	mov	r17, r15
  3e:	08 f4       	brcc	.+2      	; 0x42 <LBB0_2+0xa>
  40:	00 c0       	rjmp	.+0      	; 0x42 <LBB0_2+0xa>
  42:	1f 91       	pop	r17
  44:	0f 91       	pop	r16
  46:	ff 90       	pop	r15
  48:	08 95       	ret

Note that the .o file uses r31 for comparisons (without ever filling that register) while the .s file, correctly, uses r15, e.g. in LBB0_2:

LBB0_2:                                 ; %bb2.loopexit
                                        ;   in Loop: Header=BB0_3 Depth=1
	add	r24, r15
	cpi	r15, 20
00000038 <LBB0_2>:
  38:	8f 0d       	add	r24, r15
  3a:	f4 31       	cpi	r31, 0x14	; 20
@gergoerdi
Copy link
Collaborator Author

cpi only works on registers in the range r16..r31, so the LLVM assembly couldn't possibly work. Also, r31 is encoded as 0xf, so it's not entirely surprising that the invalid r15 operand to cpi was mapped to r31.

@gergoerdi
Copy link
Collaborator Author

CPIRdK is defined in AVRInstrInfo.td to take an arbitrary 8-bit GPR:

def CPIRdK : FRdK<0b0011,
                  (outs),
                  (ins GPR8:$rd, imm_ldi8:$k),
                  "cpi\t$rd, $k",
                  [(AVRcmp i8:$rd, imm:$k), (implicit SREG)]>;

This is super scary because of course it's trivial to fix this one, but now I'm wondering how many other instructions are defined with incorrect register classes...

@gergoerdi gergoerdi changed the title Binary object file doesn't match textual assembly file CPI, and who knows what other instructions, are defined with wrong operand registers May 10, 2017
gergoerdi added a commit to gergoerdi/llvm-avr that referenced this issue May 10, 2017
@shepmaster shepmaster added the A-llvm Affects the LLVM AVR backend label May 10, 2017
@shepmaster
Copy link
Member

but now I'm wondering how many other instructions are defined with incorrect register classes...

Unfortunately, I can't think of any automated way to verify these. All that's coming to mind is to read through the manual and the code and check each one 🤷‍♀️ .

@shepmaster shepmaster added the has-reduced-testcase A small LLVM IR file exists that demonstrates the problem label May 11, 2017
@dylanmckay
Copy link
Member

Interesting. I believe this TableGen definition has been left untouched since I forked the project from SourceForge. I'm surprised that it included a bug this bug!

On the positive side, I implemented most of the instruction set myself, while carefully reading the manual. It is unlikely there are many (if any) serious bugs like this.

Really nice catch though!

gergoerdi added a commit to gergoerdi/llvm-avr that referenced this issue May 12, 2017
@dylanmckay dylanmckay added the has-llvm-commit This issue should be fixed in upstream LLVM label May 31, 2017
@dylanmckay
Copy link
Member

Fix upstreamed in r304283.

earl pushed a commit to earl/llvm-mirror that referenced this issue May 31, 2017
@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

3 participants