From: Nicklas Bo J. <nbj...@gm...> - 2012-06-24 12:49:03
|
All patches should be installed and it has worked earlier when I wrote that test. I also have the clang patches, but I don't think it has anything to do with those. To compile icall.c, I would for example do: ../build/Debug+Asserts/bin/clang icall.c -S -emit-llvm -o icall.ll to view the llvm ir and to get assembler: ../build/Debug+Asserts/bin/llc icall.ll -march=avr -O3 -o icall.s I have that exact code as you describe, I will printf debug it later tonight. Thanks! On Sun, Jun 24, 2012 at 1:58 PM, Borja Ferrer <bor...@gm...> wrote: > Ok the problem I see is that you're not getting the arguments pieces > reversed. Can you check the code that performs this? This is done inside > AnalyzeArguments, exactly in line 768 done by std::reverse. Also check that > this function is being called in line 932. That should give you enough info > to debug it, use fprintfs to print out debugging info to help you otherwise > it's going to be hard to debug. > > I'm assuming you have all patches installed, including clang patches right? > Paste the command line you're using to call clang because I'm not getting > std Y+1, r30 > std Y+2, r31 > or any stack allocation as you get. > > > 2012/6/24 Nicklas Bo Jensen <nbj...@gm...> > >> Still gives the wrong code: >> ldi r24, 136 >> ldi r25, 153 >> ldi r22, 255 >> ldi r23, 238 >> ldi r20, 221 >> ldi r21, 204 >> ldi r18, 187 >> ldi r19, 170 >> >> I have also attached the output. >> >> On Sun, Jun 24, 2012 at 1:38 AM, Borja Ferrer <bor...@gm...>wrote: >> >>> 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 >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> > |