Re: [avr-llvm-devel] Regression tests

[Borja F. <bor...@gm...>]

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