From: Borja F. <bor...@gm...> - 2013-07-16 13:18:40
|
Also, I guess doing only this for r is not enough, you can use multibytes with other register constraints. 2013/7/16 Borja Ferrer <bor...@gm...> > 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. >>> >>> >>> >>> >>> >>> >>> >>> >> > |