LowerCallResult regression bugfix #10
Conversation
bugfix: - The reversal function call had somehow got removed from LowerCallResult, meaning calls returning i32 (or greater) were effectively being word swapped improvements: - The function name ReverseArgumentsToBigEndian is (I think) a bit misleading, I suggest something like ReverseReturnValuePartsToEnsureLittleEndian - Also added some explanatory comments to the function. - Moved the check for whether this is a split return value or not outside the function as I think that's more clear. tests: - Added a test for the regression in rust-lang#92, which is also causing rust-lang#130 - corrected directmem.ll at the same time as that had become broken by e2baccf
|
@dylanmckay no pressure intended, but the "easiest" way to get these PRs from this repo into the new llvm-project repo would be to merge them into upstream LLVM and then we can cherry-pick them 😉 |
|
I've created two branches in my LLVM fork for upstreaming
The first one, the fix for avr-rust/rust-legacy-fork#92 looks good and it doesn't introduce test failures. The second one, the fix for avr-rust/rust-legacy-fork#129, currently fails on the newly introduced test, The assembly that is now generated for the .p2align 1
.type setServoAngle2,@function
setServoAngle2: ; @setServoAngle2
; %bb.0: ; %entry
ldi r18, 19
ldi r19, 0
mov r24, r22
mov r25, r23
mov r22, r18
mov r23, r19
call __mulhi3
retBeforehand, 4 bytes were explicitly loaded to repreesnt the integer literal multiplicand, but now only the lower two are loaded, seemingly leaving the top two bytes undefined. Do you know what might be going on @carlos4242 ? |
|
I’d need to look carefully.
Just to be clear, it looks like you’re mentioning the same bug twice? Did you mean to say 92 for the first one and 129 for the second one?
Carl
… On 16 May 2019, at 14:02, Dylan McKay ***@***.***> wrote:
I've created two branches in my LLVM fork for upstreaming
https://github.com/dylanmckay/llvm/commits/upstream-fix-avr-rust-92
https://github.com/dylanmckay/llvm/commits/upstream-fix-avr-rust-129
The first one, the fix for avr-rust/rust-legacy-fork#129 looks good and it doesn't introduce test failures. The second one, the fix for avr-rust/rust-legacy-fork#129, currently fails on the newly introduced test, test/CodeGen/AVR/call-avr-rust-issue-130-regression.ll.
FAIL: LLVM :: CodeGen/AVR/call-avr-rust-issue-130-regression.ll (1 of 1)
******************** TEST 'LLVM :: CodeGen/AVR/call-avr-rust-issue-130-regression.ll' FAILED ********************
Script:
--
: 'RUN: at line 1'; /home/dylan/projects/builds/llvm-project/llvm/bin/llc -O1 < /home/dylan/projects/llvm-project/llvm/test/CodeGen/AVR/call-avr-rust-issue-130-regression.ll -march=avr | /home/dylan/projects/builds/llvm-project/llvm/bin/FileCheck /home/dylan/projects/llvm-project/llvm/test/CodeGen/AVR/call-avr-rust-issue-130-regression.ll
--
Exit Code: 1
Command Output (stderr):
--
/home/dylan/projects/llvm-project/llvm/test/CodeGen/AVR/call-avr-rust-issue-130-regression.ll:22:10: error: CHECK: expected string not found in input
; CHECK: ldi r20, 0
^
<stdin>:26:2: note: scanning from here
mov r24, r22
^
--
********************
Testing Time: 0.18s
********************
Failing Tests (1):
LLVM :: CodeGen/AVR/call-avr-rust-issue-130-regression.ll
Unexpected Failures: 1
1 warning(s) in tests.
The assembly that is now generated for the setServoAngle2 method is
.p2align 1
.type ***@***.***
setServoAngle2: ; @setServoAngle2
; %bb.0: ; %entry
ldi r18, 19
ldi r19, 0
mov r24, r22
mov r25, r23
mov r22, r18
mov r23, r19
call __mulhi3
ret
Beforehand, 4 bytes were explicitly loaded to repreesnt the integer literal multiplicand, but now only the lower two are loaded, seemingly leaving the top two bytes undefined.
Do you know what might be going on @carlos4242 ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Yes, my mistake, I have edited my comment. |
|
Ok. It will take me a few days to check it over. I’m pretty sure the test that’s regressed is one I wrote to catch the regression on 130? So I’ll need to have a nice cup of tea, sit down and look over it carefully. :) [Glad we are getting these fixed now... side note: 128 is going to be a tricky one!] |
|
OK, had a chance to look more closely at this (sorry about the delay). This has me a little puzzled as it compiles fine on my local branch, which is basically My compiler outputs this for Which looks correct to me. The version you are getting is doing 16 bit multiplication, it's calling a different gcc runtime library function, Have you got some extra code on your branch before the patches, downstream from The assembly in your case should produce the same result and is more efficient, because the end result is truncated to 16 bit before return, any parts of the multiplicands beyond bit 16 would have been truncated away, so converting both multiplicands to 16 bit first then calling Which suggests this is probably a benign unit test fail, the test might just need updating. |
bugfix:
improvements:
tests: