On Thursday 04 March 2010 01:54:55 Chris Lattner wrote:> On Mar 2, 2010, at 1:45 PM, David Greene wrote: > > This set of patches adds support for dumping IR before or after specified > > Passes. It adds the following command-line options: > > > > -print-before=<pass-option> > > -print-after=<pass-option> > > -print-before-all > > -print-after-all > > This patch looks very invasive for such a simple thing, isn't there a > better way?Possibly. What specifically do you object to? The main problem is that one needs different printers at different stages of the compiler: ModulePrinter, FunctionPrinter and MachineFunctionPrinter. It makes sense to write the code once and parameterize it on the printer type. I see addPass<> as the most "invasive" in the sense that the patch changes almost every call of PM.addPass(..) to addPass<>(PM, ...). That's a consequence of code sharing. I wanted a command-line interface that is intuitive. This requires that passes show up in the print option as a result of registering them with the PassManager. I could get rid of the "Previous" parameter to addPass<> if we don't mind duplicate output in the case where we want something printed after Pass A and before Pass B when Pass B immediately follows Pass A. I'm happy to revisit design decisions if you let me know what you want to see changed.> Also, please please please stay in 80 columns.Ah, thought I had fixed all of that. I will do another scan. -Dave
On Thursday 04 March 2010 09:28:22 David Greene wrote:> I could get rid of the "Previous" parameter to addPass<> if we don't mind > duplicate output in the case where we want something printed after > Pass A and before Pass B when Pass B immediately follows Pass A.I could get rid of "LastPass" as well if we don't mind duplicate output. This would remove the changes to the addPasses... interfaces which would reduce the impact of the patch. But the main cause of the widespread changes is changing PM.addPass(...) to addPass<>(PM,...) and I can't think of a way to avoid that change offhand. Those lines have to change in some way to tell some entity which kind of printer is needed. -Dave
On Mar 4, 2010, at 7:28 AM, David Greene wrote:> On Thursday 04 March 2010 01:54:55 Chris Lattner wrote: >> On Mar 2, 2010, at 1:45 PM, David Greene wrote: >>> This set of patches adds support for dumping IR before or after specified >>> Passes. It adds the following command-line options: >>> >>> -print-before=<pass-option> >>> -print-after=<pass-option> >>> -print-before-all >>> -print-after-all >> >> This patch looks very invasive for such a simple thing, isn't there a >> better way? > > Possibly. What specifically do you object to? The main problem is that > one needs different printers at different stages of the compiler: > ModulePrinter, FunctionPrinter and MachineFunctionPrinter. It makes > sense to write the code once and parameterize it on the printer type. > > I see addPass<> as the most "invasive" in the sense that the patch > changes almost every call of PM.addPass(..) to addPass<>(PM, ...). > That's a consequence of code sharing. > > I wanted a command-line interface that is intuitive. This requires that > passes show up in the print option as a result of registering them with > the PassManager.I'd expect the pass manager to manage all of this. To get the appropriate printer for a pass, it would invoke a virtual method on the pass itself, e.g.: P->getPrinterPass(); which would return a ModulePrinter, FunctionPrinter, MachineFunctionPrinter etc depending on the pass. -Chris
On Thursday 04 March 2010 18:48:39 Chris Lattner wrote:> I'd expect the pass manager to manage all of this. To get the appropriate > printer for a pass, it would invoke a virtual method on the pass itself, > e.g.: > > P->getPrinterPass(); > > which would return a ModulePrinter, FunctionPrinter, MachineFunctionPrinter > etc depending on the pass.Ok, that makes sense. I will rework the patch. -Dave
On Thursday 04 March 2010 09:28:22 David Greene wrote:> I'm happy to revisit design decisions if you let me know what you want to > see changed.Here's a rework using PassManager as Chris suggested. Comments? -Dave -------------- next part -------------- A non-text attachment was scrubbed... Name: clang-beforeafter.patch Type: text/x-patch Size: 605 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100308/f24c71b7/attachment.bin> -------------- next part -------------- A non-text attachment was scrubbed... Name: llvm-beforeafter.patch Type: text/x-patch Size: 20028 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100308/f24c71b7/attachment-0001.bin> -------------- next part -------------- A non-text attachment was scrubbed... Name: llvm-gcc-beforeafter.patch Type: text/x-patch Size: 1532 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100308/f24c71b7/attachment-0002.bin>
On Monday 08 March 2010 13:02:57 David Greene wrote:> On Thursday 04 March 2010 09:28:22 David Greene wrote: > > I'm happy to revisit design decisions if you let me know what you want to > > see changed. > > Here's a rework using PassManager as Chris suggested. Comments?Ping? -Dave
David Greene wrote:> Here's a rework using PassManager as Chris suggested. Comments?Tried this second patch with the svn version 97812 (the one the patch is made against), but it doesn't compile: "llvm/include/llvm/Pass.h:127: Error: expected unqualified-id before "&" token" Seems raw_ostream is forward declared but not defined (adding a missing #include fixed that). But the file MachineFunctionPrinterPass.h is still missing. kalle