From: Nicklas Bo J. <nbj...@gm...> - 2012-06-23 22:12:40
|
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 >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> > |