On 05/06/2013 07:52 AM, Rafael Espíndola wrote:> On 6 May 2013 10:29, reed kotler <rkotler at mips.com> wrote: >> I want to disable the #APP/#NOAPP for compiler generated inline asm. >> >> Unfortunately, you can change the string APP,NOAPP, but it still will put >> the "#" there and create >> a line. >> >> In the comments it said that the strings were #APP,#NOAPP but really it's >> just the part after the >> comment_string=='#' >> >> I'd like to just add a mode flag to AsmPrinter for this. >> >> Any objections? > Yes. I don't think we should introduce the notion of "compiler > generated inline asm".Hi Rafael, At this time I have to generate compiler stubs for mips16/32 floating point interoperability and I've already implemented it all and I did it by generating inline assembly in a module pass. The whole problem this is solving is complicated and has many cases and my solution was very clean and easy to understand. It's all in the Mips port and I don't have to time or desire to redo the whole thing right now. It's working fine just that it's ugly to see those APP/NOAPP markers. I think that I can actually replace the inline assembly for these particular stubs with some non inline asm using special calling conventions but I don't want to do that right now. I've filed a local bug at mips against myself to do that work. There are some further issues here but Akira and I think it's certainly doable. I want to finish testing and checking in what I have and then will work on improvements later. There are many things to still do for mips 16 and this part already is done and works as in gcc. I'm still debugging and writing test cases. I think that it's perfectly valid to generate inline assembler and it looks 1000 times cleaner than if I tried to do this same work with selection DAG. There are other stubs to be created for other parts of the mips32 port. So I'd like to get a solution to these ugly APP/NOAPP markers. Maybe you would not do things this way but I think it's a perfectly valid approach. No two people have 100% the same idea of what is best practices. The following kind of patch works without adding a mode to asm printer but in general is harder for people to use since they have to get a hook to MCAsm in order to change the inline_asm_start/end strings. (In AsmPrinter) on a per function basis. if (OutStreamer.hasRawTextSupport() && MAI->getInlineAsmStart()[0]) OutStreamer.EmitRawText(Twine("\t")+MAI->getCommentString()+ MAI->getInlineAsmStart()); So a simple method in AsmPrinter would be the easiest for people to use. It just turns off the APP/NOAPP markers which we should be able to do anyway; it's independent of the discussion regarding the goodness or not of the compiler emitting inline assembly. Reed>> Alternately I could change the logic in AsmPrinter to not print a line if >> the inlline asm start/end string is null. >> >> ???? >> >> TIA. >> >> Reed >> > Cheers, > Rafael
> It's working fine just that it's ugly to see those APP/NOAPP markers.Inline assembly is inline assembly. It has the semantics defined in the IL documentation and should all be treated uniformly. I guess I would be OK with unconditionally removing those comments (I don't see a lot of value in them) or having different verbosity levels for the asm output. What we should never have is a "if (this asm was created by llvm itself)". Cheers, Rafael
On 05/06/2013 08:51 AM, Rafael Espíndola wrote:>> It's working fine just that it's ugly to see those APP/NOAPP markers. > Inline assembly is inline assembly. It has the semantics defined in > the IL documentation and should all be treated uniformly. > > I guess I would be OK with unconditionally removing those comments (I > don't see a lot of value in them) or having different verbosity levels > for the asm output. > > What we should never have is a "if (this asm was created by llvm itself)".I would like to see a method in asm printer which turns on/off these comments. It's trivial to add and use but I can't put back to this code without permission so there is no point to write the patch if nobody will approve it. Then I could call that method when I'm processing compiler generated stubs that have inline assembly. Traditionally these comments are used in gcc so that when you look at assembly code, you can tell which was generated by the compiler and which was inline assembly the user created. People writing inline assembly tend to write multi-line strings for the inline asm block so they don't have so many of these wrappers as I do. I'm generating a line at a time. Adding additional compiler options is another issue. If you just turned these comments all off right now, I think that "make check" would fail a lot because people are looking for these markers in some tests.> Cheers, > Rafael
----- Original Message -----> On 05/06/2013 07:52 AM, Rafael Espíndola wrote: > > On 6 May 2013 10:29, reed kotler <rkotler at mips.com> wrote: > >> I want to disable the #APP/#NOAPP for compiler generated inline > >> asm. > >> > >> Unfortunately, you can change the string APP,NOAPP, but it still > >> will put > >> the "#" there and create > >> a line. > >> > >> In the comments it said that the strings were #APP,#NOAPP but > >> really it's > >> just the part after the > >> comment_string=='#' > >> > >> I'd like to just add a mode flag to AsmPrinter for this. > >> > >> Any objections? > > Yes. I don't think we should introduce the notion of "compiler > > generated inline asm". > Hi Rafael, > > At this time I have to generate compiler stubs for mips16/32 floating > point interoperability and I've already implemented it all and I did > it > by generating inline assembly in a module pass. > > The whole problem this is solving is complicated and has many cases > and > my solution was very clean and easy to understand. > > It's all in the Mips port and I don't have to time or desire to redo > the > whole thing right now. > > It's working fine just that it's ugly to see those APP/NOAPP markers. > > I think that I can actually replace the inline assembly for these > particular stubs with some non inline asm using special calling > conventions but I don't want to do that right now. I've filed a local > bug at mips against myself to do that work. There are some further > issues here but Akira and I think > it's certainly doable. > > I want to finish testing and checking in what I have and then will > work > on improvements later. > > There are many things to still do for mips 16 and this part already > is > done and works as in gcc. > I'm still debugging and writing test cases. > > I think that it's perfectly valid to generate inline assembler and it > looks 1000 times cleaner than if I tried to do this same work with > selection DAG.I hope you don't mind if I play devil's advocate here... Why is this so complicated that it would be messy to do, at least in part, at a lower level? I can understand needing IR-level analysis for some kinds of transformations, but late IR-level passes can insert target-specific intrinsics, those can be matched to pseudo instructions, and those pseudo instructions can be expanded (as late as necessary) by a custom inserter. I agree that this may add more boiler-plate work, but it is not immediately obvious why this would be 1000x messier. -Hal> > There are other stubs to be created for other parts of the mips32 > port. > > So I'd like to get a solution to these ugly APP/NOAPP markers. > > Maybe you would not do things this way but I think it's a perfectly > valid approach. > No two people have 100% the same idea of what is best practices. > > The following kind of patch works without adding a mode to asm > printer > but in general is harder > for people to use since they have to get a hook to MCAsm in order to > change the inline_asm_start/end > strings. (In AsmPrinter) on a per function basis. > > if (OutStreamer.hasRawTextSupport() && > MAI->getInlineAsmStart()[0]) > OutStreamer.EmitRawText(Twine("\t")+MAI->getCommentString()+ > MAI->getInlineAsmStart()); > > So a simple method in AsmPrinter would be the easiest for people to > use. > > It just turns off the APP/NOAPP markers which we should be able to do > anyway; it's independent of the discussion regarding the goodness or > not > of the compiler emitting inline assembly. > > > Reed > > > > >> Alternately I could change the logic in AsmPrinter to not print a > >> line if > >> the inlline asm start/end string is null. > >> > >> ???? > >> > >> TIA. > >> > >> Reed > >> > > Cheers, > > Rafael > > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >
Hi Hal, I think that it's perfectly valid to generate inline assembler and it looks 1000 times cleaner than if I tried to do this same work with selection DAG.> I hope you don't mind if I play devil's advocate here... > > Why is this so complicated that it would be messy to do, at least in part, at a lower level? I can understand needing IR-level analysis for some kinds of transformations, but late IR-level passes can insert target-specific intrinsics, those can be matched to pseudo instructions, and those pseudo instructions can be expanded (as late as necessary) by a custom inserter. I agree that this may add more boiler-plate work, but it is not immediately obvious why this would be 1000x messier. > > -HalI should probably qualify this with "1000 times messier for me" :) In this case the whole problem fit neatly in a simple IR level module pass. I had to create an alternate calling convention for one part but otherwise this IR pass made an otherwise very messy problem easy to implement. Reed>> There are other stubs to be created for other parts of the mips32 >> port. >> >> So I'd like to get a solution to these ugly APP/NOAPP markers. >> >> Maybe you would not do things this way but I think it's a perfectly >> valid approach. >> No two people have 100% the same idea of what is best practices. >> >> The following kind of patch works without adding a mode to asm >> printer >> but in general is harder >> for people to use since they have to get a hook to MCAsm in order to >> change the inline_asm_start/end >> strings. (In AsmPrinter) on a per function basis. >> >> if (OutStreamer.hasRawTextSupport() && >> MAI->getInlineAsmStart()[0]) >> OutStreamer.EmitRawText(Twine("\t")+MAI->getCommentString()+ >> MAI->getInlineAsmStart()); >> >> So a simple method in AsmPrinter would be the easiest for people to >> use. >> >> It just turns off the APP/NOAPP markers which we should be able to do >> anyway; it's independent of the discussion regarding the goodness or >> not >> of the compiler emitting inline assembly. >> >> >> Reed >> >> >> >>>> Alternately I could change the logic in AsmPrinter to not print a >>>> line if >>>> the inlline asm start/end string is null. >>>> >>>> ???? >>>> >>>> TIA. >>>> >>>> Reed >>>> >>> Cheers, >>> Rafael >> >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >>