From: Borja F. <bor...@gm...> - 2012-07-07 18:20:58
|
Ok, I'm able to reproduce this crash with both clang and llc with -O0, with -O3 I'm getting an error because it cant select the mul instruction which is a known issue. I'm investigating now. Btw, is this part of a test you're writing? 2012/7/7 Nicklas Bo Jensen <nbj...@gm...> > Could you try to compile the following piece of C: > > typedef unsigned char uint8_t; > > extern uint8_t as[4]; > extern uint8_t bs[4]; > > void foo1(void) { > for (uint8_t i = 0; i < 4; i++) { > bs[i] = as[1]; > } > } > > I'm getting some wierd errors with llc(changing a bit dependent on > -O0/-O3) like: > llvm/include/llvm/CodeGen/SelectionDAGNodes.h:534: const llvm::SDValue > &llvm::SDNode::getOperand(unsigned int) const: Assertion `Num < NumOperands > && "Invalid child # of SDNode!"' failed. > > > On Sat, Jul 7, 2012 at 6:44 PM, Borja Ferrer <bor...@gm...>wrote: > >> No problem, I just wanted to know what was the status, no need to rush it >> :) >> >> >> 2012/7/7 Nicklas Bo Jensen <nbj...@gm...> >> >>> 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 >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> > |