From: Stepan D. <stp...@na...> - 2013-07-16 11:34:55
|
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. > > > > > > > |