Re: [avr-llvm-devel] Regression tests

[Borja F. <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
>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
>