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