Menu
▾
▴
Re: [avr-llvm-devel] Inline assembly.
|
From: Stepan D. <stp...@na...> - 2013-07-18 11:12:16
|
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.git
3aa29df37b140f9c6786b6863a0cac195071b598) (http://llvm.org/git/llvm.git
https://github.com/kaomoneus/avr-llvm.git
ea6c1ebfeac54abd1583e90e518bdf3bab5e1fd5)
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.
>
>
>
>
>
>
>
>
>
>
>
>
|
