Menu
▾
▴
Re: [avr-llvm-devel] Inline assembly.
|
From: Borja F. <bor...@gm...> - 2013-07-17 14:07:39
|
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...>
> 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...>**>
>>
>>
>> What are you suggesting to do in clang?
>>
>>
>> 2013/7/16 Stepan Dyatkovskiy <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...>>>
>>
>> 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...
>> >>>>
>>
>>
>> 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...>>>>>
>>
>>
>> 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.
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>
|
