From: Borja F. <bor...@gm...> - 2012-07-07 16:21:39
|
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 >>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> > |