[Debugify] Port -debugify-each to NewPM

[MaskRay]

Summary

Preemptively switch 2 tests to the new PM

[MaskRay]

Delete an unneeded change. Fix tests

[aeubanks]

Thanks for looking into this!

I was previously worried that modifying the IR in pass instrumentation would be bad, but maybe it's ok? The biggest thing I was worried about was unintentionally invalidating analyses, but it looks like both debugify passes preserve all analyses, so it might be ok.

I'm not so sure of moving the flags into Debugify.cpp since they are debugging tools, but I'm not 100% against it.

DebugifyEachInstrumentation should probably be part of StandardInstrumentations so that all users of StandardInstrumentations will get it for free (tying into moving the flags to not be opt-specific). DebugifyEachInstrumentation::registerCallbacks can check if DebugifyEach is set or not to determine whether or not to actually register callbacks, which is cleaner than checking in NewPMDriver.cpp.

llvm/test/DebugInfo/debugify-export.ll

1–2

We should keep the legacy PM RUN line since its implementation is so different. Then use regex to check that we're running a verifier pass.

[MaskRay]

In D90365#2362581, @aeubanks wrote:

Thanks for looking into this!

I was previously worried that modifying the IR in pass instrumentation would be bad, but maybe it's ok? The biggest thing I was worried about was unintentionally invalidating analyses, but it looks like both debugify passes preserve all analyses, so it might be ok.

Yeah I was a bit concerned as well. The code says PreservedAnalyses::all() so may be it is fine? The dirty part is the const_cast.

I'm not so sure of moving the flags into Debugify.cpp since they are debugging tools, but I'm not 100% against it.

DebugifyEachInstrumentation should probably be part of StandardInstrumentations so that all users of StandardInstrumentations will get it for free (tying into moving the flags to not be opt-specific). DebugifyEachInstrumentation::registerCallbacks can check if DebugifyEach is set or not to determine whether or not to actually register callbacks, which is cleaner than checking in NewPMDriver.cpp.

-debugify-export writes a file. So even if it is moved to StandardInstrumentations, all the users (clang/opt newPM/opt legacyPM) will need to do something with the option.

[aeubanks]

In D90365#2362635, @MaskRay wrote:

In D90365#2362581, @aeubanks wrote:

Thanks for looking into this!

I was previously worried that modifying the IR in pass instrumentation would be bad, but maybe it's ok? The biggest thing I was worried about was unintentionally invalidating analyses, but it looks like both debugify passes preserve all analyses, so it might be ok.

Yeah I was a bit concerned as well. The code says PreservedAnalyses::all() so may be it is fine? The dirty part is the const_cast.

I'm not so sure of moving the flags into Debugify.cpp since they are debugging tools, but I'm not 100% against it.

DebugifyEachInstrumentation should probably be part of StandardInstrumentations so that all users of StandardInstrumentations will get it for free (tying into moving the flags to not be opt-specific). DebugifyEachInstrumentation::registerCallbacks can check if DebugifyEach is set or not to determine whether or not to actually register callbacks, which is cleaner than checking in NewPMDriver.cpp.

-debugify-export writes a file. So even if it is moved to StandardInstrumentations, all the users (clang/opt newPM/opt legacyPM) will need to do something with the option.

Ah I see, that makes sense.

I still think DebugifyEachInstrumentation should be treated in StandardInstrumentations like the others, and DebugifyEachInstrumentation::registerCallbacks can check the flags to enable/disable itself.

[MaskRay]

In D90365#2362878, @aeubanks wrote:

In D90365#2362635, @MaskRay wrote:

In D90365#2362581, @aeubanks wrote:

Thanks for looking into this!

I was previously worried that modifying the IR in pass instrumentation would be bad, but maybe it's ok? The biggest thing I was worried about was unintentionally invalidating analyses, but it looks like both debugify passes preserve all analyses, so it might be ok.

Yeah I was a bit concerned as well. The code says PreservedAnalyses::all() so may be it is fine? The dirty part is the const_cast.

I'm not so sure of moving the flags into Debugify.cpp since they are debugging tools, but I'm not 100% against it.

