[LLVMdev] [PATCH] Before/After IR Dumps
David Greene
Passes. It adds the following command-line options:
-print-before=<pass-option>
-print-after=<pass-option>
-print-before-all
-print-after-all
Use it like this:
llc -debug -print-before=linearscan-regalloc file.bc -o file.s
-help will print the pass options available to -print-before and -print-after.
The patches add support to clang, llvm and llvm-gcc, respectively.
We use this extensively to debug Passes by examining IR before and after
they run.
Please review for commit and comment. It would be great to get this into the
2.7 release.
Thanks!
-Dave
Chris Lattner
> This set of patches adds support for dumping IR before or after specified
> Passes. It adds the following command-line options:
>
> -print-before=<pass-option>
> -print-after=<pass-option>
> -print-before-all
> -print-after-all
This patch looks very invasive for such a simple thing, isn't there a better way?
Also, please please please stay in 80 columns.
-Chris
>
> Use it like this:
>
> llc -debug -print-before=linearscan-regalloc file.bc -o file.s
>
> -help will print the pass options available to -print-before and -print-after.
>
> The patches add support to clang, llvm and llvm-gcc, respectively.
>
> We use this extensively to debug Passes by examining IR before and after
> they run.
>
> Please review for commit and comment. It would be great to get this into the
> 2.7 release.
>
> Thanks!
>
> -Dave
>
> <clang-beforeafter.patch><llvm-beforeafter.patch><llvm-gcc-beforeafter.patch>_______________________________________________
> LLVM Developers mailing list
> LLV...@cs.uiuc.edu http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
_______________________________________________
LLVM Developers mailing list
LLV...@cs.uiuc.edu http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
David Greene
> On Mar 2, 2010, at 1:45 PM, David Greene wrote:
> > This set of patches adds support for dumping IR before or after specified
> > Passes. It adds the following command-line options:
> >
> > -print-before=<pass-option>
> > -print-after=<pass-option>
> > -print-before-all
> > -print-after-all
>
> This patch looks very invasive for such a simple thing, isn't there a
> better way?
Possibly. What specifically do you object to? The main problem is that
one needs different printers at different stages of the compiler:
ModulePrinter, FunctionPrinter and MachineFunctionPrinter. It makes
sense to write the code once and parameterize it on the printer type.
I see addPass<> as the most "invasive" in the sense that the patch
changes almost every call of PM.addPass(..) to addPass<>(PM, ...).
That's a consequence of code sharing.
I wanted a command-line interface that is intuitive. This requires that
passes show up in the print option as a result of registering them with
the PassManager.
I could get rid of the "Previous" parameter to addPass<> if we don't mind
duplicate output in the case where we want something printed after
Pass A and before Pass B when Pass B immediately follows Pass A.
I'm happy to revisit design decisions if you let me know what you want to
see changed.
> Also, please please please stay in 80 columns.
Ah, thought I had fixed all of that. I will do another scan.
-Dave
David Greene
> I could get rid of the "Previous" parameter to addPass<> if we don't mind
> duplicate output in the case where we want something printed after
> Pass A and before Pass B when Pass B immediately follows Pass A.
I could get rid of "LastPass" as well if we don't mind duplicate output. This
would remove the changes to the addPasses... interfaces which would
reduce the impact of the patch. But the main cause of the widespread changes
is changing PM.addPass(...) to addPass<>(PM,...) and I can't think of a way
to avoid that change offhand. Those lines have to change in some way to
tell some entity which kind of printer is needed.
Chris Lattner
> On Thursday 04 March 2010 01:54:55 Chris Lattner wrote:
>> On Mar 2, 2010, at 1:45 PM, David Greene wrote:
>>> This set of patches adds support for dumping IR before or after specified
>>> Passes. It adds the following command-line options:
>>>
>>> -print-before=<pass-option>
>>> -print-after=<pass-option>
>>> -print-before-all
>>> -print-after-all
>>
>> This patch looks very invasive for such a simple thing, isn't there a
>> better way?
>
> Possibly. What specifically do you object to? The main problem is that
> one needs different printers at different stages of the compiler:
> ModulePrinter, FunctionPrinter and MachineFunctionPrinter. It makes
> sense to write the code once and parameterize it on the printer type.
>
> I see addPass<> as the most "invasive" in the sense that the patch
> changes almost every call of PM.addPass(..) to addPass<>(PM, ...).
> That's a consequence of code sharing.
>
> I wanted a command-line interface that is intuitive. This requires that
> passes show up in the print option as a result of registering them with
> the PassManager.
I'd expect the pass manager to manage all of this. To get the appropriate printer for a pass, it would invoke a virtual method on the pass itself, e.g.:
P->getPrinterPass();
which would return a ModulePrinter, FunctionPrinter, MachineFunctionPrinter etc depending on the pass.
-Chris
David Greene
> I'd expect the pass manager to manage all of this. To get the appropriate
> printer for a pass, it would invoke a virtual method on the pass itself,
> e.g.:
>
> P->getPrinterPass();
>
> which would return a ModulePrinter, FunctionPrinter, MachineFunctionPrinter
> etc depending on the pass.
Ok, that makes sense. I will rework the patch.
-Dave
David Greene
> I'm happy to revisit design decisions if you let me know what you want to
> see changed.
Here's a rework using PassManager as Chris suggested. Comments?
-Dave
David Greene
Ping?
David A. Greene
Kalle Raiskila
> 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).
But the file MachineFunctionPrinterPass.h is still missing.
kalle
David Greene
> 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.
> But the file MachineFunctionPrinterPass.h is still missing.
Thanks, here's an updated version.
-Dave
Chris Lattner
> 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
David Greene
> 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
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.
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
David Greene
Ping?
David Greene
Chris Lattner
-Chris
Chris Lattner
>> +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
Chris Lattner
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.
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.
+ 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.
+// 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 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.
+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 :)
Your patch doesn't include the file that defines createMachineFunctionPrinterPass, but I assume it's fine.
After making these changes, please test the resultant patch against mainline with just the patch applied and commit if it looks ok. Thanks David,
-Chris
David Greene
> > 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
Chris Lattner
On Mar 29, 2010, at 8:36 AM, David Greene wrote:
> 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?
StringRef has a trivial inline constructor. I'm not sure what you're worried about here.
> The alternative is to move all interfaces over to StringRef but I don't think
> that's in the scope of this patch.
As I said, I don't strongly care.
>> 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.
Sure.
>> 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.
I think it's just missing from the patch you sent, maybe it didn't get svn added.
>
>> 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.
Thanks!
-Chris
Jakob Stoklund Olesen
On Mar 2, 2010, at 1:45 PM, David Greene wrote:
> -print-before-all
> -print-after-all
What's the difference between printing before all passes and printing after all passes?
/jakob
David Greene
> On Mar 2, 2010, at 1:45 PM, David Greene wrote:
> > -print-before-all
> > -print-after-all
>
> What's the difference between printing before all passes and printing after
> all passes?
So you want an IR dump before the very first pass? Do you want one after the
very last pass?
Usually I use both so I get before/after dumps for each pass.
-Dave
David Greene
> >> 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.
>
> I think it's just missing from the patch you sent, maybe it didn't get svn
> added.
Yep, that's what happened. It's added now.
> >> 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.
>
> Thanks!
How should we handle the llvm-gcc and clang changes? They are trivial
but I don't have commit access to either repository. Should I post them here
to be applied? There will be a window where things won't build between
when the first of these patches is applied and when all of them are applied.
-Dave
Chris Lattner
On Mar 29, 2010, at 11:14 AM, David Greene wrote:
>>>>
>>>> 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.
>>
>> Thanks!
>
> How should we handle the llvm-gcc and clang changes?
What changes are required?
> They are trivial
> but I don't have commit access to either repository. Should I post them here
> to be applied? There will be a window where things won't build between
> when the first of these patches is applied and when all of them are applied.
You have access to all llvm repositories, including clang and llvm-gcc.
-Chris
Eric Christopher
On Mar 29, 2010, at 10:47 AM, Jakob Stoklund Olesen wrote:
>
> On Mar 2, 2010, at 1:45 PM, David Greene wrote:
>
>> -print-before-all
>> -print-after-all
>
> What's the difference between printing before all passes and printing after all passes?
Makes it easier to find what you're looking for probably. Matter of semantics but if I'm looking for something before I go into the 3rd inst-combine pass it's probably easier to look for instcombine-3.before than licm.after :)
-eric
Jakob Stoklund Olesen
On Mar 29, 2010, at 11:12 AM, David Greene wrote:
> On Monday 29 March 2010 12:47:30 Jakob Stoklund Olesen wrote:
>> On Mar 2, 2010, at 1:45 PM, David Greene wrote:
>>> -print-before-all
>>> -print-after-all
>>
>> What's the difference between printing before all passes and printing after
>> all passes?
>
> So you want an IR dump before the very first pass? Do you want one after the
> very last pass?
>
> Usually I use both so I get before/after dumps for each pass.
I see that you are already collapsing before/after dumps so there is a most one dump between passes. Would it make sense to merge these two options into one?
David Greene
> I see that you are already collapsing before/after dumps so there is a most
> one dump between passes. Would it make sense to merge these two options
> into one?
Actually, they're not merged right now. That was one of the things lost
during review. But I think I could add it back in a follow-up patch.
It might make sense to merge the options. I don't think I've ever used
them separately. Again, I'd do that as a follow-up.
-Dave
David Greene
> > How should we handle the llvm-gcc and clang changes?
>
> What changes are required?
Attached. They are trivial.
> You have access to all llvm repositories, including clang and llvm-gcc.
Ok. Is there are preferred order to commit things? I think I'm ready to go.
-Dave
Chris Lattner
On Mar 30, 2010, at 12:27 PM, David Greene wrote:
> On Monday 29 March 2010 13:20:37 Chris Lattner wrote:
>
>>> How should we handle the llvm-gcc and clang changes?
>>
>> What changes are required?
>
> Attached. They are trivial.
Why not just change the order of the arguments to createPrintModulePass, and make the string default to ""?
David Greene
> On Mar 30, 2010, at 12:27 PM, David Greene wrote:
> > On Monday 29 March 2010 13:20:37 Chris Lattner wrote:
> >>> How should we handle the llvm-gcc and clang changes?
> >>
> >> What changes are required?
> >
> > Attached. They are trivial.
>
> Why not just change the order of the arguments to createPrintModulePass,
> and make the string default to ""?
1. I wanted to be consistent with createPrintFunctionPass.
2. Default arguments are often evil. Maybe not in this case but I didn't want
to encourage their use. :)
That said, I'll go ahead and make this change to reduce the impact of this.
-Dave