From: Borja F. <bor...@gm...> - 2012-06-23 23:38:21
|
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 >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> > |