From: Borja F. <bor...@gm...> - 2013-07-16 13:09:32
|
What are you suggesting to do in clang? 2013/7/16 Stepan Dyatkovskiy <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... >> >> >> >> 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. >> >> >> >> >> >> >> >> > |