Re: [avr-llvm-devel] Regression tests

[Nicklas Bo J. <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
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>