From: Borja F. <bor...@gm...> - 2012-07-09 12:13:45
|
Thanks, I see what's going on. I will fix it later today. 2012/7/9 Nicklas Bo Jensen <nbj...@gm...> > For example: > > char foo(char *arr) > { > return arr[63]; > } > > Should generate ldd r24, Y/Z+63, but generates: > > movw r31:r30, r25:r24 > adiw r31:r30, 63 > ld r24, Z > ret > > On Sun, Jul 8, 2012 at 3:25 PM, Borja Ferrer <bor...@gm...>wrote: > >> can you give me an example where this is happening? >> >> 2012/7/8 Nicklas Bo Jensen <nbj...@gm...> >> >>> Thanks again. >>> >>> I've noticed that in the instruction: LDD Rd, Y/Z+q, the range for >>> q generated is 0<=q<63 , whereas the instruction set also includes 63, i.e. >>> 0<=q<=63. >>> >>> On Sat, Jul 7, 2012 at 10:33 PM, Borja Ferrer <bor...@gm...>wrote: >>> >>>> postinc/predec is fragile yes. Back when i was coding that part I used >>>> to test it with a for loop, something like: >>>> >>>> for (i=0; i<x; i++) {a++ = b;} >>>> being a a ptr. >>>> OR >>>> *a++ = 1; >>>> *a++=2; >>>> etc if you dont want to use a loop, better to make the test shorter. >>>> >>>> There are many cases where a postinc/predec should be generated but it >>>> doesnt happen, so that will have to be optimized later, but for now adding >>>> tests for very basic stuff is fine. >>>> >>>> >>>> 2012/7/7 Nicklas Bo Jensen <nbj...@gm...> >>>> >>>>> That sounds good for std/ldd. Regarding postinc/predec I think the >>>>> generated code is too fragile to test good, but I will look further into if >>>>> it's possible to find a better test. >>>>> >>>>> On Sat, Jul 7, 2012 at 9:59 PM, Borja Ferrer <bor...@gm...>wrote: >>>>> >>>>>> Do only one store/load per test function, that way you won't get >>>>>> predec/postinc instructions. What I was thinking of is: test x[0]=value; >>>>>> for every data type, then do the same thing for load. Then test displaced >>>>>> loads/stores with x[44]=value, you cover there std/ldd. And then finally >>>>>> postinc/predec. >>>>>> >>>>>> In fact from the piece of code you pasted, the optimal thing i guess >>>>>> would be doing st X+, reg instead of wasting reg Z, so it could be >>>>>> improved, but i'm not interested now in optimum code. >>>>>> >>>>>> >>>>>> 2012/7/7 Nicklas Bo Jensen <nbj...@gm...> >>>>>> >>>>>>> Of course, but I'm also thinking pre/post dec/inc. >>>>>>> >>>>>>> For example: >>>>>>> >>>>>>> void foo(char *arr) >>>>>>> { >>>>>>> arr[0] = 1; >>>>>>> arr[1] = 2; >>>>>>> arr[2] = 3; >>>>>>> arr[3] = 4; >>>>>>> } >>>>>>> >>>>>>> generates: >>>>>>> movw r31:r30, r25:r24 >>>>>>> ldi r24, 1 >>>>>>> movw r27:r26, r31:r30 >>>>>>> st X+, r24 >>>>>>> ldi r24, 2 >>>>>>> st X, r24 >>>>>>> ldi r24, 3 >>>>>>> std Z+2, r24 >>>>>>> ldi r24, 4 >>>>>>> std Z+3, r24 >>>>>>> ret >>>>>>> >>>>>>> First two stores using st and last two using std. The performance of >>>>>>> st vs std is equivalent right? >>>>>>> >>>>>>> Testing these is going to be hard if i'ts not deterministic which >>>>>>> instruction is used when. >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Sat, Jul 7, 2012 at 9:43 PM, Borja Ferrer <bor...@gm...>wrote: >>>>>>> >>>>>>>> This : >>>>>>>> void test44(int *x) >>>>>>>> { >>>>>>>> x[0]=2; >>>>>>>> } >>>>>>>> gives: >>>>>>>> >>>>>>>> movw r31:r30, r25:r24 >>>>>>>> ldi r24, 2 >>>>>>>> ldi r25, 0 >>>>>>>> st Z, r24 >>>>>>>> std Z+1, r25 >>>>>>>> ret >>>>>>>> >>>>>>>> >>>>>>>> 2012/7/7 Nicklas Bo Jensen <nbj...@gm...> >>>>>>>> >>>>>>>>> Ah thanks! I cannot seem to get ld/st though? >>>>>>>>> >>>>>>>>> >>>>>>>>> On Sat, Jul 7, 2012 at 9:19 PM, Borja Ferrer < >>>>>>>>> bor...@gm...> wrote: >>>>>>>>> >>>>>>>>>> Ok great. If you want that piece of code really working, you can >>>>>>>>>> tell llvm to expand the mul nodes, you can do it in the AVRTargetLowering >>>>>>>>>> constructor with the setOperationAction. >>>>>>>>>> >>>>>>>>>> To generate those instructions it's quite easy, just use a >>>>>>>>>> pointer as an incoming argument in a function and store or load values from >>>>>>>>>> there, then use x[3] and such to get offsets in relation to a base address >>>>>>>>>> for std/ldd. It's important to test offsets in range (up to 63) and bigger >>>>>>>>>> ones which need materialization each time. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> 2012/7/7 Nicklas Bo Jensen <nbj...@gm...> >>>>>>>>>> >>>>>>>>>>> Nice, that was a quick fix :) I'ts working now with -O0, with >>>>>>>>>>> -O3 I'm getting the mul error. >>>>>>>>>>> >>>>>>>>>>> I mean st/std/ld/ldd. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Sat, Jul 7, 2012 at 8:51 PM, Borja Ferrer < >>>>>>>>>>> bor...@gm...> wrote: >>>>>>>>>>> >>>>>>>>>>>> Ok fixed, thanks for the report! If you now get the issue about >>>>>>>>>>>> the mul, it's a known thing. >>>>>>>>>>>> >>>>>>>>>>>> What do you mean by indirect mem accesses? >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> 2012/7/7 Nicklas Bo Jensen <nbj...@gm...> >>>>>>>>>>>> >>>>>>>>>>>>> Great, not just my setup then :) >>>>>>>>>>>>> >>>>>>>>>>>>> No, it's not part of any tests. I'm just trying out different >>>>>>>>>>>>> things. I'm having some trouble generating indirect memory accesses, any >>>>>>>>>>>>> tips? >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> On Sat, Jul 7, 2012 at 8:20 PM, Borja Ferrer < >>>>>>>>>>>>> bor...@gm...> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> 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 >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> > |