Re: [avr-llvm-devel] Regression tests

[Nicklas Bo J. <nbj...@gm...>]

You're right, it's the -O3 flag.

I'm stepping through the code with the debugger right now. The indirect
call does trigger AnalyzeArguments to reach line 768 (std::reverse).

There is one thing puzzling me a bit, the argument size is calculated to be
1, i.e. Args[0] is 1, but shouldn't a 64bit argument evaluate to 4 in size?


On Sun, Jun 24, 2012 at 8:16 PM, Borja Ferrer <bor...@gm...> wrote:

> 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
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>