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

[Borja F. <bor...@gm...>]

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...>

> 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.
>>
>>
>>
>>
>>
>>
>