Menu
▾
▴
Re: [avr-llvm-devel] Inline assembly.
|
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.
>
>
>
>
>
>
>
>
>
>
>
>
|
