Menu
▾
▴
Re: [avr-llvm-devel] Regression tests
|
From: Nicklas Bo J. <nbj...@gm...> - 2012-07-07 17:42:37
|
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
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
|