DebugifyEachInstrumentation should probably be part of StandardInstrumentations so that all users of StandardInstrumentations will get it for free (tying into moving the flags to not be opt-specific). DebugifyEachInstrumentation::registerCallbacks can check if DebugifyEach is set or not to determine whether or not to actually register callbacks, which is cleaner than checking in NewPMDriver.cpp.

-debugify-export writes a file. So even if it is moved to StandardInstrumentations, all the users (clang/opt newPM/opt legacyPM) will need to do something with the option.

Ah I see, that makes sense.

I still think DebugifyEachInstrumentation should be treated in StandardInstrumentations like the others, and DebugifyEachInstrumentation::registerCallbacks can check the flags to enable/disable itself.

Emmm. Since we don't really benefit from having the code StandardInstrumentations (using -debugify-export requires the knowledge about DebugifyStatsMap), having the code in Debugify.cpp is fine?
The implementation is actually all in Debugify.cpp . Having DebugifyEachInstrumentation can make StandardInstrumentations unaware of DebugInfo, which achieves good isolation.
This allows us to make apply* and check* static after legacy PM debugify code in opt.cpp is removed.

[aeubanks]

The issue I have is that the flags can be used anywhere, but each user of StandardInstrumentations has to manually register DebugifyEachInstrumentation if they want to use it. I'd prefer either that the flags be opt-specific and opt can register DebugifyEachInstrumentation, or the flags move to Debugify.cpp so any user of PassBuilder can use them and StandardInstrumentations handle DebugifyEachInstrumentation so that not every user of PassBuilder has to remember to use it.

[MaskRay]

Move -debugify-each and -debugify-export from Debugify.cpp to opt

[MaskRay]

In D90365#2364972, @aeubanks wrote:

The issue I have is that the flags can be used anywhere, but each user of StandardInstrumentations has to manually register DebugifyEachInstrumentation if they want to use it. I'd prefer either that the flags be opt-specific and opt can register DebugifyEachInstrumentation, or the flags move to Debugify.cpp so any user of PassBuilder can use them and StandardInstrumentations handle DebugifyEachInstrumentation so that not every user of PassBuilder has to remember to use it.

Thanks! This argument makes sense. I moved the two cl::opt options from Debugify.cpp to opt since clang cannot really use them (well, probably it could with some efforts, but that shall belong to a separate change).

Since only opt knows about debugify, it makes sense to have a DebugifyEachInstrumentation.

[aeubanks]

lgtm besides comments about tests

llvm/test/DebugInfo/pr37964.ll

1

-passes=gvn instead of -gvn -enable-newpm=1

[MaskRay]

-passes=gvn instead of -gvn -enable-newpm=1

[aeubanks]

There's one more comment on debugify-export.ll, otherwise lgtm

[MaskRay]

debugify-export.ll: test both -enable-new-pm=[01], use -disable-output

llvm/include/llvm/Transforms/Utils/Debugify.h

createCheckDebugifyFunctionPass(bool Strip = false,
                                 llvm::StringRef NameOfWrappedPass = "",
                                 DebugifyStatsMap *StatsMap = nullptr);
 
 struct NewPMCheckDebugifyPass
     : public llvm::PassInfoMixin<NewPMCheckDebugifyPass> {
   llvm::PreservedAnalyses run(llvm::Module &M, llvm::ModuleAnalysisManager &AM);
 };
 
