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