From: Stepan D. <stp...@na...> - 2013-07-10 19:15:26
|
Hi Borja, > About the patch: > Is all the code in AVRISelLowering.cpp necesarry? I find it to be a very > "manual" way of doing this optimization, it feels like not being very > general, so please confirm me that this the only way of doing it. Im > asking this in case in AVRISelDAGToDAG.cpp you could add some code to > match this sort of DAGs with the existing adressing mode matching > functions to avoid the code in the other file. Maybe look at what other > backends do? I've compared it with ARM backend today (the only one that does something serious with InlineAsm node). Well all stuff I did in LowerINLINEASM is implemented in SelectInlineAsm, in ARMISelDAGToDAG. Well that's true, its not a lowering. I closer to the final stage, when you just correct the things and select proper instructions. So I did the same: I've moved everything from LowerINLINEASM to SelectInlineAsm. The new patch is attached. Theoretically, we can move everything related to memory constraint into single SelectInlineAsm method. But in this case I'll need to check all default behaviour that will be overridden. For me it is better to have as much default behaviour as possible. > In AVRISelDAGToDAG.cpp you've removed this line: > cast<MemSDNode>(Op)->getMemoryVT().getSimpleVT(); > to be: > + if (MemSDNode* MemNode = dyn_cast<MemSDNode>(Op)) > Is this condition always going to be true? the cast above never failed > for me so in theory the dyn_cast should always be true, making the rest > of elses there always false. If Op is not a MemSDNode it dyn_cast will return false. So MemSDNode is load or store node. InlineAsm in our case could play the same role too. If you think it is some extra code in SelectAddr I can move it to SelectInlineAsmMemoryOperand, though I'll have to copypaste something from SelectAddr in this case. -Stepan. Borja Ferrer wrote: > Jej no problem, I know you've been busy with some ARM stuff. > > Yes let's get this in first, and then we can talk about the multibyte stuff. > > About the patch: > Is all the code in AVRISelLowering.cpp necesarry? I find it to be a very > "manual" way of doing this optimization, it feels like not being very > general, so please confirm me that this the only way of doing it. Im > asking this in case in AVRISelDAGToDAG.cpp you could add some code to > match this sort of DAGs with the existing adressing mode matching > functions to avoid the code in the other file. Maybe look at what other > backends do? > > In AVRISelDAGToDAG.cpp you've removed this line: > cast<MemSDNode>(Op)->getMemoryVT().getSimpleVT(); > to be: > + if (MemSDNode* MemNode = dyn_cast<MemSDNode>(Op)) > Is this condition always going to be true? the cast above never failed > for me so in theory the dyn_cast should always be true, making the rest > of elses there always false. > > > > 2013/7/9 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na...>> > > Hello Borja, > Sorry for latency, I had to be on dev. meeting. > I've implemented optimization for memory constraint support. The > patch is in attachment. > > P.S.: When I started to work on this opt, I forgot about multibyte > constraints patch is not committed. So I can send it after we commit > this opt. > > -Stepan. > > Borja Ferrer wrote: > > Yes but the first goal now is to have inline asm feature complete xD > Then we can move into other places of the library. > > Taking a look at the previous emails this is what needs still to > be done: > 1) Implement the missed optimization above for the memory > constraint. > Also see if you can come with further cases. Remember to add a > test case. > 2) Implement the stuff in the "C names used in assembler code" > section > of the avrlibc manual. You mentioned some issues here in a > previous email. > 3) Implement the a0, a1, etc... constraints. > > > > 2013/7/1 Stepan Dyatkovskiy <stp...@na... > <mailto:stp...@na...> <mailto:stp...@na... > <mailto:stp...@na...>>> > > > Hi Borja. > 1. OK. Will add it. > 2. > > > movw r30, r24 > > adiw r30, 1 > Yup, we can replace it with "load r24, Z+1". I'll improve > LowerINLINEASM a bit then. > > 3. If I'm got right, our main goal now is to get avr-libc > compilable? Whould you tell me would else remained to do with > inline-asm? > > -Stepan. > > Borja Ferrer wrote: > > 1) Yes, an assert could do it. Fixup your patch to only > allow A > down to > D constraints. > 2) This code was compiled with -O3, cant you reproduce it? > > > 2013/6/29 Borja Ferrer <bor...@gm... > <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> > <mailto:bor...@gm... > <mailto:bor...@gm...> <mailto:bor...@gm... > <mailto:bor...@gm...>>__>__> > > > Ok more things, I've looked into the 64bit > constraints and gcc > doesnt seem to support them, for this code: > > unsigned long long delay(unsigned long long a, > unsigned > long long b) > > { > uint8_t cnt; > asm volatile ( > "add %A0, %A1" "\n\t" > "add %B0, %B1" "\n\t" > "add %C0, %C1" "\n\t" > "add %D0, %D1" "\n\t" > "add %E0, %E1" "\n\t" > "add %F0, %F1" "\n\t" > "add %G0, %G1" "\n\t" > "add %H0, %H1" "\n\t" > : "=r" (a) > : "r" (b)); > return a; > } > > gcc produces (ignoring frames): > movw r18,r10 > movw r20,r12 > movw r22,r14 > movw r24,r16 > /* #APP */ > ; 266 "test.c" 1 > add r10, r18 > add r11, r19 > add r12, r20 > add r13, r21 > add r10, r18 > add r10, r18 > add r10, r18 > add r10, r18 > > ; 0 "" 2 > /* #NOAPP */ > movw r18,r10 > movw r20,r12 > movw r22,r14 > movw r24,r16 > > notice how the last 4 add instructions are wrong. > > > > 2013/6/29 Borja Ferrer <bor...@gm... > <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> > <mailto:bor...@gm... > <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__>__> > > > Ignore my previous email, it was my fault. > Now, for the > following code: > > uint8_t delay(unsigned char *p) > > { > uint8_t cnt=8; > asm volatile ( > "ld %0, %1" "\n\t" > : "=r" (cnt) > : "Q" (p[1])); > return cnt; > } > > we get: > movw r30, r24 > adiw r30, 1 > ;APP > ld r24, Z > > ;NO_APP > ret > > Ideally, that adiw should be folded into the load. > > > > 2013/6/29 Borja Ferrer <bor...@gm... > <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> > <mailto:bor...@gm... > <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__>__> > > > Stepan I've been testing the memory > constraint but im > getting an assertion for the following code: > > uint8_t delay(unsigned char **p) > { > uint8_t cnt=8; > asm volatile ( > "ld %0, %1" "\n\t" > : "=r" (cnt) > : "Q" (p[7])); > return cnt; > } > > What's wrong in here? > > > 2013/6/29 Borja Ferrer > <bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> > <mailto:bor...@gm... > <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__>__> > > > 1) Ok then, I will look into it. > 2) Yes, in theory types should be > legalized up > to i16 > max, but for some reason i got that > assertion > triggered, > i think it was with multibyte inline > asm, so > this has to > be solved. > > > 2013/6/28 Borja Ferrer > <bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> > <mailto:bor...@gm... > <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__>__> > > > Hello Stepan, I will take a look > to your patch > later, but some questions: > > 1) does avr-gcc support 64bit values? > 2) now that you're working with > big data > types, does > it make sense to remove the assert I > commented out > about value types that are > different to i8 > AND i16? > > > > 2013/6/28 Stepan Dyatkovskiy > <stp...@na... <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>>>> > > > Hi Borja. > This is almost the final multibyte > constraint > patch. Tests are not created yet. > > Currently I have issue with 64 bit > values. Even > if I pass them as inline asm > operands, llvm > still truncates them to i32. > > What works: > > long a; // 32 bit > void f() { > asm("instr %A0 %B0 %C0 %D0": : > "r"(a)); > } > > > What doesn't work: > > long long a; // 64 bit > void f() { > asm("instr %A0 %B0 %E0": : > "r"(a)); > } > > The last one fires assertion > since llvm > allocates registers for the > first 32 > bits only. > > -Stepan. > > Borja Ferrer wrote: > > Yes fine > > El jueves, 27 de junio de > 2013, Stepan > Dyatkovskiy escribió: > > OK. Will do. > Currently there is a > patch > with memory > constraint and draft > multibyte constraint > (supports > only A > and B). To be clean under > multibyte constraints > I mean > something > like this: > asm volatile("mov > __tmp_reg__, > %A0" "\n\t" > "mov > %A0, %B0" > "\n\t" > "mov %B0, > __tmp_reg__" "\n\t" > : "=r" > (value) > : "0" > (value) > ); > > -Stepan. > Borja Ferrer wrote: > > btw, if this is > now feature > complete you can commit it. > > > 2013/6/27 Borja > Ferrer > <bor...@gm... > <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__> > > <mailto:bor...@gm... > <mailto:bor...@gm...> <mailto:bor...@gm... > <mailto:bor...@gm...>> > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__>__>__> > > No, that > code is a > personal > modification of your examples. > If you > aren't > getting the > correct > output from clang that's > because you > probably > didn't apply > a patch > i commited last week for > clang in our SVN. > The > assertion you're > getting > looks reasonable, it's an old > friend of > mine. The > reason is > that if Y > is being reserved as the frame > pointer, you > cant have an > instruction that uses 2 > registers > when > only Z is > available. I > wouldn't mind too much > about it. > > Now, my 2nd > question: > > Is there > anything > else that > needs to be covered for > the memory > constraint? > > > 2013/6/27 Stepan > Dyatkovskiy > <stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > <mailto:stp...@na... <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>>> > > <mailto:stp...@na... <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>>>>> > > This one > looks good. > Though ./llc emits "run out of > registers > > allocation. I'll > look at > it. Is that from avr-libc? > -Stepan. > Borja > Ferrer wrote: > > Yes, > and then I > replied the following: > > For > this C code: > > char > delay_count; > char > aaa; > uint8_t > delay(unsigned > char p) > { > > uint8_t > cnt; > > asm > volatile ( > > "inst %0, > %1" "\n\t" > > : "=Q" > (delay_count) > > : > "Q" (aaa)); > > return cnt; > } > > > Clang produces: > > > define i8 > @delay(i8 > %p) #0 { > entry: > > tail call > void asm > sideeffect "inst $0, > $1\0A\09", > > "=*Q,*Q"(i8* > > @delay_count, i8* > @aaa) #2, !srcloc !4 > > ret i8 undef > } > > > notice the *Q > constraints, so is there > anything > wrong in there? > > Is there > anything else > that needs to be covered > for the > memory > constraint? > > > > 2013/6/27 Stepan > Dyatkovskiy > <stp...@na... <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>>> > > > <mailto:stp...@na... <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>>>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>>> > > > <mailto:stp...@na... <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>>>>>> > > > > Hi Borja, > > > > Stepan, reply > in this thread to my 2 > questions again > if > the email > > got lost. > > > My prev. > reply: > > > > > Ahh, > I didn't > know avr-gcc was buggy on this > feature, > that's probably > > > the > reason for > why the code i pasted in my > previous > > email didn't > > work. > ... > > > What's wrong > with clang? I fixed clang > inline asm > > support back on > > > friday, so is > there anything else that is > broken? > It > should emit > '*Q' instead of 'Q'. > That's why > code > from > prev. email > > didn't > work, just > compare -emit-llvm -S > output of > > avr-clang with > > other > backends, > > > > About the > getPointerRegClass > function, what > is it > used > for? Also, > > please > > > move the > implementation to the cpp file > instead of > > leaving it in > > the .h. > > Currently, it is > used in registers coelescing > pass. > > While being > > registers > inflating, llvm lookups > all register > uses. If > it found > > that > register is > used by inline-asm as memory > operand, > it > requests > the > pointer class > with this method (I think > the largest > > one). But > > may be > in future > this method will be used in > more cases. > > > avr-gcc > has buggy > implementation of memory > constraint, > > that's why > > avr-libs > doesn't > use it at all. We have a > chance to be > > first here > > > -Stepan > > > Borja > Ferrer wrote: > > > Stepan, reply > in this thread to my 2 > questions > > again if the > > email got lost. > > > > 2013/6/27 > Borja Ferrer > <bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__> > > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__>__> > > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__> > > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__>__>__> > > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__> > > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__>__> > > <mailto:bor...@gm... > <mailto:bor...@gm...> <mailto:bor...@gm... > <mailto:bor...@gm...>> > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__> > > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__>__>__>__>__> > > > > > > No, your > last email only says : "So, > can I > > commit that > memory > > constraint patch?" > > > > 2013/6/27 Stepan Dyatkovskiy > > <stp...@na... <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>>>> > > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>>> > > <mailto:stp...@na... <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>>>>> > > > <mailto:stp...@na... <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>>> > > > <mailto:stp...@na... <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>>>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>>> > > > <mailto:stp...@na... <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... > <mailto:stp...@na...>>>>>>__> > > > > > Hm... Didn't you get my > previous > mail? If > not, > I think, > > I have > > to > change my mail box. > > > -Stepan > > > > -------- Original > Message -------- > > Subject: Re: > [avr-llvm-devel] Inline > > assembly. Mostly > > just > a stub. > > Date: Tue, 25 Jun 2013 > 18:30:46 +0400 > > From: Stepan Dyatkovskiy > > <stp...@na... <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>>>> > > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>>> > > <mailto:stp...@na... <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>>>>> > > > <mailto:stp...@na... <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>>> > > > <mailto:stp...@na... <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>>>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>>> > > > <mailto:stp...@na... <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>> > <mailto:stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... > <mailto:stp...@na...>>>>>>__> > > To: > Borja Ferrer > > <bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__> > > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__>__> > > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__> > > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__>__>__> > > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__> > > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__>__> > > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__> > > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>> > > <mailto:bor...@gm... <mailto:bor...@gm...> > <mailto:bor...@gm... > <mailto:bor...@gm...>>__>__>__>__>__> > > CC: > avr-llvm > > > <avr-llvm-devel@lists.__source____________forge.net > <http://source__________forge.net> > <http://source________forge.__net > <http://source________forge.net>> <http://source______forge.net> > > > > <http://source____forge.net> > > <http://source__forge.net> > > <http://source > <http://sourceforge.net> > > > > > > > > > > > > |