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

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

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

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