Menu
▾
▴
Re: [avr-llvm-devel] Inline assembly.
|
From: Borja F. <bor...@gm...> - 2013-07-18 14:33:35
|
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.
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>
|
