Menu
▾
▴
Re: [avr-llvm-devel] Regression tests
|
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
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
|
