Menu
▾
▴
avr-llvm-devel
|
From: Borja F. <bor...@gm...> - 2012-06-01 17:05:05
|
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? > > Checking for the push and pops is going to be very fragile I think. > > Other backends have less thorough tests, but a bit alike, e.g. MBlaze in > test/CodeGen/MBlaze/cc.ll. Here they do tests that registers are passed and > returned in the correct registers or passed on the stack if needed. > > I've come quite far with memory operations, and have noticed the > operations stsw and lddw in AVRInstrInfo.td, however I cannot find these in > the Atmel AVR Instruction Set and cannot make llvm generate them. Could you > give me a hint to why this is? > > On Thu, May 31, 2012 at 1:24 PM, Borja Ferrer <bor...@gm...>wrote: > >> They look very good. Yes, the order of loads is an issue, I don't know >> how fragile this can get in case there are future changes that could alter >> the order. Also it could be nice to check for stack allocation and restore, >> have you checked if it's possible to do it? Add a TODO in the file for args >> and for calls for testing for structs, this is something unimplemented that >> needs to gets covered, I don't know yet any conventions for those. >> >> One last thing, what do other backends do to cover these tests? Anything >> similar to what we do? >> >> >> >> 2012/5/30 Nicklas Bo Jensen <nbj...@gm...> >> >>> Thanks for your explanation, it's clear. >>> >>> After a few days on holiday, I've worked on testing the calling part of >>> the calling convention. Please see the attached file. >>> >>> I'm testing only passing parameters in the registers and on the stack. >>> I'ts a bit hard to test in a good way, as for example the order of loads >>> might be different, breaking the tests without having invalid code. I've >>> tried to only tests the essentials in each case. Any comments or ideas for >>> further tests? >>> >>> >>> On Sat, May 26, 2012 at 6:28 PM, Borja Ferrer <bor...@gm...>wrote: >>> >>>> About point 2: >>>> As the calling convention states, arguments that don't fit in registers >>>> r25 down to r8 are passed through the stack, that's why the last argument >>>> is being passed this way. If registers Y or Z are used depends on a number >>>> of conditions that are bit hard to explain, it depends if there are spilled >>>> registers, allocas, and incoming arguments in the stack. In this particular >>>> case, Y is ALWAYS used when reading incoming arguments from the stack, Z >>>> COULD be used only when passing arguments into a function call. So the >>>> difference is that Y is always used to READ incoming arguments from the >>>> stack, and Z or Y can be used to WRITE arguments on the stack as the >>>> preparation of the function call. >>>> >>>> About the code you attached I find it's correct. It's a void function >>>> so why care on computing an add instruction where its result is unused, >>>> also the return value of the called function is unused as well since the >>>> add instruction is useless. That's why you see r25:r24 being overwritten by >>>> the stack pointer so it can be restored to its original state when the >>>> function ends. >>>> This is all a bit tricky so if you dont understand feel free to ask, >>>> it's important to get it right so you can write good tests. >>>> >>>> 2012/5/25 Nicklas Bo Jensen <nbj...@gm...> >>>> >>>>> 1) The i8 @return8_reg test is fully commented out, can it be removed? >>>>>> I don't understand what you want to test here. >>>>>> >>>>> Can be removed, was not supposed to be there :) >>>>> >>>>> >>>>>> 2) In return64_arg2 the backend will always use Y to read arguments >>>>>> from the stack, so there's no need to check for Z. >>>>>> >>>>> Ah ok, is there any rule for putting them on the stack? Here it can be >>>>> both Y and Z right? >>>>> >>>>> >>>>>> About your calling tests, I can't see them xD >>>>> >>>>> Attached :) >>>>> >>>> >>>> >>> >> > |
|
From: Nicklas Bo J. <nbj...@gm...> - 2012-06-03 20:46:03
|
../build/Debug+Asserts/bin/llc icall.ll -march=avr -o icall.s llc: /home/nicklas/install/avr-llvm/llvm/lib/Target/AVR/AVRISelDAGToDAG.cpp:287: virtual llvm::SDNode *<anonymous namespace>::AVRDAGToDAGISel::Select(llvm::SDNode *): Assertion `Callee.getOpcode() == ISD::CopyFromReg && "Unknown callee source"' failed. 0 llc 0x000000000138ff8e 1 llc 0x000000000139048a 2 libpthread.so.0 0x00007f4b19bc0cb0 3 libc.so.6 0x00007f4b18e16445 gsignal + 53 4 libc.so.6 0x00007f4b18e19bab abort + 379 5 libc.so.6 0x00007f4b18e0f10e 6 libc.so.6 0x00007f4b18e0f1b2 7 llc 0x0000000000675f8d 8 llc 0x0000000000c852cb llvm::SelectionDAGISel::DoInstructionSelection() + 555 9 llc 0x0000000000c84979 llvm::SelectionDAGISel::CodeGenAndEmitDAG() + 3161 10 llc 0x0000000000c83d0d llvm::SelectionDAGISel::SelectBasicBlock(llvm::ilist_iterator<llvm::Instruction const>, llvm::ilist_iterator<llvm::Instruction const>, bool&) + 253 11 llc 0x0000000000c83b82 llvm::SelectionDAGISel::SelectAllBasicBlocks(llvm::Function const&) + 3122 12 llc 0x0000000000c8233a llvm::SelectionDAGISel::runOnMachineFunction(llvm::MachineFunction&) + 826 13 llc 0x0000000000e012fe llvm::MachineFunctionPass::runOnFunction(llvm::Function&) + 110 14 llc 0x0000000001311c86 llvm::FPPassManager::runOnFunction(llvm::Function&) + 438 15 llc 0x0000000001311fad llvm::FPPassManager::runOnModule(llvm::Module&) + 125 16 llc 0x00000000013121fd llvm::MPPassManager::runOnModule(llvm::Module&) + 493 17 llc 0x00000000013128a7 llvm::PassManagerImpl::run(llvm::Module&) + 167 18 llc 0x0000000001312a91 llvm::PassManager::run(llvm::Module&) + 33 19 llc 0x00000000005e11c7 main + 3831 20 libc.so.6 0x00007f4b18e0176d __libc_start_main + 237 21 llc 0x00000000005dedf9 Stack dump: 0. Program arguments: ../build/Debug+Asserts/bin/llc icall.ll -march=avr -o icall.s 1. Running pass 'Function Pass Manager' on module 'icall.ll'. 2. Running pass 'AVR DAG->DAG Instruction Selection' on function '@foobar' Aborted (core dumped) |
|
From: Borja F. <bor...@gm...> - 2012-06-04 11:16:43
|
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? >>> >>> Checking for the push and pops is going to be very fragile I think. >>> >>> Other backends have less thorough tests, but a bit alike, e.g. MBlaze in >>> test/CodeGen/MBlaze/cc.ll. Here they do tests that registers are passed and >>> returned in the correct registers or passed on the stack if needed. >>> >>> I've come quite far with memory operations, and have noticed the >>> operations stsw and lddw in AVRInstrInfo.td, however I cannot find these in >>> the Atmel AVR Instruction Set and cannot make llvm generate them. Could you >>> give me a hint to why this is? >>> >>> On Thu, May 31, 2012 at 1:24 PM, Borja Ferrer <bor...@gm...>wrote: >>> >>>> They look very good. Yes, the order of loads is an issue, I don't know >>>> how fragile this can get in case there are future changes that could alter >>>> the order. Also it could be nice to check for stack allocation and restore, >>>> have you checked if it's possible to do it? Add a TODO in the file for args >>>> and for calls for testing for structs, this is something unimplemented that >>>> needs to gets covered, I don't know yet any conventions for those. >>>> >>>> One last thing, what do other backends do to cover these tests? >>>> Anything similar to what we do? >>>> >>>> >>>> >>>> 2012/5/30 Nicklas Bo Jensen <nbj...@gm...> >>>> >>>>> Thanks for your explanation, it's clear. >>>>> >>>>> After a few days on holiday, I've worked on testing the calling part >>>>> of the calling convention. Please see the attached file. >>>>> >>>>> I'm testing only passing parameters in the registers and on the stack. >>>>> I'ts a bit hard to test in a good way, as for example the order of loads >>>>> might be different, breaking the tests without having invalid code. I've >>>>> tried to only tests the essentials in each case. Any comments or ideas for >>>>> further tests? >>>>> >>>>> >>>>> On Sat, May 26, 2012 at 6:28 PM, Borja Ferrer <bor...@gm...>wrote: >>>>> >>>>>> About point 2: >>>>>> As the calling convention states, arguments that don't fit in >>>>>> registers r25 down to r8 are passed through the stack, that's why the last >>>>>> argument is being passed this way. If registers Y or Z are used depends on >>>>>> a number of conditions that are bit hard to explain, it depends if there >>>>>> are spilled registers, allocas, and incoming arguments in the stack. In >>>>>> this particular case, Y is ALWAYS used when reading incoming arguments from >>>>>> the stack, Z COULD be used only when passing arguments into a function >>>>>> call. So the difference is that Y is always used to READ incoming arguments >>>>>> from the stack, and Z or Y can be used to WRITE arguments on the stack as >>>>>> the preparation of the function call. >>>>>> >>>>>> About the code you attached I find it's correct. It's a void function >>>>>> so why care on computing an add instruction where its result is unused, >>>>>> also the return value of the called function is unused as well since the >>>>>> add instruction is useless. That's why you see r25:r24 being overwritten by >>>>>> the stack pointer so it can be restored to its original state when the >>>>>> function ends. >>>>>> This is all a bit tricky so if you dont understand feel free to ask, >>>>>> it's important to get it right so you can write good tests. >>>>>> >>>>>> 2012/5/25 Nicklas Bo Jensen <nbj...@gm...> >>>>>> >>>>>>> 1) The i8 @return8_reg test is fully commented out, can it be >>>>>>>> removed? I don't understand what you want to test here. >>>>>>>> >>>>>>> Can be removed, was not supposed to be there :) >>>>>>> >>>>>>> >>>>>>>> 2) In return64_arg2 the backend will always use Y to read arguments >>>>>>>> from the stack, so there's no need to check for Z. >>>>>>>> >>>>>>> Ah ok, is there any rule for putting them on the stack? Here it can >>>>>>> be both Y and Z right? >>>>>>> >>>>>>> >>>>>>>> About your calling tests, I can't see them xD >>>>>>> >>>>>>> Attached :) >>>>>>> >>>>>> >>>>>> >>>>> >>>> >>> >> > |
|
From: Borja F. <bor...@gm...> - 2012-06-05 19:53:42
|
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? >>>> >>>> Checking for the push and pops is going to be very fragile I think. >>>> >>>> Other backends have less thorough tests, but a bit alike, e.g. MBlaze >>>> in test/CodeGen/MBlaze/cc.ll. Here they do tests that registers are passed >>>> and returned in the correct registers or passed on the stack if needed. >>>> >>>> I've come quite far with memory operations, and have noticed the >>>> operations stsw and lddw in AVRInstrInfo.td, however I cannot find these in >>>> the Atmel AVR Instruction Set and cannot make llvm generate them. Could you >>>> give me a hint to why this is? >>>> >>>> On Thu, May 31, 2012 at 1:24 PM, Borja Ferrer <bor...@gm...>wrote: >>>> >>>>> They look very good. Yes, the order of loads is an issue, I don't know >>>>> how fragile this can get in case there are future changes that could alter >>>>> the order. Also it could be nice to check for stack allocation and restore, >>>>> have you checked if it's possible to do it? Add a TODO in the file for args >>>>> and for calls for testing for structs, this is something unimplemented that >>>>> needs to gets covered, I don't know yet any conventions for those. >>>>> >>>>> One last thing, what do other backends do to cover these tests? >>>>> Anything similar to what we do? >>>>> >>>>> >>>>> >>>>> 2012/5/30 Nicklas Bo Jensen <nbj...@gm...> >>>>> >>>>>> Thanks for your explanation, it's clear. >>>>>> >>>>>> After a few days on holiday, I've worked on testing the calling part >>>>>> of the calling convention. Please see the attached file. >>>>>> >>>>>> I'm testing only passing parameters in the registers and on the >>>>>> stack. I'ts a bit hard to test in a good way, as for example the order of >>>>>> loads might be different, breaking the tests without having invalid code. >>>>>> I've tried to only tests the essentials in each case. Any comments or ideas >>>>>> for further tests? >>>>>> >>>>>> >>>>>> On Sat, May 26, 2012 at 6:28 PM, Borja Ferrer <bor...@gm...>wrote: >>>>>> >>>>>>> About point 2: >>>>>>> As the calling convention states, arguments that don't fit in >>>>>>> registers r25 down to r8 are passed through the stack, that's why the last >>>>>>> argument is being passed this way. If registers Y or Z are used depends on >>>>>>> a number of conditions that are bit hard to explain, it depends if there >>>>>>> are spilled registers, allocas, and incoming arguments in the stack. In >>>>>>> this particular case, Y is ALWAYS used when reading incoming arguments from >>>>>>> the stack, Z COULD be used only when passing arguments into a function >>>>>>> call. So the difference is that Y is always used to READ incoming arguments >>>>>>> from the stack, and Z or Y can be used to WRITE arguments on the stack as >>>>>>> the preparation of the function call. >>>>>>> >>>>>>> About the code you attached I find it's correct. It's a void >>>>>>> function so why care on computing an add instruction where its result is >>>>>>> unused, also the return value of the called function is unused as well >>>>>>> since the add instruction is useless. That's why you see r25:r24 being >>>>>>> overwritten by the stack pointer so it can be restored to its original >>>>>>> state when the function ends. >>>>>>> This is all a bit tricky so if you dont understand feel free to ask, >>>>>>> it's important to get it right so you can write good tests. >>>>>>> >>>>>>> 2012/5/25 Nicklas Bo Jensen <nbj...@gm...> >>>>>>> >>>>>>>> 1) The i8 @return8_reg test is fully commented out, can it be >>>>>>>>> removed? I don't understand what you want to test here. >>>>>>>>> >>>>>>>> Can be removed, was not supposed to be there :) >>>>>>>> >>>>>>>> >>>>>>>>> 2) In return64_arg2 the backend will always use Y to read >>>>>>>>> arguments from the stack, so there's no need to check for Z. >>>>>>>>> >>>>>>>> Ah ok, is there any rule for putting them on the stack? Here it can >>>>>>>> be both Y and Z right? >>>>>>>> >>>>>>>> >>>>>>>>> About your calling tests, I can't see them xD >>>>>>>> >>>>>>>> Attached :) >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> > |
|
From: Borja F. <bor...@gm...> - 2012-06-06 00:46:07
|
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? >>>>> >>>>> Checking for the push and pops is going to be very fragile I think. >>>>> >>>>> Other backends have less thorough tests, but a bit alike, e.g. MBlaze >>>>> in test/CodeGen/MBlaze/cc.ll. Here they do tests that registers are passed >>>>> and returned in the correct registers or passed on the stack if needed. >>>>> >>>>> I've come quite far with memory operations, and have noticed the >>>>> operations stsw and lddw in AVRInstrInfo.td, however I cannot find these in >>>>> the Atmel AVR Instruction Set and cannot make llvm generate them. Could you >>>>> give me a hint to why this is? >>>>> >>>>> On Thu, May 31, 2012 at 1:24 PM, Borja Ferrer <bor...@gm...>wrote: >>>>> >>>>>> They look very good. Yes, the order of loads is an issue, I don't >>>>>> know how fragile this can get in case there are future changes that could >>>>>> alter the order. Also it could be nice to check for stack allocation and >>>>>> restore, have you checked if it's possible to do it? Add a TODO in the file >>>>>> for args and for calls for testing for structs, this is something >>>>>> unimplemented that needs to gets covered, I don't know yet any conventions >>>>>> for those. >>>>>> >>>>>> One last thing, what do other backends do to cover these tests? >>>>>> Anything similar to what we do? >>>>>> >>>>>> >>>>>> >>>>>> 2012/5/30 Nicklas Bo Jensen <nbj...@gm...> >>>>>> >>>>>>> Thanks for your explanation, it's clear. >>>>>>> >>>>>>> After a few days on holiday, I've worked on testing the calling part >>>>>>> of the calling convention. Please see the attached file. >>>>>>> >>>>>>> I'm testing only passing parameters in the registers and on the >>>>>>> stack. I'ts a bit hard to test in a good way, as for example the order of >>>>>>> loads might be different, breaking the tests without having invalid code. >>>>>>> I've tried to only tests the essentials in each case. Any comments or ideas >>>>>>> for further tests? >>>>>>> >>>>>>> >>>>>>> On Sat, May 26, 2012 at 6:28 PM, Borja Ferrer <bor...@gm... >>>>>>> > wrote: >>>>>>> >>>>>>>> About point 2: >>>>>>>> As the calling convention states, arguments that don't fit in >>>>>>>> registers r25 down to r8 are passed through the stack, that's why the last >>>>>>>> argument is being passed this way. If registers Y or Z are used depends on >>>>>>>> a number of conditions that are bit hard to explain, it depends if there >>>>>>>> are spilled registers, allocas, and incoming arguments in the stack. In >>>>>>>> this particular case, Y is ALWAYS used when reading incoming arguments from >>>>>>>> the stack, Z COULD be used only when passing arguments into a function >>>>>>>> call. So the difference is that Y is always used to READ incoming arguments >>>>>>>> from the stack, and Z or Y can be used to WRITE arguments on the stack as >>>>>>>> the preparation of the function call. >>>>>>>> >>>>>>>> About the code you attached I find it's correct. It's a void >>>>>>>> function so why care on computing an add instruction where its result is >>>>>>>> unused, also the return value of the called function is unused as well >>>>>>>> since the add instruction is useless. That's why you see r25:r24 being >>>>>>>> overwritten by the stack pointer so it can be restored to its original >>>>>>>> state when the function ends. >>>>>>>> This is all a bit tricky so if you dont understand feel free to >>>>>>>> ask, it's important to get it right so you can write good tests. >>>>>>>> >>>>>>>> 2012/5/25 Nicklas Bo Jensen <nbj...@gm...> >>>>>>>> >>>>>>>>> 1) The i8 @return8_reg test is fully commented out, can it be >>>>>>>>>> removed? I don't understand what you want to test here. >>>>>>>>>> >>>>>>>>> Can be removed, was not supposed to be there :) >>>>>>>>> >>>>>>>>> >>>>>>>>>> 2) In return64_arg2 the backend will always use Y to read >>>>>>>>>> arguments from the stack, so there's no need to check for Z. >>>>>>>>>> >>>>>>>>> Ah ok, is there any rule for putting them on the stack? Here it >>>>>>>>> can be both Y and Z right? >>>>>>>>> >>>>>>>>> >>>>>>>>>> About your calling tests, I can't see them xD >>>>>>>>> >>>>>>>>> Attached :) >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> > |
|
From: Borja F. <bor...@gm...> - 2012-06-17 18:11:30
|
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 > > |
|
From: Borja F. <bor...@gm...> - 2012-06-17 23:10:38
|
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 >> >> |
|
From: Nicklas Bo J. <nbj...@gm...> - 2012-06-18 07:58:08
|
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 >>> >>> > |
|
From: Borja F. <bor...@gm...> - 2012-06-18 11:46:52
|
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 >>>> >>>> >> > |
|
From: Nicklas Bo J. <nbj...@gm...> - 2012-06-18 15:48:30
|
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
>>>>>
>>>>>
>>>
>>
>
|
|
From: Borja F. <bor...@gm...> - 2012-06-18 17:20:42
|
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 >>>>>> >>>>>> >>>> >>> >> > |
|
From: Borja F. <bor...@gm...> - 2012-06-19 00:42:44
|
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 >>>>>>> >>>>>>> >>>>> >>>> >>> >> > |
|
From: Borja F. <bor...@gm...> - 2012-06-19 11:48:09
|
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 >>>>>>>> >>>>>>>> >>>>>> >>>>> >>>> >>> >> > |
|
From: Nicklas Bo J. <nbj...@gm...> - 2012-06-23 16:59:16
|
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 >>>>>>>>> >>>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> > |
|
From: Borja F. <bor...@gm...> - 2012-06-23 18:15:13
|
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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
|
|
From: Nicklas Bo J. <nbj...@gm...> - 2012-06-23 22:12:40
|
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
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
|
|
From: Borja F. <bor...@gm...> - 2012-06-23 22:45:42
|
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
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
|
|
From: Nicklas Bo J. <nbj...@gm...> - 2012-06-23 23:00:53
|
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
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
|
|
From: Borja F. <bor...@gm...> - 2012-06-23 23:38:21
|
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
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
|
|
From: Borja F. <bor...@gm...> - 2012-06-24 11:59:06
|
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
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
|
|
From: Nicklas Bo J. <nbj...@gm...> - 2012-06-24 12:49:03
|
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
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
|
|
From: Borja F. <bor...@gm...> - 2012-06-24 18:16:23
|
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
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
|
|
From: Nicklas Bo J. <nbj...@gm...> - 2012-06-24 19:40:15
|
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
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
|
|
From: Borja F. <bor...@gm...> - 2012-06-24 19:58:05
|
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
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
|
|
From: Nicklas Bo J. <nbj...@gm...> - 2012-06-24 20:23:12
|
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
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
|
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X