+struct DebugifyEachInstrumentation {
+  DebugifyStatsMap StatsMap;
+
+  void registerCallbacks(PassInstrumentationCallbacks &PIC);
+};
+
 /// DebugifyCustomPassManager wraps each pass with the debugify passes if
 /// needed.
 /// NOTE: We support legacy custom pass manager only.
 /// TODO: Add New PM support for custom pass manager.
 class DebugifyCustomPassManager : public legacy::PassManager {
   DebugifyStatsMap DIStatsMap;
   bool EnableDebugifyEach = false;
 

llvm/lib/Transforms/Utils/Debugify.cpp

bool llvm::applyDebugifyMetadata(
   // Claim that this synthetic debug info is valid.
   StringRef DIVersionKey = "Debug Info Version";
   if (!M.getModuleFlag(DIVersionKey))
     M.addModuleFlag(Module::Warning, DIVersionKey, DEBUG_METADATA_VERSION);
 
   return true;
 }
 
+static bool applyDebugify(Function &F) {
+  Module &M = *F.getParent();
+  auto FuncIt = F.getIterator();
+  return applyDebugifyMetadata(M, make_range(FuncIt, std::next(FuncIt)),
+                               "FunctionDebugify: ", /*ApplyToMF=*/nullptr);
+}
+
+static bool applyDebugify(Module &M) {
+  return applyDebugifyMetadata(M, M.functions(),
+                               "ModuleDebugify: ", /*ApplyToMF=*/nullptr);
+}
+
 bool llvm::stripDebugifyMetadata(Module &M) {
   bool Changed = false;
 
   // Remove the llvm.debugify module-level named metadata.
   NamedMDNode *DebugifyMD = M.getNamedMetadata("llvm.debugify");
   if (DebugifyMD) {
     M.eraseNamedMetadata(DebugifyMD);
     Changed = true;
if (Strip)
     return stripDebugifyMetadata(M);
 
   return false;
 }
 
 /// ModulePass for attaching synthetic debug info to everything, used with the
 /// legacy module pass manager.
 struct DebugifyModulePass : public ModulePass {
-  bool runOnModule(Module &M) override {
-    return applyDebugifyMetadata(M, M.functions(),
-                                 "ModuleDebugify: ", /*ApplyToMF*/ nullptr);
-  }
+  bool runOnModule(Module &M) override { return applyDebugify(M); }
 
   DebugifyModulePass() : ModulePass(ID) {}
 
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesAll();
   }
 
   static char ID; // Pass identification.
 };
 
 /// FunctionPass for attaching synthetic debug info to instructions within a
 /// single function, used with the legacy module pass manager.
 struct DebugifyFunctionPass : public FunctionPass {
-  bool runOnFunction(Function &F) override {
-    Module &M = *F.getParent();
-    auto FuncIt = F.getIterator();
-    return applyDebugifyMetadata(M, make_range(FuncIt, std::next(FuncIt)),
-                                 "FunctionDebugify: ", /*ApplyToMF*/ nullptr);
-  }
+  bool runOnFunction(Function &F) override { return applyDebugify(F); }
 
   DebugifyFunctionPass() : FunctionPass(ID) {}
 
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesAll();
   }
 
   static char ID; // Pass identification.

 
 PreservedAnalyses NewPMCheckDebugifyPass::run(Module &M,
                                               ModuleAnalysisManager &) {
   checkDebugifyMetadata(M, M.functions(), "", "CheckModuleDebugify", false,
                         nullptr);
   return PreservedAnalyses::all();
 }
 
+void DebugifyEachInstrumentation::registerCallbacks(
+    PassInstrumentationCallbacks &PIC) {
+  PIC.registerBeforeNonSkippedPassCallback([](StringRef P, Any IR) {
+    if (any_isa<const Function *>(IR))
+      applyDebugify(*const_cast<Function *>(any_cast<const Function *>(IR)));
+    else if (any_isa<const Module *>(IR))
+      applyDebugify(*const_cast<Module *>(any_cast<const Module *>(IR)));
+  });
+  PIC.registerAfterPassCallback([this](StringRef P, Any IR,
+                                       const PreservedAnalyses &PassPA) {
+    if (any_isa<const Function *>(IR)) {
+      auto &F = *const_cast<Function *>(any_cast<const Function *>(IR));
+      Module &M = *F.getParent();
+      auto It = F.getIterator();
+      checkDebugifyMetadata(M, make_range(It, std::next(It)), P,
+                            "CheckFunctionDebugify", /*Strip=*/true, &StatsMap);
+    } else if (any_isa<const Module *>(IR)) {
+      auto &M = *const_cast<Module *>(any_cast<const Module *>(IR));
+      checkDebugifyMetadata(M, M.functions(), P, "CheckModuleDebugify",
+                            /*Strip=*/true, &StatsMap);
+    }
+  });
+}
+
 char DebugifyModulePass::ID = 0;
 static RegisterPass<DebugifyModulePass> DM("debugify",
                                            "Attach debug info to everything");
 
 char CheckDebugifyModulePass::ID = 0;
 static RegisterPass<CheckDebugifyModulePass>
     CDM("check-debugify", "Check debug info from -debugify");
 
 char DebugifyFunctionPass::ID = 0;
 static RegisterPass<DebugifyFunctionPass> DF("debugify-function",
                                              "Attach debug info to a function");
 
 char CheckDebugifyFunctionPass::ID = 0;
 static RegisterPass<CheckDebugifyFunctionPass>
     CDF("check-debugify-function", "Check debug info from -debugify-function");

llvm/test/DebugInfo/debugify-each.ll

 ; RUN: opt -debugify-each -O3 -S -o /dev/null < %s 2> %t
 ; RUN: FileCheck %s -input-file=%t -check-prefix=MODULE-PASS
 ; RUN: FileCheck %s -input-file=%t -check-prefix=FUNCTION-PASS
+; RUN: opt -disable-output -debugify-each -passes='default<O3>' %s 2> %t
+; RUN: FileCheck %s -input-file=%t -check-prefix=MODULE-PASS
+; RUN: FileCheck %s -input-file=%t -check-prefix=FUNCTION-PASS
 
 ; RUN: opt -enable-debugify -debugify-each -O3 -S -o /dev/null < %s 2> %t
 ; RUN: FileCheck %s -input-file=%t -check-prefix=MODULE-PASS
 ; RUN: FileCheck %s -input-file=%t -check-prefix=FUNCTION-PASS
 
 ; RUN: opt -debugify-each -instrprof -instrprof -sroa -sccp -S -o /dev/null < %s 2> %t
 ; RUN: FileCheck %s -input-file=%t -check-prefix=MODULE-PASS
 ; RUN: FileCheck %s -input-file=%t -check-prefix=FUNCTION-PASS

llvm/test/DebugInfo/debugify-export.ll

-; RUN: opt < %s -debugify-each -debugify-quiet -debugify-export - -o /dev/null | FileCheck %s
+; RUN: opt %s -disable-output -debugify-each -debugify-quiet -debugify-export - -enable-new-pm=0 | FileCheck %s
+; RUN: opt %s -disable-output -debugify-each -debugify-quiet -debugify-export - -enable-new-pm=1 | FileCheck %s
 
 ; CHECK: Pass Name
 ; CHECK-SAME: # of missing debug values
 ; CHECK-SAME: # of missing locations
 ; CHECK-SAME: Missing/Expected value ratio
 ; CHECK-SAME: Missing/Expected location ratio
 
-; CHECK: Module Verifier
+; CHECK:      {{Module Verifier|VerifierPass}}
 ; CHECK-SAME: 0,0,0.000000e+00,0.000000e+00
 
 define void @foo() {
   ret void
 }

llvm/test/DebugInfo/pr37964.ll

-; RUN: opt -disable-output -debugify-each -gvn < %s 2>&1 | FileCheck %s
+; RUN: opt -disable-output -debugify-each -passes=gvn < %s 2>&1 | FileCheck %s
 
 ; CHECK-NOT: ERROR: Instruction with empty DebugLoc in function _Z3bazv --  {{%.*}} = phi
-; CHECK: CheckFunctionDebugify [Global Value Numbering]: PASS
+; CHECK: CheckFunctionDebugify [GVN]: PASS
 
 @foo = dso_local local_unnamed_addr global i32 0, align 4
 @x = global i8 17
 
 define dso_local void @_Z3bazv() local_unnamed_addr #0 {
 entry:
   br label %for.cond
 

llvm/tools/opt/NewPMDriver.h

 /// opt.cpp.
 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TOOLS_OPT_NEWPMDRIVER_H
 #define LLVM_TOOLS_OPT_NEWPMDRIVER_H
 
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/Support/CommandLine.h"
 
 namespace llvm {
 class StringRef;
 class Module;
 class TargetMachine;
 class ToolOutputFile;
 class TargetLibraryInfoImpl;
 
+extern cl::opt<bool> DebugifyEach;
+extern cl::opt<std::string> DebugifyExport;
+
 namespace opt_tool {
 enum OutputKind {
   OK_NoOutput,
   OK_OutputAssembly,
   OK_OutputBitcode,
   OK_OutputThinLTOBitcode,
 };
 enum VerifierKind {

llvm/tools/opt/NewPMDriver.cpp

 #include "llvm/IR/IRPrintingPasses.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/IR/Verifier.h"
 #include "llvm/Passes/PassBuilder.h"
 #include "llvm/Passes/PassPlugin.h"
 #include "llvm/Passes/StandardInstrumentations.h"
-#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ToolOutputFile.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Transforms/IPO/ThinLTOBitcodeWriter.h"
 #include "llvm/Transforms/Instrumentation/AddressSanitizer.h"
 #include "llvm/Transforms/Scalar/LoopPassManager.h"
 #include "llvm/Transforms/Utils/Debugify.h"
 
 using namespace llvm;
 using namespace opt_tool;
 
+namespace llvm {
+cl::opt<bool> DebugifyEach(
+    "debugify-each",
+    cl::desc("Start each pass with debugify and end it with check-debugify"));
+
+cl::opt<std::string>
+    DebugifyExport("debugify-export",
+                   cl::desc("Export per-pass debugify statistics to this file"),
+                   cl::value_desc("filename"));
+} // namespace llvm
+
 static cl::opt<bool>
     DebugPM("debug-pass-manager", cl::Hidden,
             cl::desc("Print pass management debugging information"));
 
 static cl::list<std::string>
     PassPlugins("load-pass-plugin",
                 cl::desc("Load passes from plugin library"));
 
if (CSPGOKindFlag == CSInstrGen) {
       if (!P)
         errs() << "CSInstrUse needs to be together with InstrUse";
       P->CSAction = PGOOptions::CSIRUse;
     }
   }
   PassInstrumentationCallbacks PIC;
   StandardInstrumentations SI(DebugPM, VerifyEachPass);
   SI.registerCallbacks(PIC);
+  DebugifyEachInstrumentation Debugify;
+  if (DebugifyEach)
+    Debugify.registerCallbacks(PIC);
 
   PipelineTuningOptions PTO;
   // LoopUnrolling defaults on to true and DisableLoopUnrolling is initialized
   // to false above so we shouldn't necessarily need to check whether or not the
   // option has been enabled.
   PTO.LoopUnrolling = !DisableLoopUnrolling;
   PTO.Coroutines = Coroutines;
   PassBuilder PB(DebugPM, TM, PTO, P, &PIC);
if (OK != OK_NoOutput) {
     Out->keep();
     if (OK == OK_OutputThinLTOBitcode && ThinLTOLinkOut)
       ThinLTOLinkOut->keep();
   }
 
   if (OptRemarkFile)
     OptRemarkFile->keep();
 
+  if (DebugifyEach && !DebugifyExport.empty())
+    exportDebugifyStats(DebugifyExport, Debugify.StatsMap);
+
   return true;
 }

llvm/tools/opt/opt.cpp

 static cl::opt<bool>
 AnalyzeOnly("analyze", cl::desc("Only perform analysis, no optimization"));
 
 static cl::opt<bool> EnableDebugify(
     "enable-debugify",
     cl::desc(
         "Start the pipeline with debugify and end it with check-debugify"));
 
-static cl::opt<bool> DebugifyEach(
-    "debugify-each",
-    cl::desc(
-        "Start each pass with debugify and end it with check-debugify"));
-
-static cl::opt<std::string>
-    DebugifyExport("debugify-export",
-                   cl::desc("Export per-pass debugify statistics to this file"),
-                   cl::value_desc("filename"), cl::init(""));
-
 static cl::opt<bool>
 PrintBreakpoints("print-breakpoints-for-testing",
                  cl::desc("Print select breakpoints location for testing"));
 
 static cl::opt<std::string> ClDataLayout("data-layout",
                                          cl::desc("data layout string to use"),
                                          cl::value_desc("layout-string"),
                                          cl::init(""));