[LLVMdev] [PATCH] Before/After IR Dumps

[David Greene]

On Sunday 14 March 2010 18:32:35 Chris Lattner wrote:

> This is much better than the first iteration but still has many issues. 
> There is no need to keep cc'ing cfe-dev on these patches which have nothing
> to do with clang.

There's a clang patch in this set.

> Please rename the getPrinterPass method to createPrinterPass to indicate
> that the result returns a new pass, not returning an existing one.  Please

Ok.

> make it take a StringRef, not an std::string and eliminate the <string>
> #include from Pass.h.

Ok.

> MachineFunctionPrinterPass.h doesn't need to #include
> MachineFunctionPass.h, just use a forward declaration.  Also, please move

Ok.

> the prototype of createMachineFunctionPrinterPass into
> include/llvm/CodeGen/Passes.h, eliminating the need for the header in the
> first place.

Ok.

> This hunk looks unrelated, please remove it:

Err...I need to override the virtual method.  Believe me, I did NOT want to
mess with this stuff.  I wouldn't have touched it if I didn't have to.

The original patch didn't mess with PassManager at all, which is why it was
structured the way it was (templates, etc.).  I agree this patch is better, 
but this is part of the tradeoff.

> +++ include/llvm/PassManager.h	(working copy)
> @@ -18,6 +18,7 @@
>  #define LLVM_PASSMANAGER_H
>
>  #include "llvm/Pass.h"
> +#include "llvm/Support/PassNameParser.h"
>
>  namespace llvm {
>
>
>
> +Pass *CallGraphSCCPass::getPrinterPass(raw_ostream &O,
> +                                       const std::string &Banner) const {
> +  return createPrintModulePass(Banner, &O);
> +}
>
> This isn't correct at all, this should return a CGSCCPass as a printer. 
> Adding a Module pass will change the ordering of passes in the passmanager,
> which will cause your option to completely change behavior of the
> optimization sequence.

Which is why I didn't want to mess with PassManager.  All of this subtle stuff 
gets really tricky.

So now I need another printer class?  This is getting messy...

> +Pass *LoopPass::getPrinterPass(raw_ostream &O,
> +                               const std::string &Banner) const {
> +  return createPrintFunctionPass(Banner, &O);
> +}
>
> likewise.  The printer needs to be a loop pass.

And another one...

These extra printer passes are troublesome.  I have to define them even though
they are not used anywhere.  After a LoopPass runs, wouldn't one want to look
at the whole Function, not just the Loop?  That's what I would want for 
debugging purposes.  So how would one go about doing this with this design?
The previous design allowed the client adding the Pass to determine what kind
of printout it wanted afterward.

I'm not arguing for the old version, just pointing out tradeoffs.

> +++ lib/CodeGen/MachineFunctionPrinterPass.cpp	(revision 0)
> @@ -0,0 +1,51 @@
> +//===-- MachineFunction.cpp
> -----------------------------------------------===//
>
> Please update the comment.

Ok.

>
> +#include "llvm/CodeGen/MachineFunction.h"
> +#include "llvm/CodeGen/MachineFunctionPrinterPass.h"
> +#include "llvm/Support/raw_ostream.h"
>
> Include the file's interface first.  This should be CodeGen/Passes.h after
> the change requested above.

Ok.

> +namespace llvm {
> +struct MachineFunctionPrinterPass : public MachineFunctionPass {
>
> This can be in an anonymous namespace, please add a doxygen comment.

Right.

> +++ lib/Target/X86/X86ISelDAGToDAG.cpp	(working copy)
> @@ -1548,6 +1548,107 @@
>
> +  case ISD::STORE: {
> +    // Handle unaligned non-temporal stores.
> +    StoreSDNode *ST = cast<StoreSDNode>(Node);
> +    DebugLoc dl = Node->getDebugLoc();
> +    EVT VT = ST->getMemoryVT();
> +    if (VT.isVector() &&
> +        VT.isFloatingPoint() &&
> +        VT.getSizeInBits() == 128 &&
> +        ST->getAlignment() < 16) {
> +      // Unaligned stores
>
> This is completely unrelated to your patch.

Erk.  I thought I got rid of that.  Will do so now.

> +++ lib/VMCore/PassManager.cpp	(working copy)
>
>  #include "llvm/PassManagers.h"
> +#include "llvm/Assembly/PrintModulePass.h"
>  #include "llvm/Assembly/Writer.h"
>  #include "llvm/Support/CommandLine.h"
>
> You don't need this #include.

Ok.

> +// Register a pass and add options to dump the IR before and after it
> +// is run
> +typedef llvm::cl::list<const llvm::PassInfo *, bool, PassNameParser>
> +PassOptionList;
>
> This comment is out of date.

Not sure, I'll check.

> +// Print IR out before/after specified passes
> +PassOptionList
> +PrintBefore("print-before",
> +            llvm::cl::desc("Print IR before specified passes"));
>
> Please properly punctuate your comment.

...

> +namespace {
> +/// This is a utility to check whether a pass shoulkd have IR dumped
> +/// before it.
> +bool PrintBeforePass(Pass *P) {
>
> Please just mark stand-alone functions "static" don't put them in anonymous
> namespaces.  

Ok.  Out of curiosity, why the preference for static?

> Typo in the comment.  Please rename this to
> "ShouldPrintBeforePass", "PrintBeforePass" implies that it does printing.

Ok.

> +  bool PrintBeforeThis = PrintBeforeAll;
> +  if (!PrintBeforeThis)
>
> don't nest the entire function, use an early return like this:

Ok.

> +    for (unsigned i = 0; i < PrintBefore.size(); ++i) {
> Don't evaluate PrintBefore.size() every time through the loop.

Ok, but that's a bit nitpicky...  :)

> +      if (PassInf && P->getPassInfo())
> +        if (PassInf->getPassArgument() ==
> +            P->getPassInfo()->getPassArgument()) {
> +          PrintBeforeThis = true;
> +          break;
> +        }
> +    }
> +  return PrintBeforeThis;
> +}
>
> Instead of using break with a variable, just "return true;" out of the
> loop.

Ok, leftover structure from the original version.  Will clean it up.

> Please merge and factor PrintBeforePass/PrintAfterPass instead of copying
> and pasting it.

Ok.

> +++ lib/VMCore/PrintModulePass.cpp	(working copy)
> @@ -20,24 +20,25 @@
>  #include "llvm/Support/raw_ostream.h"
>  using namespace llvm;
>
> -namespace {
> +namespace llvm {
>
> Why did you change this?

So it would be visible.  I will re-check.

                                              -Dave