32 bit multiplication produces byte-swapped result

[carlos4242]

This is very similar to #129 and may well be the same bug/root cause or not.

I'll get a reduced test case later.

This swift code...

SetupSerial(baudRate: 9600)
func setServoAngle(angle: UInt32) -> UInt32 {
  var adjustedAngle: UInt32 = angle
  let adj2: UInt32 = adjustedAngle &* UInt32(19)
  return adj2
}

let rad = UInt8(truncatingBitPattern: random())
let ang = setServoAngle(angle: UInt32(rad))
print(unsignedInt: UInt16(rad))
print(longInt: Int32(bitPattern: ang))

while(true) {
}

Produces this LLVM IR...

; Function Attrs: noreturn
define i32 @main(i32, i8** nocapture readnone) local_unnamed_addr #0 {
entry:
  tail call void @_TF3AVR11SetupSerialFT8baudRateVs6UInt16_T_(i16 9600)
  %2 = tail call i16 @_TF3AVR6randomFT_Vs5Int16()
  %3 = trunc i16 %2 to i8
  store i8 %3, i8* getelementptr inbounds (%Vs5UInt8, %Vs5UInt8* @_Tv4main3radVs5UInt8, i64 0, i32 0), align 1
  %.mask = and i16 %2, 255
  %4 = zext i16 %.mask to i32
  %5 = mul nuw nsw i32 %4, 19
  store i32 %5, i32* getelementptr inbounds (%Vs6UInt32, %Vs6UInt32* @_Tv4main3angVs6UInt32, i64 0, i32 0), align 4
  %6 = tail call i1 @_TIF3AVR5printFT11unsignedIntVs6UInt1610addNewlineSb_T_A0_()
  tail call void @_TF3AVR5printFT11unsignedIntVs6UInt1610addNewlineSb_T_(i16 %.mask, i1 %6)
  %7 = tail call i1 @_TIF3AVR5printFT7longIntVs5Int3210addNewlineSb_T_A0_()
  tail call void @_TF3AVR5printFT7longIntVs5Int3210addNewlineSb_T_(i32 %5, i1 %7)
  br label %8

; <label>:8:                                      ; preds = %8, %entry
  br label %8
}

When run through llc it produces this assembly:

000001c0 <main>:
     1c0:	cf 92       	push	r12
     1c2:	df 92       	push	r13
     1c4:	ef 92       	push	r14
     1c6:	ff 92       	push	r15
     1c8:	0f 93       	push	r16
     1ca:	1f 93       	push	r17
     1cc:	80 e8       	ldi	r24, 0x80	; 128
     1ce:	95 e2       	ldi	r25, 0x25	; 37
     1d0:	0e 94 92 01 	call	0x324	; 0x324 <_TF3AVR11SetupSerialFT8baudRateVs6UInt16_T_>
     1d4:	0e 94 8c 01 	call	0x318	; 0x318 <_TF3AVR6randomFT_Vs5Int16>
     1d8:	8c 01       	movw	r16, r24
     1da:	00 93 8c 01 	sts	0x018C, r16
     1de:	10 70       	andi	r17, 0x00	; 0
     1e0:	23 e1       	ldi	r18, 0x13	; 19
     1e2:	30 e0       	ldi	r19, 0x00	; 0
     1e4:	40 e0       	ldi	r20, 0x00	; 0
     1e6:	50 e0       	ldi	r21, 0x00	; 0
     1e8:	b8 01       	movw	r22, r16
     1ea:	ca 01       	movw	r24, r20
     1ec:	0e 94 c5 10 	call	0x218a	; 0x218a <__mulsi3>
     1f0:	7c 01       	movw	r14, r24
     1f2:	6b 01       	movw	r12, r22
     1f4:	d0 92 93 01 	sts	0x0193, r13
     1f8:	c0 92 92 01 	sts	0x0192, r12
     1fc:	f0 92 91 01 	sts	0x0191, r15
     200:	e0 92 90 01 	sts	0x0190, r14
     204:	0e 94 26 02 	call	0x44c	; 0x44c <_TIF3AVR5printFT11unsignedIntVs6UInt1610addNewlineSb_T_A0_>
     208:	68 2f       	mov	r22, r24
     20a:	c8 01       	movw	r24, r16
     20c:	0e 94 28 02 	call	0x450	; 0x450 <_TF3AVR5printFT11unsignedIntVs6UInt1610addNewlineSb_T_>
     210:	0e 94 b2 01 	call	0x364	; 0x364 <_TIF3AVR5printFT7longIntVs5Int3210addNewlineSb_T_A0_>
     214:	48 2f       	mov	r20, r24
     216:	b7 01       	movw	r22, r14
     218:	c6 01       	movw	r24, r12
     21a:	0e 94 b4 01 	call	0x368	; 0x368 <_TF3AVR5printFT7longIntVs5Int3210addNewlineSb_T_>

0000021e <LBB0_1>:
     21e:	ff cf       	rjmp	.-2      	; 0x21e <LBB0_1>

Looking at the call to __mulsi3, the result comes back in r22-r25, but it looks like the register pairs are swapped.

When I test the code I get these results:

167
207945728

The calculation should be 167 * 19 = 3173

bit pattern
0000 0000 0000 0000 0000 1100 0110 0101

But instead we get 207945728

bit pattern
0000 1100 0110 0101 0000 0000 0000 0000

You can see that the words have been swapped.

[carlos4242]

I've come up with a patch that fixes it but I was surprised such a serious issue exists in the AVR back end. I just ran the unit tests and found they are regressing. call.ll is breaking and I'm pretty sure this issue is what's causing it. Can anyone remember when they were last run? Maybe we can triage which commit introduced the regression?

[carlos4242]

This is fixed by avr-rust/llvm#10

[carlos4242]

LABEL AS: has local patch

[dylanmckay]

I've added a 32-bit multiplication test to the compiler integration test suite dylanmckay/avr-compiler-integration-tests@d14e459 and it passes on LLVM trunk.

[carlos4242]

Cool, I was just about to login and try to figure out where we are with all these patches and threads. :)

I'm on holiday this week so hoping to have a bit of time. Last I knew we had patches for 129 and 130 but there was some interaction where some tests weren't passing. Can we get together on email or chat and sort out what's working and what's not if you have time?

It feels like we probably have everything fixed in one branch or another, just maybe needs a bit of organising and some of the tests tweaked?

Carl

[carlos4242]

p.s. I should really change my avatar one day. Since becoming a dad, I never have time to bleach my hair any more, it's short and gingerish. 🤣

[carlos4242]

So it looks like #130 is just the same as talking about a regression from #92, which is fixed by the patch https://github.com/dylanmckay/llvm/commits/upstream-fix-avr-rust-92, which has either gone to upstream already or is going. That probably means that we can close this issue, once the patch is upstreamed? Also you've added extra tests to the compiler integration test suite to help prevent future regressions in this area, so I think that's everything?

[carlos4242]

Cool Sent from my BlackBerry® wireless device

…

-----Original Message----- From: Dylan McKay <[email protected]> Date: Sat, 08 Jun 2019 06:42:44 To: avr-rust/rust<[email protected]> Reply-To: avr-rust/rust <[email protected]> Cc: Carl Peto<[email protected]>; Author<[email protected]> Subject: Re: [avr-rust/rust] 32 bit multiplication produces byte-swapped result (#130) I've added a 32-bit multiplication test to the compiler integration test suite dylanmckay/avr-compiler-integration-tests@d14e459 and it passes on LLVM trunk.

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #130 (comment)