Menu
▾
▴
Re: [avr-llvm-devel] Inline assembly.
|
From: Stepan D. <stp...@na...> - 2013-07-16 11:34:55
|
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.
>
>
>
>
>
>
>
|
