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
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. PleaseOk.> 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 moveOk.> 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
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? -Dave -------------- next part -------------- A non-text attachment was scrubbed... Name: llvm-beforeafter.patch Type: text/x-patch Size: 22042 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100317/4eb9d101/attachment.bin>
On Mar 15, 2010, at 11:45 AM, David Greene wrote:>> +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?It reduces nesting and makes it easier to understand whether a function is local or not. static void foo() ... is obviously static, but: void foo() ... looks non-static, but it turns out it is if there is a namespace{ 200 lines above it. -Chris