On 05/06/2013 04:23 PM, Eric Christopher wrote:> On Mon, May 6, 2013 at 3:08 PM, reed kotler <rkotler at mips.com> wrote: >> 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'm pretty sure you're the only one who thinks this. What's so > complicated about doing this either at selection dag or MI lowering > time? > > -ericAn earlier part of this I actually did do with selection DAG and it was messy and the code was much less easy to understand than this Module IR pass is. r173320 I intend to move most of this code into the new pass at a later time. It will make it possible to understand the whole scheme in one place. Much of the code needs to access the IR anyway, even though it finds it starting with the 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 >> I 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 >>>> >> >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
That code looks a little odd but not too awful. I still don't understand what's so wrong/complicated about this. You seem to be of the opinion that injecting asm into a stream ala gcc is a good idea in clang, it really isn't. -eric On Mon, May 6, 2013 at 5:45 PM, reed kotler <rkotler at mips.com> wrote:> On 05/06/2013 04:23 PM, Eric Christopher wrote: >> >> On Mon, May 6, 2013 at 3:08 PM, reed kotler <rkotler at mips.com> wrote: >>> >>> 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'm pretty sure you're the only one who thinks this. What's so >> complicated about doing this either at selection dag or MI lowering >> time? >> >> -eric > > An earlier part of this I actually did do with selection DAG and it was > messy and the code was much less easy to understand than this Module IR pass > is. > > r173320 > > I intend to move most of this code into the new pass at a later time. It > will make it possible > to understand the whole scheme in one place. > > Much of the code needs to access the IR anyway, even though it finds it > starting with the > 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 >>> >>> I 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 >>>>> >>> >>> _______________________________________________ >>> LLVM Developers mailing list >>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > > >
On 05/09/2013 04:22 PM, Eric Christopher wrote:> That code looks a little odd but not too awful. I still don't > understand what's so wrong/complicated about this. You seem to be of > the opinion that injecting asm into a stream ala gcc is a good idea in > clang, it really isn't. > > -ericHi Eric, I'm going to start putting this code back soon. It's in several patches, the first of which does not have any inline assembly code being emitted. It's not a huge amount of code and it's in it's own separate IR module pass. I am doing more testing and some cleanup as a result of internal review by Akira. When I do, you can look at what I did and tell me how I could have done it differently. I try and float all my ideas by the list before doing any major work and I would have brought this up if I had thought it was important. The emitting of the actual body of the stub is the trivial part of this. Maybe I can move that code somewhere else. You can advise. This interoperability between mips16/mips32 for floating point is very complicated and by doing it they way I did, it's really easy to read the code and see the whole thing in one place. I can't really imagine trying to understand all of this with it spread out in the DAGISEL target lowering code. The stubs and the code I'm compiling are not even for the same instruction: the code is mip16 code and the stubs are mips32. I don't think it's an objective fact that injecting asm in the stream is not a good idea but surely some (maybe many or most) people would agree with you. reed> > On Mon, May 6, 2013 at 5:45 PM, reed kotler <rkotler at mips.com> wrote: >> On 05/06/2013 04:23 PM, Eric Christopher wrote: >>> >>> On Mon, May 6, 2013 at 3:08 PM, reed kotler <rkotler at mips.com> wrote: >>>> >>>> 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'm pretty sure you're the only one who thinks this. What's so >>> complicated about doing this either at selection dag or MI lowering >>> time? >>> >>> -eric >> >> An earlier part of this I actually did do with selection DAG and it was >> messy and the code was much less easy to understand than this Module IR pass >> is. >> >> r173320 >> >> I intend to move most of this code into the new pass at a later time. It >> will make it possible >> to understand the whole scheme in one place. >> >> Much of the code needs to access the IR anyway, even though it finds it >> starting with the >> 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 >>>> >>>> I 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 >>>>>> >>>> >>>> _______________________________________________ >>>> LLVM Developers mailing list >>>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> >> >>