Add a -verify-after-all option

[sanjoy]

Summary

When investigating PR27997 I notied that there is no easy way in opt or
clang to verify the IR after each transform pass. There is a
-verify-each option in opt, but

• When running opt -O3 it appends the verifier pass after the whole -O3 pipeline, not after each individual transform pass
• It is in opt so isn't accessible from clang via -mllvm

With -verify-after-all, triaging bugs like PR27997 should become
easier, since we'd be able to run something like `clang -O3 -mllvm
-verify-after-all -mllvm -print-after-all` to quickly discover which
pass broke SSA, if any.

Depends on D21004

[davide]

This is very good. In fact, I have a patch that accomplish exactly the same locally (but I was always too lazy to upstream it). In my patch, I just used -verify-after, and I have preference for that as it's shorter, but I don't want to start a bike shed here, so up to you.

[mehdi_amini]

I'd rather keep the name "verify-each" or "verify-after-each" (the "after-all" sounds odd to me) and nuke the opt option which seems redundant now.

[davide]

Oh, also, do you have any (easy way) to add a test for this option?

[sanjoy]

In D21005#449347, @mehdi_amini wrote:

I'd rather keep the name "verify-each" or "verify-after-each" (the "after-all" sounds odd to me) and nuke the opt option which seems redundant now.

I named this keeping consistent with the similar -print-after-all option (in the future we might even want a -verify-after=... option like we have for -print-after=... today).

I do plan to nuke the opt -verify-each once this change is in.

[sanjoy]

In D21005#449348, @davide wrote:

Oh, also, do you have any (easy way) to add a test for this option?

I can add a test that makes sure the function exists and is supported by opt (do you want me to do that?), but I don't see how I can test for a bad transform without adding a deliberately broken pass (which seems overkill). But suggestions are welcome.

[davide]

In D21005#449357, @sanjoy wrote:

In D21005#449348, @davide wrote:

Oh, also, do you have any (easy way) to add a test for this option?

I can add a test that makes sure the function exists and is supported by opt (do you want me to do that?), but I don't see how I can test for a bad transform without adding a deliberately broken pass (which seems overkill). But suggestions are welcome.

Something that tests the basic functionality works is fine. I really don't think adding a broken pass is needed.

[mehdi_amini]

-dump-pass=Structure could be a good test. There are already test that checks the pass pipeline that may be extended to test this option I think.

[sanjoy]

This patch isn't quite right as is -- it breaks out of a loop pass manager, i.e. if I had e.g. -indvars -loop-rotate now I will have -indvars < verify function > -loop-rotate with the < verify function > in the middle running outside the LoopPass; and the -loop-rotate and -indvars running in different loop pass managers when earlier they'd run in the same loop pass manager. I think this can be quite misleading since it will change the optimizations that fire when -verify-after-all is enabled.

I think I'll have to have something like createPrinterPass so that the above does not happen.

[davide]

This is very good. In fact, I have a patch that accomplish exactly the same locally (but I was always too lazy to upstream it). In my patch, I just used -verify-after, and I have preference for that as it's shorter, but I don't want to start a bike shed here, so up to you.

In D21005#449389, @sanjoy wrote:

This patch isn't quite right as is -- it breaks out of a loop pass manager, i.e. if I had e.g. -indvars -loop-rotate now I will have -indvars < verify function > -loop-rotate with the < verify function > in the middle running outside the LoopPass; and the -loop-rotate and -indvars running in different loop pass managers when earlier they'd run in the same loop pass manager. I think this can be quite misleading since it will change the optimizations that fire when -verify-after-all is enabled.

I think I'll have to have something like createPrinterPass so that the above does not happen.

I just used this for function passes so I never noticed the problem, but yes, I agree it can happen.
Can you please elaborate on how you plan to use createPrinterPass ?

[sanjoy]

In D21005#449402, @davide wrote:

I just used this for function passes so I never noticed the problem, but yes, I agree it can happen.
Can you please elaborate on how you plan to use createPrinterPass ?

I don't plan to use createPrinterPass, but add a virtual function similar to it as PassManager::createVerifierPass that creates and returns a verifier pass of the right type (i.e. ModulePass, FunctionPass or LoopPass).

[davide]

In D21005#449404, @sanjoy wrote:

In D21005#449402, @davide wrote:

I just used this for function passes so I never noticed the problem, but yes, I agree it can happen.
Can you please elaborate on how you plan to use createPrinterPass ?

I don't plan to use createPrinterPass, but add a virtual function similar to it as PassManager::createVerifierPass that creates and returns a verifier pass of the right type (i.e. ModulePass, FunctionPass or LoopPass).

Thanks. I assume this will also extend to CGSCCPass when one will be available, right?
Anyway, I'll defer the judgement of this patch to Chandler as I don't (yet) understand all the interactions between the PMs, clearly =)

[chandlerc]

(FWIW, +1 to the idea of using the virtual function to get the correct kind of verifier pass)

[sanjoy]

I'm not working on this actively now, so abandoning for now. Will reopen if that changes.

lib/IR/LegacyPassManager.cpp

 
 
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/IRPrintingPasses.h"
 #include "llvm/IR/LegacyPassManager.h"
 #include "llvm/IR/LegacyPassManagers.h"
 #include "llvm/IR/LegacyPassNameParser.h"
 #include "llvm/IR/Module.h"
+#include "llvm/IR/Verifier.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/Mutex.h"
 #include "llvm/Support/TimeValue.h"
 #include "llvm/Support/Timer.h"
 #include "llvm/Support/raw_ostream.h"

 PrintBeforeAll("print-before-all",
                llvm::cl::desc("Print IR before each pass"),
                cl::init(false));
 static cl::opt<bool>
 PrintAfterAll("print-after-all",
               llvm::cl::desc("Print IR after each pass"),
               cl::init(false));
 
+static cl::opt<bool> VerifyAfterAll(
+    "verify-after-all",
+    llvm::cl::desc(
+        "Verify the IR after each pass (and crash if the IR does not verify)"),
+    cl::init(false));
+
 static cl::list<std::string>
     PrintFuncsList("filter-print-funcs", cl::value_desc("function names"),
                    cl::desc("Only print IR for functions whose name "
                             "match this for all print-[before|after][-all] "
                             "options"),
                    cl::CommaSeparated);
 
 /// This is a helper to determine whether to print IR before or
void PMTopLevelManager::schedulePass(Pass *P) {
   // Add the requested pass to the best available pass manager.
   P->assignPassManager(activeStack, getTopLevelPassManagerType());
 
   if (PI && !PI->isAnalysis() && ShouldPrintAfterPass(PI)) {
     Pass *PP = P->createPrinterPass(
       dbgs(), std::string("*** IR Dump After ") + P->getPassName() + " ***");
     PP->assignPassManager(activeStack, getTopLevelPassManagerType());
   }
+
+  if (PI && !PI->isAnalysis() && VerifyAfterAll) {
+    createVerifierPass()->assignPassManager(activeStack,
+                                            getTopLevelPassManagerType());
+  }
 }
 
 /// Find the pass that implements Analysis AID. Search immutable
 /// passes and all pass managers. If desired pass is not found
 /// then return NULL.
 Pass *PMTopLevelManager::findAnalysisPass(AnalysisID AID) {
   // For immutable passes we have a direct mapping from ID to pass, so check
   // that first.