[LLVMdev] [PATCH] Before/After IR Dumps

[Chris Lattner]

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