Menu
▾
▴
Re: [avr-llvm-devel] Regression tests
|
From: Nicklas Bo J. <nbj...@gm...> - 2012-06-24 12:49:03
|
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
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
|
