[LLVMdev] [PATCH] Before/After IR Dumps

[Chris Lattner]

On Mar 12, 2010, at 8:10 AM, David Greene wrote:
> On Friday 12 March 2010 08:13:05 Kalle Raiskila wrote:
>> David Greene wrote:
>>> Here's a rework using PassManager as Chris suggested.  Comments?
>> 
>> Tried this second patch with the svn version 97812 (the one the patch is
>> made against), but it doesn't compile:
>> "llvm/include/llvm/Pass.h:127: Error: expected unqualified-id before "&"
>> token"
>> Seems raw_ostream is forward declared but not defined (adding a missing
>> #include fixed that).
> 
> It shouldn't need to be defined.  Perhaps std::string is the issue, though I
> don't know why I don't see the problem.  Probably because 
> MachineFunctionPrinterPass.h includes <string> somewhere along
> the line.

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.

Please rename the getPrinterPass method to createPrinterPass to indicate that the result returns a new pass, not returning an existing one.  Please make it take a StringRef, not an std::string and eliminate the <string> #include from Pass.h.

MachineFunctionPrinterPass.h doesn't need to #include MachineFunctionPass.h, just use a forward declaration.  Also, please move the prototype of createMachineFunctionPrinterPass into include/llvm/CodeGen/Passes.h, eliminating the need for the header in the first place.

This hunk looks unrelated, please remove it:

+++ 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.

+Pass *LoopPass::getPrinterPass(raw_ostream &O,
+                               const std::string &Banner) const {
+  return createPrintFunctionPass(Banner, &O);
+}

likewise.  The printer needs to be a loop pass.

+++ lib/CodeGen/MachineFunctionPrinterPass.cpp	(revision 0)
@@ -0,0 +1,51 @@
+//===-- MachineFunction.cpp -----------------------------------------------===//

Please update the comment.


+#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.

+namespace llvm {
+struct MachineFunctionPrinterPass : public MachineFunctionPass {

This can be in an anonymous namespace, please add a doxygen comment.

+++ 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.


+++ 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.

+// 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.

+// 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.  Typo in the comment.  Please rename this to "ShouldPrintBeforePass", "PrintBeforePass" implies that it does printing.


+  bool PrintBeforeThis = PrintBeforeAll;
+  if (!PrintBeforeThis)

don't nest the entire function, use an early return like this:

  if (PrintBeforeAll) return true;


+    for (unsigned i = 0; i < PrintBefore.size(); ++i) {

Don't evaluate PrintBefore.size() every time through the loop.


+      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.


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

+++ 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?


Please make these modification and verify that regression tests pass with *only* this patch applied to a mainline tree.

-Chris