On Monday 15 March 2010 13:45:14 David Greene wrote:> On Sunday 14 March 2010 18:32:35 Chris Lattner wrote: > > This is much better than the first iteration but still has many issues.I believe I've addressed all your points with this patch except I didn't use StringRef. It doesn't appear to be useful since createPrinterPass will be sent a const std::string & and will feed a constant std::string & in the create*PrinterPass interfaces. Any additional feedback from anyone? -Dave -------------- next part -------------- A non-text attachment was scrubbed... Name: llvm-beforeafter.patch Type: text/x-patch Size: 22042 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100317/4eb9d101/attachment.bin>
On Wednesday 17 March 2010 13:25:26 David Greene wrote:> On Monday 15 March 2010 13:45:14 David Greene wrote: > > On Sunday 14 March 2010 18:32:35 Chris Lattner wrote: > > > This is much better than the first iteration but still has many issues. > > I believe I've addressed all your points with this patch except I didn't > use StringRef. It doesn't appear to be useful since createPrinterPass will > be sent a const std::string & and will feed a constant std::string & in the > create*PrinterPass interfaces. > > Any additional feedback from anyone?Ping? -Dave
On Friday 19 March 2010 14:43:48 David Greene wrote:> On Wednesday 17 March 2010 13:25:26 David Greene wrote: > > On Monday 15 March 2010 13:45:14 David Greene wrote: > > > On Sunday 14 March 2010 18:32:35 Chris Lattner wrote: > > > > This is much better than the first iteration but still has many > > > > issues. > > > > I believe I've addressed all your points with this patch except I didn't > > use StringRef. It doesn't appear to be useful since createPrinterPass > > will be sent a const std::string & and will feed a constant std::string & > > in the create*PrinterPass interfaces. > > > > Any additional feedback from anyone? > > Ping?Ping? -Dave
On Mar 17, 2010, at 11:25 AM, David Greene wrote:> On Monday 15 March 2010 13:45:14 David Greene wrote: >> On Sunday 14 March 2010 18:32:35 Chris Lattner wrote: >>> This is much better than the first iteration but still has many issues. > > I believe I've addressed all your points with this patch except I didn't use > StringRef. It doesn't appear to be useful since createPrinterPass will > be sent a const std::string & and will feed a constant std::string & > in the create*PrinterPass interfaces. > > Any additional feedback from anyone?This is looking much better, thanks. I'd still prefer using StringRef instead of std::string here because it cleans up the interface, but I won't insist. Please don't do this though: + bool runOnSCC(std::vector<CallGraphNode *> &SCC) { + ModulePass *Printer = createPrintModulePass(Banner, &Out); + Printer->runOnModule(*M); + delete Printer; + return false; It would be better to just loop over the callgraphnodes and call F->print(Out) on the functions for each node. If you'd prefer, you can just emit "not implemented for CGSCC nodes yet" if you're not interested in implementing the logic. + bool runOnLoop(Loop *L, LPPassManager &) { + FunctionPass *Printer = createPrintFunctionPass(Banner, &Out); + Printer->runOnFunction(*L->getHeader()->getParent()); + delete Printer; + return false; + } This can just call print on the function, no need to create the pass. It would be even better to just print the blocks in the loop or print "not implemented yet" though. +// Print IR out before/after specified passes. +PassOptionList +PrintBefore("print-before", + llvm::cl::desc("Print IR before specified passes")); + +PassOptionList +PrintAfter("print-after", + llvm::cl::desc("Print IR after specified passes")); + +cl::opt<bool> +PrintBeforeAll("print-before-all", + llvm::cl::desc("Print IR before each pass"), + cl::init(false)); +cl::opt<bool> +PrintAfterAll("print-after-all", + llvm::cl::desc("Print IR after each pass"), + cl::init(false)); Please declare these as static, for the same reasons functions are, and move the end of the anon namespace up. It might also be better to just make this take a list of strings, which would allow you to collapse these to support -print-before=all and -print-after=all. +Pass *BasicBlockPass::createPrinterPass(raw_ostream &O, + const std::string &Banner) const { + return createPrintFunctionPass(Banner, &O); +} + This will disrupt the pass structure. The printer should be a BasicBlockPass. However, BasicBlockPass is probably nearly dead, if you want to remove it instead as a separate patch, that would also be great :) Your patch doesn't include the file that defines createMachineFunctionPrinterPass, but I assume it's fine. After making these changes, please test the resultant patch against mainline with just the patch applied and commit if it looks ok. Thanks David, -Chris
On Friday 26 March 2010 18:19:07 Chris Lattner wrote:> > Any additional feedback from anyone? > > This is looking much better, thanks. I'd still prefer using StringRef > instead of std::string here because it cleans up the interface, but I won't > insist.Wouldn't using StringRef actually cause more constructors to be invoked to manage the type conversions? The alternative is to move all interfaces over to StringRef but I don't think that's in the scope of this patch.> Please don't do this though: > > + bool runOnSCC(std::vector<CallGraphNode *> &SCC) { > + ModulePass *Printer = createPrintModulePass(Banner, &Out); > + Printer->runOnModule(*M); > + delete Printer; > + return false; > > It would be better to just loop over the callgraphnodes and call > F->print(Out) on the functions for each node. If you'd prefer, you can > just emit "not implemented for CGSCC nodes yet" if you're not interested in > implementing the logic.Ok, I'll try to implement the logic. <Goes off to learn about CallGraph... :)>> + bool runOnLoop(Loop *L, LPPassManager &) { > + FunctionPass *Printer = createPrintFunctionPass(Banner, &Out); > + Printer->runOnFunction(*L->getHeader()->getParent()); > + delete Printer; > + return false; > + } > > This can just call print on the function, no need to create the pass. It > would be even better to just print the blocks in the loop or print "not > implemented yet" though.Ok.> +// Print IR out before/after specified passes. > +PassOptionList > +PrintBefore("print-before", > + llvm::cl::desc("Print IR before specified passes")); > + > +PassOptionList > +PrintAfter("print-after", > + llvm::cl::desc("Print IR after specified passes")); > + > +cl::opt<bool> > +PrintBeforeAll("print-before-all", > + llvm::cl::desc("Print IR before each pass"), > + cl::init(false)); > +cl::opt<bool> > +PrintAfterAll("print-after-all", > + llvm::cl::desc("Print IR after each pass"), > + cl::init(false)); > > Please declare these as static, for the same reasons functions are, andOk.> move the end of the anon namespace up. It might also be better to just > make this take a list of strings, which would allow you to collapse these > to support -print-before=all and -print-after=all.Interesting idea. I'll think about it for a follow-on patch.> +Pass *BasicBlockPass::createPrinterPass(raw_ostream &O, > + const std::string &Banner) const { > + return createPrintFunctionPass(Banner, &O); > +} > + > > This will disrupt the pass structure. The printer should be a > BasicBlockPass. However, BasicBlockPass is probably nearly dead, if you > want to remove it instead as a separate patch, that would also be great :)I'll see what I can do.> Your patch doesn't include the file that defines > createMachineFunctionPrinterPass, but I assume it's fine.Odd. This was a patch generated from a clean upstream. It built fine. I'll see if maybe I missed something.> After making these changes, please test the resultant patch against > mainline with just the patch applied and commit if it looks ok. Thanks > David,Will do. -Dave