Re: [avr-llvm-devel] Inline assembly. Mostly just a stub.

[Stepan D. <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...>>
>
>     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...>>>
>
>
>              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.
>
>
>
>
>