[LLVMdev] [PATCH] Before/After IR Dumps

[David Greene]

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, and

Ok.

> 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