From: Borja F. <bor...@gm...> - 2013-07-18 22:48:11
|
Ok fine ;) I've just commited a fix for the problem you mentioned before. Let me know if there are other issues with the assembler. About the multibyte stuff: I've carried some tests and i managed to crash it when i used other register constraints, probably related with your modification in avriselowering.cpp 2013/7/18 Stepan Dyatkovskiy <stp...@na...> > Hello Borja, > Sorry, but I can present configure in Sunday only. > > I'll proceed to work on remained inline issues then. > > -- > Truly yours, > Stepan Dyatkovskiy > > > 18.07.2013, 18:33, "Borja Ferrer" <bor...@gm...>: > > Heh thanks for those tests. I know you're a bit eager to compile avrlibc > :) but I would like first to get inline asm completed since it's the last > big blocker to get things compilable. > I'm going to try something I thought to see if your multibyte patch works > correctly under certain circumstances. > > About your questions: > > 1) Yes, the main reason is that we use a patched version of the greedy > regalloc, using others will produce wrong code. You can locally use that > patch to get avrlibc compilable but the code produced can be wrong. > 2) Can you attach the regenerated configure file so i can try it locally? > The best thing would be if Eric applies it to their svn repo. > > I'm aware of this error, I will try to fix it today. Is this the only type > of error you're getting? > > > > > 2013/7/18 Stepan Dyatkovskiy <stp...@na...> > > Hi Borja, > Well, ok. I will write several tests for it. Though I think also about > avr-libc compilation. Today I've made two small fixes in this direction: > > 1. Are there any reasons to forbid default FastRA for -O0? Note, currently > for -O0 it doesn't work at all. So I've temoporary restored default > behaviour here: > --- a/lib/Target/AVR/**AVRTargetMachine.cpp > +++ b/lib/Target/AVR/**AVRTargetMachine.cpp > @@ -140,6 +140,10 @@ bool AVRPassConfig::addPreEmitPass(**) > > FunctionPass *AVRPassConfig::**createTargetRegisterAllocator(**bool > Optimized) > { > - // Unconditionally use our custom greedy register allocator. > - return createGreedyRegisterAllocator(**); > +// // Unconditionally use our custom greedy register allocator. > +// return createGreedyRegisterAllocator(**); > + if (Optimized) > + return createGreedyRegisterAllocator(**); > + else > + return createFastRegisterAllocator(); > } > > 2. I have also fixed avr-libc configure.ac to allow avr*clang compilers > too. Current ToT filters out everything that is not in avr*gcc mask. > > 3. I also have patch applied (with multibytes referencing), I sent you few > days ago. > > After I did these three changes, configure script has been finished > successfully. Though during compilation it fails with next error (assembly > file is attached): > > /tmp/fclose-3936b6.s: Assembler messages: > /tmp/fclose-3936b6.s:22: Error: can't resolve `0' {*ABS* section} - > `__iob' {*UND* section} > /tmp/fclose-3936b6.s:22: Error: expression too complex > /tmp/fclose-3936b6.s:23: Error: can't resolve `0' {*ABS* section} - > `__iob' {*UND* section} > /tmp/fclose-3936b6.s:23: Error: expression too complex > > Note it was for -mmcu=avr5. > > P.S.: > Sorry about, I'm not following inline-asm testing strictly. To motivate > myself I need to see not only pedantic unit-test implementation but a real > progress too. > > Full invocation log: > > stepan@stepan-Inspiron-N5010:/**media/stepan/workproj/llvm.** > project/2-avr/avr-libc/trunk/**build.opt/avr/lib/avr5$ > /media/stepan/workproj/llvm.**project/2-avr/builds/llvm.** > debug-avr-arm-x86_64.build/**Debug+Asserts/bin/avr-clang -### > -DHAVE_CONFIG_H -I. -I../../../../avr-libc/avr/**lib/avr5 -I../../.. > -I../../../../avr-libc/common -I../../../../avr-libc/include > -I../../../include -Wall -W -Wstrict-prototypes -mmcu=avr5 > -D__COMPILING_AVR_LIBC__ -mcall-prologues -Os -MT fclose.o -MD -MP -MF > .deps/fclose.Tpo -c -o fclose.o ../../../../avr-libc/libc/**stdio/fclose.c > clang version 3.4 (http://llvm.org/git/clang.git3aa29df37b140f9c6786b6863a0cac > **195071b598) (http://llvm.org/git/llvm.git https://github.com/kaomoneus/* > *avr-llvm.git <https://github.com/kaomoneus/avr-llvm.git>ea6c1ebfeac54abd1583e90e518bdf > **3bab5e1fd5) > Target: avr > Thread model: posix > > "/media/stepan/workproj/llvm.**project/2-avr/builds/llvm.** > debug-avr-arm-x86_64.build/**Debug+Asserts/bin/clang" "-cc1" "-triple" > "avr" "-S" "-disable-free" "-main-file-name" "fclose.c" > "-mrelocation-model" "static" "-mdisable-fp-elim" "-fmath-errno" > "-mconstructor-aliases" "-target-linker-version" "2.22.90.20120924" > "-coverage-file" "/tmp/fclose-29b051.s" "-resource-dir" > "/media/stepan/workproj/llvm.**project/2-avr/builds/llvm.** > debug-avr-arm-x86_64.build/**Debug+Asserts/bin/../lib/**clang/3.4" > "-dependency-file" ".deps/fclose.Tpo" "-sys-header-deps" "-MP" "-MT" > "fclose.o" "-D" "HAVE_CONFIG_H" "-D" "__COMPILING_AVR_LIBC__" "-I" "." "-I" > "../../../../avr-libc/avr/lib/**avr5" "-I" "../../.." "-I" > "../../../../avr-libc/common" "-I" "../../../../avr-libc/include" "-I" > "../../../include" "-Os" "-Wall" "-W" "-Wstrict-prototypes" > "-fno-dwarf-directory-asm" "-fdebug-compilation-dir" > "/media/stepan/workproj/llvm.**project/2-avr/avr-libc/trunk/**build.opt/avr/lib/avr5" > "-ferror-limit" "19" "-fmessage-length" "147" "-mstackrealign" > "-fobjc-runtime=gcc" "-fobjc-default-synthesize-**properties" > "-fdiagnostics-show-option" "-fcolor-diagnostics" "-vectorize-loops" "-o" > "/tmp/fclose-29b051.s" "-x" "c" "../../../../avr-libc/libc/** > stdio/fclose.c" > "/usr/bin/avr-gcc" "-D" "HAVE_CONFIG_H" "-I" "." "-I" > "../../../../avr-libc/avr/lib/**avr5" "-I" "../../.." "-I" > "../../../../avr-libc/common" "-I" "../../../../avr-libc/include" "-I" > "../../../include" "-Wall" "-W" "-Wstrict-prototypes" "-mmcu=avr5" "-D" > "__COMPILING_AVR_LIBC__" "-mcall-prologues" "-Os" "-MT" "fclose.o" "-MD" > "-MP" "-MF" ".deps/fclose.Tpo" "-c" "-o" "fclose.o" "-x" "assembler" > "/tmp/fclose-29b051.s" > > Borja Ferrer wrote: > > Ok, if this will work with other reg constraints then add some tests to > cover them aswell. > > What errors are you getting exactly with avrlibc? IIRC there's an assert > in llc that fires when running the greedy regalloc in O0. I dont know > the reason for this assert and if it can be safely removed, this has to > be asked in the dev list and also get some possible solutions. > > 2013/7/17 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na... > >> > > > Hi Borja. > It should work for all other constraints as well. I don't like idea > of fixing clang too. So we can just update > getRegForInlineAsmConstraint method. > > I'd tried to compile avr-libc. Currently clang doesn't want to > compile everything with -O0. While configure script tests everything > with -O0. Do you have any ideas how to fix -O0 ? > > -Stepan. > > Borja Ferrer wrote: > > Also, I guess doing only this for r is not enough, you can use > multibytes with other register constraints. > > > 2013/7/16 Borja Ferrer <bor...@gm... > <mailto:bor...@gm...> > <mailto:bor...@gm... <mailto:bor...@gm...>**>__> > > > > What are you suggesting to do in clang? > > > 2013/7/16 Stepan Dyatkovskiy <stp...@na... > <mailto:stp...@na...> > <mailto:stp...@na... <mailto:stp...@na...>>> > > > > Back to multibyte referenced. > > I've attached patch with this feature + unit-test. > There is some > question though. > Currently, .c code: > int a; > void f() { > asm("instr %A0 %B0": : "r"(a)); > } > is transformed into IR below: > define void @f(i16 %a) { > entry: > call void asm sideeffect "instr ${0:A} ${0:B}", > "r"(i16 %a) > ret void > } > To prevent the crash I had to fix > getRegForInlineAsmConstraint, > I set it to return DREGS when it see i16 type or > bigger. Though, > may be you want to fix clang and replace "r" constraint > with > another one. > > -Stepan. > > Borja Ferrer wrote: > > Just remembered after hitting resend that this has > to be > done always, > similarly like in SelectAddr(). However in this > case it can > be done > unconditionally since imm is guaranteed to fit in > 6bits, so > even if it > comes in a constant node of type i64 it's safe to > truncate > it to i8. > > > 2013/7/15 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, > > > > + CanHandleRegImmOpt &= ImmNode != 0; > > + CanHandleRegImmOpt &= > ImmNode->getAPIntValue()._____**_getZExtValue() > > < 64; > > > You absolutely right here. Thanks. > > > > There's a piece of code i initially removed > because > i found it > redundant: > > + if (ImmNode->getValueType(0) != > MVT::i8) > > + { > > + Disp = CurDAG->getTargetConstant( > > + > ImmNode->getAPIntValue()._____**_getZExtValue(), > > MVT::i8); > > > + } > > + else > > + { > > + Disp = ImmOp; > > + } > > Initially did the same: just "Disp = ImmOp", > and got > the crash. I > don't remember what crash says exactly. > Currently, I'm > not able to > compile anything. What I had found out is that > Disp (aka > displacement) should be i8 only. Our back-end > just > don't expect > other types of displacements. > > -Stepan. > > Borja Ferrer wrote: > > Thanks! > > I've been cleaning a bit the code, and fixed > a > possible crash here: > > + CanHandleRegImmOpt &= ImmNode != 0; > + CanHandleRegImmOpt &= > ImmNode->getAPIntValue()._____**_getZExtValue() < 64; > > > > Since ImmNode comes from a dyncast, if > it's NULL > the second line > would crash. > > > > There's a piece of code i initially > removed because > i found it > redundant: > + if (ImmNode->getValueType(0) != > MVT::i8) > + { > + Disp = CurDAG->getTargetConstant( > + > ImmNode->getAPIntValue()._____**_getZExtValue(), > > MVT::i8); > > + } > + else > + { > + Disp = ImmOp; > + } > > but without im getting crashes. Can you > explain why > this happens > and is needed? > > > > 2013/7/15 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...>>>>> > > > r269 > -Stepan > Borja Ferrer wrote: > > Ohhh this is a lot better, > atleast fixing > patches > iteratively > brought us > to a nice solution :) > > If this patch passes all tests > with the > machine > verifier enabled > then > feel free to commit it. I'll take > a look > at the glue > operands > after this. > > > 2013/7/12 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...>>>>>**> > > > Hello Borja. > Seems I've made things much > simpler. > Can't > understand why I > didn't > it in beginning. > > I moved everything into > SelectInlineAsmMemoryOperand hook. > #1: We don't need 'for' at > all. Since it > implemented in > hook caller. > #2: This problem > disappeared, see #1. > #4: The same. > #6: Still not sure about > glues. I > just tried to > keep all > chains in > proper order. But you rather > look at new > SelectInlineAsmMemoryOperand. > > > > One last thing, when you > run the > regression tests, > enable locally the > > machine verifier to catch > any > additional errors. To > enable it > > unconditionally search > for the > command object > (i think it's > declared in > > codegen/passes.cpp as a > cl::opt) > and turn it on by > default. When > you're > > doing your own tests you can > enable it passing > -verify-machineisntrs or > > something like that to llc. > OK. > > -Stepan. > > > Borja Ferrer wrote: > > Hello Stepan, > > 1) Wow, those are really > big > conditions xD. > You could > split them > into > single conditions and > use continues: > if (cond 1) continue; > if (cond 2) continue; > etc... > > instead of having a huge > if() > that is quite > hard to read. > > 2) Yes I thought about that > aswell, that not all > constant nodes > are asm > flags, but the ARM > backend does > this, so > either they > have a bug > there or > it's safe? > I prefer if you could > move the > big condition > inside the > for() > into the > top of the loop, it > makes the > for() quite > unreadable. > And adjust > there > the i variable as needed > for each > suboperand > as you > mentioned > before. > > 4) Why do you say they > handle > everything in the > SelectInlineAsm > method? > As far as i can see they > only > handle one > specific case, > otherwise they > return NULL and let the > default > behaviour do > the work. > > 6) Yes glues are a bit > tricky, > dont worry > about them, I > will fix > them > when this gets commited. > > One last thing, when you > run the > regression > tests, enable > locally the > machine verifier to > catch any > additional > errors. To > enable it > unconditionally search > for the > command object > (i think it's > declared in > codegen/passes.cpp as a > cl::opt) > and turn it on by > default. When > you're > doing your own tests you > can > enable it passing > -verify-machineisntrs or > something like that to llc. > > > > > > > > > > > > |