Menu
▾
▴
Re: [avr-llvm-devel] Regression tests
|
From: Borja F. <bor...@gm...> - 2012-06-24 18:16:23
|
With that command line I get the same code as you (now i get the spill i
wasn't getting before), however i still get correct code:
movw r31:r30, r25:r24
std Y+1, r30
std Y+2, r31
ldi r18, 136
ldi r19, 153
ldi r20, 255
ldi r21, 238
ldi r22, 221
ldi r23, 204
ldi r24, 187
ldi r25, 170
icall
This is my usual command line:
clang -O3 -emit-llvm test.c -S -o test.bc -ccc-host-triple
avr-generic-generic -ccc-clang-archs avr
llc -O3 -march=avr test.bc -pre-RA-sched=default -regalloc=avrgreedy
-debug-only=isel -o test.S -print-isel-input -print-after-all -stats 2> dump
2012/6/24 Nicklas Bo Jensen <nbj...@gm...>
> All patches should be installed and it has worked earlier when I wrote
> that test. I also have the clang patches, but I don't think it has anything
> to do with those.
>
> To compile icall.c, I would for example do:
>
> ../build/Debug+Asserts/bin/clang icall.c -S -emit-llvm -o icall.ll
>
> to view the llvm ir and to get assembler:
>
> ../build/Debug+Asserts/bin/llc icall.ll -march=avr -O3 -o icall.s
>
> I have that exact code as you describe, I will printf debug it later
> tonight.
>
> Thanks!
>
>
>
> On Sun, Jun 24, 2012 at 1:58 PM, Borja Ferrer <bor...@gm...>wrote:
>
>> Ok the problem I see is that you're not getting the arguments pieces
>> reversed. Can you check the code that performs this? This is done inside
>> AnalyzeArguments, exactly in line 768 done by std::reverse. Also check that
>> this function is being called in line 932. That should give you enough info
>> to debug it, use fprintfs to print out debugging info to help you otherwise
>> it's going to be hard to debug.
>>
>> I'm assuming you have all patches installed, including clang patches
>> right?
>> Paste the command line you're using to call clang because I'm not getting
>> std Y+1, r30
>> std Y+2, r31
>> or any stack allocation as you get.
>>
>>
>> 2012/6/24 Nicklas Bo Jensen <nbj...@gm...>
>>
>>> Still gives the wrong code:
>>> ldi r24, 136
>>> ldi r25, 153
>>> ldi r22, 255
>>> ldi r23, 238
>>> ldi r20, 221
>>> ldi r21, 204
>>> ldi r18, 187
>>> ldi r19, 170
>>>
>>> I have also attached the output.
>>>
>>> On Sun, Jun 24, 2012 at 1:38 AM, Borja Ferrer <bor...@gm...>wrote:
>>>
>>>> What do you get with the following code?
>>>>
>>>> long icall(long (*x)(long long))
>>>> {
>>>> return x(0xAABBCCDDEEFF9988LL)+5;
>>>> }
>>>>
>>>> John could you check if you're getting the right code?
>>>>
>>>>
>>>> 2012/6/24 Nicklas Bo Jensen <nbj...@gm...>
>>>>
>>>>> No, regular calls/and/xor/or works fine with 3335977107.
>>>>>
>>>>> On Sun, Jun 24, 2012 at 12:45 AM, Borja Ferrer <bor...@gm...>wrote:
>>>>>
>>>>>> This is very weird. Do you get this problem with other code when you
>>>>>> use this particular number (3335977107) like with a regular call or with an
>>>>>> and/xor/or?
>>>>>>
>>>>>>
>>>>>> 2012/6/24 Nicklas Bo Jensen <nbj...@gm...>
>>>>>>
>>>>>>> I also get the wrong result compiling that snippet...
>>>>>>>
>>>>>>> I have tried to revert the last patches, those that not needed for
>>>>>>> syncing with the llvm repo, same result. I have also updated llvm. Same
>>>>>>> result.
>>>>>>>
>>>>>>> Please let me know if you have any ideas what it could be :)
>>>>>>>
>>>>>>> On Sat, Jun 23, 2012 at 8:15 PM, Borja Ferrer <bor...@gm...
>>>>>>> > wrote:
>>>>>>>
>>>>>>>> Thanks for the patch there!
>>>>>>>> Very interesting because I'm not getting that error in icall at
>>>>>>>> all, I'm getting the second code snippet you pasted.
>>>>>>>>
>>>>>>>> With this code:
>>>>>>>> long icall(long (*x)(long))
>>>>>>>> {
>>>>>>>> return x(0xC6D6F893)+5;
>>>>>>>> }
>>>>>>>>
>>>>>>>> I get:
>>>>>>>> movw r19:r18, r25:r24
>>>>>>>>
>>>>>>>> ldi r22, 147
>>>>>>>> ldi r23, 248
>>>>>>>> ldi r24, 214
>>>>>>>> ldi r25, 198
>>>>>>>> movw r31:r30, r19:r18
>>>>>>>> icall
>>>>>>>> subi r22, 251
>>>>>>>> sbci r23, 255
>>>>>>>> sbci r24, 255
>>>>>>>> sbci r25, 255
>>>>>>>> ret
>>>>>>>>
>>>>>>>> Can you re check to see what's going on?
>>>>>>>>
>>>>>>>> The comments in zext.ll and sext.ll are there for reference, they
>>>>>>>> cover the cases implemented in the pseudo instruction expander, that way
>>>>>>>> we're able to know what test covers each case.
>>>>>>>> Yes i have comments for the memory tests, I'll write them in the
>>>>>>>> next email. We should continue with memory operations, there are many more
>>>>>>>> things to test there that i'll list next time. For now we have to see
>>>>>>>> what's going on with that failing test.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 2012/6/23 Nicklas Bo Jensen <nbj...@gm...>
>>>>>>>>
>>>>>>>>> I've committed the small change in Targets.cpp as you described.
>>>>>>>>>
>>>>>>>>> We are breaking a test now. In call.s, icall is loading arguments
>>>>>>>>> incorrectly now.
>>>>>>>>>
>>>>>>>>> The instruction %1 = call i32 %foo(i32 3335977107)
>>>>>>>>> gives:
>>>>>>>>> ldi r24, 147
>>>>>>>>> ldi r25, 248
>>>>>>>>> ldi r22, 214
>>>>>>>>> ldi r23, 198
>>>>>>>>> but should give:
>>>>>>>>> ldi r22, 147
>>>>>>>>> ldi r23, 248
>>>>>>>>> ldi r24, 214
>>>>>>>>> ldi r25, 198
>>>>>>>>>
>>>>>>>>> Also in your new sext.ll and zext.ll tests there are some comments
>>>>>>>>> above each function, should these be deleted?
>>>>>>>>>
>>>>>>>>> Last did you have any comments above the memory tests?
>>>>>>>>>
>>>>>>>>> Any preferences for what I should continue on?
>>>>>>>>>
>>>>>>>>> Thanks!
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Tue, Jun 19, 2012 at 1:47 PM, Borja Ferrer <
>>>>>>>>> bor...@gm...> wrote:
>>>>>>>>>
>>>>>>>>>> Nicklas, in the meantime while the patch gets fixed you can
>>>>>>>>>> manually fix the error by adding that virtual BuiltinVaListKind
>>>>>>>>>> getBuiltinVaListKind() const function in the AVR class, in the above link
>>>>>>>>>> you have an example on how to do it.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 2012/6/19 Borja Ferrer <bor...@gm...>
>>>>>>>>>>
>>>>>>>>>>> John can you fix this issue, i have a different local copy of
>>>>>>>>>>> Targets.cpp that i don't want to change now with the patch in SVN. The fix
>>>>>>>>>>> is in this commit
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets.cpp?r1=158334&r2=158592&sortby=date&diff_format=h
>>>>>>>>>>>
>>>>>>>>>>> so updating the patch file will do it.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> 2012/6/18 Borja Ferrer <bor...@gm...>
>>>>>>>>>>>
>>>>>>>>>>>> You've done it right, the problem is here:
>>>>>>>>>>>>
>>>>>>>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets.cpp?r1=158334&r2=158592&sortby=date&diff_format=h
>>>>>>>>>>>>
>>>>>>>>>>>> it's caused by a recent API change that has broken out of tree
>>>>>>>>>>>> targets, i'll fix it soon.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> 2012/6/18 Nicklas Bo Jensen <nbj...@gm...>
>>>>>>>>>>>>
>>>>>>>>>>>>> Yes of course.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I'm having some problems compiling getting the following error:
>>>>>>>>>>>>>
>>>>>>>>>>>>> /avr-llvm/llvm/tools/clang/lib/Basic/Targets.cpp:4172:16:
>>>>>>>>>>>>> error: allocating an object of
>>>>>>>>>>>>> abstract class type 'AVRTargetInfo'
>>>>>>>>>>>>> return new AVRTargetInfo(T);
>>>>>>>>>>>>> ^
>>>>>>>>>>>>> /avr-llvm/llvm/tools/clang/lib/Basic/../../include/clang/Basic/TargetInfo.h:406:29:
>>>>>>>>>>>>> note:
>>>>>>>>>>>>> unimplemented pure virtual method 'getBuiltinVaListKind'
>>>>>>>>>>>>> in 'AVRTargetInfo'
>>>>>>>>>>>>> virtual BuiltinVaListKind getBuiltinVaListKind() const = 0;
>>>>>>>>>>>>> ^
>>>>>>>>>>>>> 1 error generated.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I think I have applied the patches from svn correctly, at
>>>>>>>>>>>>> least did it a few times. I'm not stuck, but any ideas would be nice.
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Mon, Jun 18, 2012 at 1:46 PM, Borja Ferrer <
>>>>>>>>>>>>> bor...@gm...> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Ok great!
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> One more thing about the icall test, it can be further
>>>>>>>>>>>>>> reduced because:
>>>>>>>>>>>>>> %1 = alloca i32 (i32)*
>>>>>>>>>>>>>> store i32 (i32)* %foo, i32 (i32)** %1
>>>>>>>>>>>>>> %2 = load i32 (i32)** %1
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> all that code is redundant, you're getting it because you've
>>>>>>>>>>>>>> ran it with -O0. If you think about it, you've allocated an i32 var in the
>>>>>>>>>>>>>> stack just to copy the value of foo but then it's not used at all. This
>>>>>>>>>>>>>> code is typical from unoptimized builds, so you can reduce it to something
>>>>>>>>>>>>>> like:
>>>>>>>>>>>>>> %3 = call i32 %foo(i32 3335977107)
>>>>>>>>>>>>>> %4 = add nsw i32 %3, 5
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> here you're using the foo incoming arg directly.
>>>>>>>>>>>>>> So as a recommendation, I would also run clang with -03 to
>>>>>>>>>>>>>> get optimized outputs that are usually clean of redundant allocas.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 2012/6/18 Nicklas Bo Jensen <nbj...@gm...>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I've committed the updated tests, excluding mem.ll awaiting
>>>>>>>>>>>>>>> your feedback on it (Besides not using immediate 0).
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Mon, Jun 18, 2012 at 1:10 AM, Borja Ferrer <
>>>>>>>>>>>>>>> bor...@gm...> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> return test:
>>>>>>>>>>>>>>>> 1) you can remove the @return8_reg() test since it's
>>>>>>>>>>>>>>>> completely commented out.
>>>>>>>>>>>>>>>> 2) in return64_arg2 you're always going to push r29:r28 as
>>>>>>>>>>>>>>>> we disccused in a previous email, so you can remove the SPLOW/HI vars, also
>>>>>>>>>>>>>>>> SP is going to be always the Y reg pair. Using Z here would be a bug.
>>>>>>>>>>>>>>>> 3) in return64_trunc same as in point 2.
>>>>>>>>>>>>>>>> 4) add a :TODO: test returning byval structs.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> call test:
>>>>>>>>>>>>>>>> 1) in the icall test, the code you're testing is a bit
>>>>>>>>>>>>>>>> fragile. Basically if you run it with -O3 it will get optimized away and
>>>>>>>>>>>>>>>> you wont get an icall. Instead, generate the icall using a function pointer
>>>>>>>>>>>>>>>> from an incoming argument.
>>>>>>>>>>>>>>>> 2) do the same as in 1 but passing an argument to the icall
>>>>>>>>>>>>>>>> (any data type will do). (remember i had to fix this last week, so testing
>>>>>>>>>>>>>>>> it is imporant in case it breaks again in the future)
>>>>>>>>>>>>>>>> 3) do the same as in 1 using the return value of the icall
>>>>>>>>>>>>>>>> (any data type will do).
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> One general thing concerning all written tests up to now,
>>>>>>>>>>>>>>>> can you change all imm values (including the ones already commited), so
>>>>>>>>>>>>>>>> that you don't test things like "ldi reg, 0". So for a 16bit type instead
>>>>>>>>>>>>>>>> of using a 5 use a value were none of the registers are zero like 0xABCD
>>>>>>>>>>>>>>>> instead of 0x0005 where the hi part is 0. In the future we'll add an
>>>>>>>>>>>>>>>> optimization that relies on r0 having a 0, so many tests will break because
>>>>>>>>>>>>>>>> of this since we could get a mov reg, r0 instead of an ldi.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I'll reply tomorrow with comments about the memory tests.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 2012/6/17 Borja Ferrer <bor...@gm...>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks, I'm reviewing them now
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> El viernes, 15 de junio de 2012, Nicklas Bo Jensen
>>>>>>>>>>>>>>>>> escribió:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Please review these tests.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Any comments?
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On Wed, Jun 6, 2012 at 2:46 AM, Borja Ferrer <
>>>>>>>>>>>>>>>>>> bor...@gm...> wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Fixed in SVN, let me know any other issues. Thanks for
>>>>>>>>>>>>>>>>>> reporting it!
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> 2012/6/5 Borja Ferrer <bor...@gm...>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Something has changed in llvm or maybe it never worked,
>>>>>>>>>>>>>>>>>> can't remember, but I'm unable to pass arguments through an icall, I'm
>>>>>>>>>>>>>>>>>> investigating for a fix. In the meantime you can disable that assertion
>>>>>>>>>>>>>>>>>> until I come with a proper fix.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> 2012/6/4 Borja Ferrer <bor...@gm...>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I'll take a look to it shortly.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> 2012/6/3 Nicklas Bo Jensen <nbj...@gm...>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Ok. Sure your explanations are really clear :) Thanks!
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I'm having some trouble testing icall, which i'm doing
>>>>>>>>>>>>>>>>>> using a function pointer. I've attached a small C example, the
>>>>>>>>>>>>>>>>>> corresponding cleaned up llvm code and the output of running llc on it.
>>>>>>>>>>>>>>>>>> There is a failed assertion.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Any thoughts?
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On Fri, Jun 1, 2012 at 7:04 PM, Borja Ferrer <
>>>>>>>>>>>>>>>>>> bor...@gm...> wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Just checking for the sub instructions that allocate and
>>>>>>>>>>>>>>>>>> restore the stack should be enough, also check that the allocated size is
>>>>>>>>>>>>>>>>>> correct, the rest of stuff can be omitted. I would also check the whole
>>>>>>>>>>>>>>>>>> sequence including the interrupt instructions in a single test, no need to
>>>>>>>>>>>>>>>>>> do it everywhere. When we get to test allocas we'll have to test for
>>>>>>>>>>>>>>>>>> prologue and epilogue code, which is very similar to the one you're seeing
>>>>>>>>>>>>>>>>>> here.
>>>>>>>>>>>>>>>>>> About testing push and pops: no need to test that, it's
>>>>>>>>>>>>>>>>>> part of the prologue/epilogue of the function, so it's not related to
>>>>>>>>>>>>>>>>>> calls, also, that is handled by llvm so I wouldn't test that at all.
>>>>>>>>>>>>>>>>>> Remember to test the icall instruction, as far as I remember it needs to be
>>>>>>>>>>>>>>>>>> tweaked because for now it doesn't save the registers between calls like
>>>>>>>>>>>>>>>>>> regular calls do.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Ok, memory instructions, most of the instructions you see
>>>>>>>>>>>>>>>>>> in that file that end up with a "w" are pseudo instructions (excluding
>>>>>>>>>>>>>>>>>> sbiw/adiw, movw or other real ones that work with 16bit data) meaning that
>>>>>>>>>>>>>>>>>> they dont get into the final assembly since they don't exist in the
>>>>>>>>>>>>>>>>>> instruction set. It's a trick to make llvm think they're supported
>>>>>>>>>>>>>>>>>> instructions that way, movws and the real 16bit insts get generated,
>>>>>>>>>>>>>>>>>> however these pseudo ops get expanded into 8bit ops, that's why you wont
>>>>>>>>>>>>>>>>>> never see those in the final assembly, and if you ever see one then it's a
>>>>>>>>>>>>>>>>>> bug. So if you do an and with 16bit types, llvm selects the andw pseudo op,
>>>>>>>>>>>>>>>>>> and a later pass expands it into two real 8bit and insts that are supported
>>>>>>>>>>>>>>>>>> by the avr inst set, dont' know if this makes things clear.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> 2012/5/31 Nicklas Bo Jensen <nbj...@gm...>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Checking for stack allocation and restore could be
>>>>>>>>>>>>>>>>>> something like:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> ; CHECK: in [[REG1:r[0-9]+]], 61
>>>>>>>>>>>>>>>>>> ; CHECK: in [[REG2:r[0-9]+]], 62
>>>>>>>>>>>>>>>>>> ; CHECK: sbiw [[REG2]]:[[REG1]], 4
>>>>>>>>>>>>>>>>>> ; CHECK: out 62, [[REG2]]
>>>>>>>>>>>>>>>>>> ; CHECK: out 61, [[REG1]]
>>>>>>>>>>>>>>>>>> ; ... other tests
>>>>>>>>>>>>>>>>>> ; CHECK: in [[REG1:r[0-9]+]], 61
>>>>>>>>>>>>>>>>>> ; CHECK: in [[REG2:r[0-9]+]], 62
>>>>>>>>>>>>>>>>>> ; CHECK: subi [[REG1]], 254
>>>>>>>>>>>>>>>>>> ; CHECK: sbci [[REG2]], 255
>>>>>>>>>>>>>>>>>> ; CHECK: out 62, [[REG2]]
>>>>>>>>>>>>>>>>>> ; CHECK: out 61, [[REG1]]
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Is it something like that you have in mind? Should
>>>>>>>>>>>>>>>>>> disabling and enabling interupts (cli+writeback of SREG) also be tested?
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Checkin
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
|
