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