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