Fix IRPrinting bug

[tyb0807]

Summary

Hello all,

Currently, all IRPrinting passes are not considered as analysis, so when -print-after/before-all option enabled, we'll print the IR twice in a row. Note that we do not have this issue with BitcodeWriter pass, since it is already an analysis pass. For example, with:

$ opt -O1 -print-after-all -debug-pass=Arguments -o /dev/null

we got

Pass Arguments:  -targetlibinfo -print-module ... -write-bitcode

whereas with

$ opt -O1 -print-after-all -debug-pass=Arguments -S -o /dev/null

we got

Pass Arguments:  -targetlibinfo -print-module ... -print-module -print-module

This patch will remove the second -print-module pass from the pipeline.

[dexonsmith]

This seems like a reasonable thing to do. Please add tests to check the output of both pass managers.

[tyb0807]

Thanks for taking a look at this. However, IIRC, the new pass manager does not have print-before/after(-all) mode yet. That is the next thing that I want to do (if you agree, of course)

[dexonsmith]

I'm not sure this technique will work in the new pass manager, since analyses are run on-demand IIRC. I wonder if it would be better to leave these as passes, and implement -print-after-all by inserting -print-module/etc. passes into the pipeline as the pipeline is being constructed. You could avoid adding extra -print-module statements in cases where that pass already exists.

In any case you'll need a test for what you have changed.

[tyb0807]

No don't get me wrong, this patch is supposed to fix the issue in the LegacyPassManager, and I do not intend to change anything about the new pass manager in this patch.

As to implementing print-after-all in the new pass manager, I do not know what should be the best way to do yet... Your idea seems to be a good solution.

Anyway, tests added for this patch.

[dexonsmith]

In D48723#1147935, @tyb0807 wrote:

No don't get me wrong, this patch is supposed to fix the issue in the LegacyPassManager, and I do not intend to change anything about the new pass manager in this patch.

As to implementing print-after-all in the new pass manager, I do not know what should be the best way to do yet... Your idea seems to be a good solution.

I think it would be oddly divergent if these are analyses in one pass manager and not the other, which is why I'm pushing to understand what we'll do for both.

test/Other/printer.ll

11

Can you add a positive check that Print Module IR is actually being called?

[tyb0807]

Addressed review comment.

[tyb0807]

I think it would be oddly divergent if these are analyses in one pass manager and not the other, which is why I'm pushing to understand what we'll do for both.

Hmmmm, in the new pass manager, it seems to me that passes are no longer inherited from Pass object, thus no longer classified as analyses or transformations (please correct me if I'm wrong)

Anyway, if this is not the right way to do, then we should change BitcodeWriter to a non-analysis pass I guess? There should be no difference between these 2 passes on whether it is an analysis or not IMHO

[tyb0807]

Is it better now?

[dexonsmith]

In D48723#1148297, @tyb0807 wrote:

Anyway, if this is not the right way to do, then we should change BitcodeWriter to a non-analysis pass I guess? There should be no difference between these 2 passes on whether it is an analysis or not IMHO

That's a good point. I'm fine with this then. LGTM.

[tyb0807]

Thank you for reviewing this. Unfortunately I do not have the commit access, can you do it on my behalf please?
Thank you again

[tyb0807]

Friendly ping!

[dexonsmith]

Committed in r336869!

lib/IR/IRPrintingPasses.cpp

public:
 
   StringRef getPassName() const override { return "Print BasicBlock IR"; }
 };
 
 }
 
 char PrintModulePassWrapper::ID = 0;
 INITIALIZE_PASS(PrintModulePassWrapper, "print-module",
-                "Print module to stderr", false, false)
+                "Print module to stderr", false, true)
 char PrintFunctionPassWrapper::ID = 0;
 INITIALIZE_PASS(PrintFunctionPassWrapper, "print-function",
-                "Print function to stderr", false, false)
+                "Print function to stderr", false, true)
 char PrintBasicBlockPass::ID = 0;
 INITIALIZE_PASS(PrintBasicBlockPass, "print-bb", "Print BB to stderr", false,
-                false)
+                true)
 
 ModulePass *llvm::createPrintModulePass(llvm::raw_ostream &OS,
                                         const std::string &Banner,
                                         bool ShouldPreserveUseListOrder) {
   return new PrintModulePassWrapper(OS, Banner, ShouldPreserveUseListOrder);
 }
 
 FunctionPass *llvm::createPrintFunctionPass(llvm::raw_ostream &OS,

test/Other/printer.ll

+; RUN: opt -mem2reg -instcombine -print-after-all -S < %s 2>&1 | FileCheck %s
+define void @tester(){
+  ret void
+}
+
+define void @foo(){
+  ret void
+}
+
+;CHECK: IR Dump After Promote Memory to Register
+;CHECK: IR Dump After Combine redundant instructions
+;CHECK: IR Dump After Module Verifier
+;CHECK-NOT: IR Dump After Print Module IR