From: Borja F. <bor...@gm...> - 2012-06-23 22:45:42
|
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 >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> > |