From: Nicklas Bo J. <nbj...@gm...> - 2012-07-07 16:26:27
|
No, sorry, I've been busy lately. I will take it up again in the next days. On Sat, Jul 7, 2012 at 6:21 PM, Borja Ferrer <bor...@gm...> wrote: > Nicklas, are you still working in those memory tests? > > 2012/6/26 Borja Ferrer <bor...@gm...> > >> What about directmem.ll to keep it a bit shorter? >> >> Btw, the ldd/std tests and such keep them in two separate files, like >> load.ll and store.ll since they're going to get quite big on their own. >> >> >> 2012/6/25 Nicklas Bo Jensen <nbj...@gm...> >> >>> I've commited the tests after going over your comments. >>> >>> The file should probably still be renamed though, we could call it >>> loadstoredirect.ll? Then tests for st/std/ld/ldd could be in a file called >>> loadstoreindirect.ll? >>> >>> On Mon, Jun 25, 2012 at 1:43 PM, Borja Ferrer <bor...@gm...>wrote: >>> >>>> Ohhh well, happy to hear it's now fixed!! Now we can move to something >>>> else. >>>> >>>> Memory test comments: >>>> First of all since you're testing lds/sts i would rename the file to >>>> something else, mem is too general, but i can't think of a good name now, >>>> suggestions are welcome. >>>> 1) I would remove the allocX_local tests, I'm writing alloca tests >>>> somewhere else, and they're unrelated to sts/lds. >>>> 2) Change all imm values to something that doesn't contain zeros, as we >>>> mentioned previously. This will change some tests a bit, but it's trivial >>>> to implement. >>>> >>>> Apart of these comments, the tests look great. >>>> >>>> Once this gets done we can then move to other memory tests, mainly >>>> testing st/ld and std/ldd instructions. Which require testing displacements >>>> of small and big offsets, postinc and predec operations, and probably other >>>> stuff that I'll have to look up. >>>> >>>> >>>> >>>> 2012/6/25 Nicklas Bo Jensen <nbj...@gm...> >>>> >>>>> OK, it's working now. I was missing MyFlags.Flags.setSplitPiece(j) in >>>>> line 6450 in selectiondagbuilder.cpp. Not sure how it 'unpatched' itself. >>>>> >>>>> Sorry for the trouble. >>>>> >>>>> >>>>> On Mon, Jun 25, 2012 at 12:40 AM, Borja Ferrer <bor...@gm...>wrote: >>>>> >>>>>> IIRC CLI.Outs.size() should be 4 because you have 4 argument pieces, >>>>>> however ParseExternFuncCallArgs should have filled the Args array to have >>>>>> only 1 element of size 4 because the function in reality has only 1 >>>>>> argument. The clue here is investigating why is this failing for you and >>>>>> not for me. I guess you're jumping to the AnalyzeArguments from line 932, >>>>>> the one in the else clause. >>>>>> >>>>>> Add printf's to see what getSplitPiece returns for each iteration, in >>>>>> theory assuming this wrong behaviour this function should always return 0 >>>>>> so when doing Out.push_back(Size), Size is always 1. >>>>>> The splitpiece stuff is set by MyFlags.Flags.setSplitPiece(j) in line >>>>>> 6450 in selectiondagbuilder.cpp >>>>>> >>>>>> >>>>>> 2012/6/24 Nicklas Bo Jensen <nbj...@gm...> >>>>>> >>>>>>> Yes, I have four elements with value one filled >>>>>>> by ParseExternFuncCallArgs. >>>>>>> >>>>>>> I can't tell if ParseExternFuncCallArgs is buggy, but it seems as >>>>>>> though it is given that there are four arguments. For example >>>>>>> CLI.Outs.size() evaluates to 4 in line 893. Is that correctly understood? >>>>>>> >>>>>>> On Sun, Jun 24, 2012 at 9:57 PM, Borja Ferrer <bor...@gm... >>>>>>> > wrote: >>>>>>> >>>>>>>> Indeed, if you mean the Size var set in line 750 it should be 4. >>>>>>>> The Args vector should only contain 1 element because you only have 1 >>>>>>>> argument in the function and the size should be set to 4. This is the >>>>>>>> reason of why the arguments aren't reversed. In your case I guess you have >>>>>>>> 4 elements of size 1. This vector is filled by ParseExternFuncCallArgs() or >>>>>>>> by ParseFunctionArgs() depending on the call type, so the problem is there, >>>>>>>> in this case ParseExternFuncCallArgs is doing the work right? So this is >>>>>>>> the function you should debug. >>>>>>>> >>>>>>>> >>>>>>>> 2012/6/24 Nicklas Bo Jensen <nbj...@gm...> >>>>>>>> >>>>>>>>> You're right, it's the -O3 flag. >>>>>>>>> >>>>>>>>> I'm stepping through the code with the debugger right now. The >>>>>>>>> indirect call does trigger AnalyzeArguments to reach line 768 >>>>>>>>> (std::reverse). >>>>>>>>> >>>>>>>>> There is one thing puzzling me a bit, the argument size is >>>>>>>>> calculated to be 1, i.e. Args[0] is 1, but shouldn't a 64bit argument >>>>>>>>> evaluate to 4 in size? >>>>>>>>> >>>>>>>>> >>>>>>>>> On Sun, Jun 24, 2012 at 8:16 PM, Borja Ferrer < >>>>>>>>> bor...@gm...> wrote: >>>>>>>>> >>>>>>>>>> With that command line I get the same code as you (now i get the >>>>>>>>>> spill i wasn't getting before), however i still get correct code: >>>>>>>>>> movw r31:r30, r25:r24 >>>>>>>>>> >>>>>>>>>> std Y+1, r30 >>>>>>>>>> std Y+2, r31 >>>>>>>>>> ldi r18, 136 >>>>>>>>>> ldi r19, 153 >>>>>>>>>> ldi r20, 255 >>>>>>>>>> ldi r21, 238 >>>>>>>>>> ldi r22, 221 >>>>>>>>>> ldi r23, 204 >>>>>>>>>> ldi r24, 187 >>>>>>>>>> ldi r25, 170 >>>>>>>>>> icall >>>>>>>>>> >>>>>>>>>> This is my usual command line: >>>>>>>>>> >>>>>>>>>> clang -O3 -emit-llvm test.c -S -o test.bc -ccc-host-triple >>>>>>>>>> avr-generic-generic -ccc-clang-archs avr >>>>>>>>>> llc -O3 -march=avr test.bc -pre-RA-sched=default >>>>>>>>>> -regalloc=avrgreedy -debug-only=isel -o test.S -print-isel-input >>>>>>>>>> -print-after-all -stats 2> dump >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> 2012/6/24 Nicklas Bo Jensen <nbj...@gm...> >>>>>>>>>> >>>>>>>>>>> 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 >>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> > |