[PassManager] Add pretty stack entries before P->run() call.

[fhahn]

Summary

This patch adds PrettyStackEntries before running passes. The entries
include the pass name and the IR unit the pass runs on.

The information is used the print additional information when a pass
crashes, including the name and a reference to the IR unit on which it
crashed. This is similar to the behavior of the legacy pass manager.

The improved stack trace now includes:

Stack dump:
0. Program arguments: bin/opt -loop-vectorize -force-vector-width=4 crash.ll

1. Running pass 'ModuleToFunctionPassAdaptor' on module 'crash.ll'
2. Running pass 'LoopVectorizePass' on function '@a'

[myhsu]

I think this is a really neat feature :-)
Please remember to fix the clang-format errors.

llvm/lib/IR/PassManager.cpp

166

I don't think MF->getName() should be commented

[fhahn]

Fix formatting, replace commented out code with explanation why the code doesn't call MF->getName().

[fhahn]

llvm/lib/IR/PassManager.cpp

166

Ah thanks, I forgot about that. Replaced with a comment.

[aeubanks]

Definitely seems good.
This should be added for all adaptors (module->cgscc,module->function,cgscc->function,function->loop) and pass managers (loop/cgscc are implemented separately)
PassManager.h/cpp, LoopPassManager.h/cpp, CGSCCPassManager.h/cpp

Could be tested with the crash pass that was supposed to be landed in https://reviews.llvm.org/D86657, which needs to be relanded

llvm/lib/IR/PassManager.cpp

177–180

We'll either have a Loop, Function, SCC, or Module (MachineFunction doesn't work yet, but I guess it doesn't hurt to add)
See StandardInstrumentations.cpp for similar code

[fhahn]

In D120993#3361927, @aeubanks wrote:

Definitely seems good.
This should be added for all adaptors (module->cgscc,module->function,cgscc->function,function->loop) and pass managers (loop/cgscc are implemented separately)
PassManager.h/cpp, LoopPassManager.h/cpp, CGSCCPassManager.h/cpp

Could be tested with the crash pass that was supposed to be landed in https://reviews.llvm.org/D86657, which needs to be relanded

I added the missing implementations and added crashing passes for all of them.

[aeubanks]

llvm/lib/Passes/PassBuilder.cpp

395 ↗

(On Diff #413809)

CrashingCGSCCPass

llvm/test/Other/crash-module.ll

7 ↗

(On Diff #413809)

this is failing on windows, I don't think we need to be so strict on checking this line

[fhahn]

Fix CGSCC template arg, do not check specific arguments in test.

[fhahn]

llvm/lib/Passes/PassBuilder.cpp

395 ↗

(On Diff #413809)

Thanks, that should bet tied now.

llvm/test/Other/crash-module.ll

7 ↗

(On Diff #413809)

Was the issue that part after Program arguments:? I dropped that from the checks.

[aeubanks]

llvm/lib/Transforms/Scalar/LoopPassManager.cpp

297 ↗

(On Diff #413839)

this might be UAF if L is deleted in the loop pass before we crash (see runAfterPassInvalidated below where we don't reference L for this reason), but given that this is for crash stack traces, it probably doesn't matter

llvm/test/Other/crash-module.ll

7 ↗

(On Diff #413809)

the presubmit said the test was failing, likely due to opt vs opt.exe, but I didn't look too closely
I think this works

[fhahn]

Reduce size of class, relax check lines to account for stack dump not always being printed.

[fhahn]

Rebase

[nikic]

FYI it looks like this isn't as free as I would have expected: http://llvm-compile-time-tracker.com/compare.php?from=338dfcd60f843082bb589b287d890dbd9394eb82&to=128745cc2681c284bc6d0150a319673a6d6e8424&stat=instructions

[fhahn]

In D120993#3370083, @nikic wrote:

FYI it looks like this isn't as free as I would have expected: http://llvm-compile-time-tracker.com/compare.php?from=338dfcd60f843082bb589b287d890dbd9394eb82&to=128745cc2681c284bc6d0150a319673a6d6e8424&stat=instructions

Yeah that's not great. I'll revert it in a bit and check if there's anything in particular that's very expensive.

[aeubanks]

In D120993#3370129, @fhahn wrote:

In D120993#3370083, @nikic wrote:

FYI it looks like this isn't as free as I would have expected: http://llvm-compile-time-tracker.com/compare.php?from=338dfcd60f843082bb589b287d890dbd9394eb82&to=128745cc2681c284bc6d0150a319673a6d6e8424&stat=instructions

Yeah that's not great. I'll revert it in a bit and check if there's anything in particular that's very expensive.

If I had to guess, it might be eagerly retrieving loop names or cgscc names since they can be pretty big if the scc contains a lot of functions or if the loop has a lot of blocks
we could start without the cgscc name and using the function name for loop passes

[myhsu]

In D120993#3370129, @fhahn wrote:

In D120993#3370083, @nikic wrote:

FYI it looks like this isn't as free as I would have expected: http://llvm-compile-time-tracker.com/compare.php?from=338dfcd60f843082bb589b287d890dbd9394eb82&to=128745cc2681c284bc6d0150a319673a6d6e8424&stat=instructions

Yeah that's not great. I'll revert it in a bit and check if there's anything in particular that's very expensive.

My ballpark guess is that the whole SIGINFO printing feature is kinda expensive, since we're forced to load GlobalSigInfoGenerationCounter every time, but the feature is actually never used upstream.

[fhahn]

In D120993#3370296, @aeubanks wrote:

In D120993#3370129, @fhahn wrote:

In D120993#3370083, @nikic wrote:

FYI it looks like this isn't as free as I would have expected: http://llvm-compile-time-tracker.com/compare.php?from=338dfcd60f843082bb589b287d890dbd9394eb82&to=128745cc2681c284bc6d0150a319673a6d6e8424&stat=instructions

Yeah that's not great. I'll revert it in a bit and check if there's anything in particular that's very expensive.

If I had to guess, it might be eagerly retrieving loop names or cgscc names since they can be pretty big if the scc contains a lot of functions or if the loop has a lot of blocks
we could start without the cgscc name and using the function name for loop passes

I tried splitting off the loop & CGSCC handling into separate classes that only take pointers to Loop/ SCC. The compile-time impact is better, but still noticeable (worst config: +0.17% on ReleastThinLTO - https://llvm-compile-time-tracker.com/compare.php?from=af98b0af6705df3859ee3c98525b57025d40d9e8&to=bc328a534f528fbf8d7a37eaf08e70bd78b75896&stat=instructions)

[dexonsmith]

In D120993#3372697, @fhahn wrote:

In D120993#3370296, @aeubanks wrote:

In D120993#3370129, @fhahn wrote:

In D120993#3370083, @nikic wrote:

FYI it looks like this isn't as free as I would have expected: http://llvm-compile-time-tracker.com/compare.php?from=338dfcd60f843082bb589b287d890dbd9394eb82&to=128745cc2681c284bc6d0150a319673a6d6e8424&stat=instructions

Yeah that's not great. I'll revert it in a bit and check if there's anything in particular that's very expensive.

If I had to guess, it might be eagerly retrieving loop names or cgscc names since they can be pretty big if the scc contains a lot of functions or if the loop has a lot of blocks
we could start without the cgscc name and using the function name for loop passes

I tried splitting off the loop & CGSCC handling into separate classes that only take pointers to Loop/ SCC. The compile-time impact is better, but still noticeable (worst config: +0.17% on ReleastThinLTO - https://llvm-compile-time-tracker.com/compare.php?from=af98b0af6705df3859ee3c98525b57025d40d9e8&to=bc328a534f528fbf8d7a37eaf08e70bd78b75896&stat=instructions)

A few ideas that might be worth considering (if they help), if you can't get it as cheap as it should be:

1. Add a -cc1 option to clang for whether to add pretty stack tracing; turn it on by default, but have the driver pass -skip-pretty-stack-traces-in-passes=true to turn it off in release builds (checking NDEBUG, like -discard-value-names=true or whatever it is).
2. Land it but only handle the passes where the granularity is coarse enough that it's "cheap" (e.g., maybe just module/cgscc/function passes?).
3. Both: check for everything by default, but have the driver pass a -cc1 option that turns off the expensive ones.

llvm/include/llvm/IR/PassManager.h

 /// * http://www.youtube.com/watch?v=_BpMYeUFXv8
 /// * http://channel9.msdn.com/Events/GoingNative/2013/Inheritance-Is-The-Base-Class-of-Evil
 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_IR_PASSMANAGER_H
 #define LLVM_IR_PASSMANAGER_H
 
+#include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/TinyPtrVector.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/PassInstrumentation.h"
 #include "llvm/IR/PassManagerInternal.h"
 #include "llvm/Support/TimeProfiler.h"
 #include "llvm/Support/TypeName.h"
 #include <cassert>
 #include <cstring>
 #include <iterator>
 #include <list>
 #include <memory>
 #include <tuple>

 } // namespace detail
 
 // Forward declare the pass instrumentation analysis explicitly queried in
 // generic PassManager code.
 // FIXME: figure out a way to move PassInstrumentationAnalysis into its own
 // header.
 class PassInstrumentationAnalysis;
 
+class MachineFunction;
+
+/// Add stack entry with the pass name and the IR unit it runs on.
+class NewPassManagerPrettyStackEntry : public PrettyStackTraceEntry {
+  StringRef PassName;
+  Module *M = nullptr;
+  MachineFunction *MF = nullptr;
+  Value *V = nullptr;
+
+public:
+  NewPassManagerPrettyStackEntry(StringRef PassName, Module &M)
+    : PassName(PassName), M(&M) {}
+
+  NewPassManagerPrettyStackEntry(StringRef PassName, MachineFunction &MF)
+    : PassName(PassName), MF(&MF) {}
+
+  NewPassManagerPrettyStackEntry(StringRef PassName, Value &V)
+    : PassName(PassName), V(&V) {}
+
+  /// print - Emit information about this stack frame to OS.
+  void print(raw_ostream &OS) const override;
+};
+
 /// Manages a sequence of passes over a particular unit of IR.
 ///
 /// A pass manager contains a sequence of passes to run over a particular unit
 /// of IR (e.g. Functions, Modules). It is itself a valid pass over that unit of
 /// IR, and when run over some given IR will run each of its contained passes in
 /// sequence. Pass managers are the primary and most basic building block of a
 /// pass pipeline.
 ///
for (unsigned Idx = 0, Size = Passes.size(); Idx != Size; ++Idx) {
       // pass, skip its execution completely if asked to (callback returns
       // false).
       if (!PI.runBeforePass<IRUnitT>(*P, IR))
         continue;
 
       PreservedAnalyses PassPA;
       {
         TimeTraceScope TimeScope(P->name(), IR.getName());
+        NewPassManagerPrettyStackEntry StackEntry(P->name(), IR);
         PassPA = P->run(IR, AM, ExtraArgs...);
       }
 
       // Call onto PassInstrumentation's AfterPass callbacks immediately after
       // running the pass.
       PI.runAfterPass<IRUnitT>(*P, IR, PassPA);
 
       // Update the analysis manager as each pass runs and potentially

llvm/lib/IR/PassManager.cpp

for (Function &F : M) {
     // pass, skip its execution completely if asked to (callback returns
     // false).
     if (!PI.runBeforePass<Function>(*Pass, F))
       continue;
 
     PreservedAnalyses PassPA;
     {
       TimeTraceScope TimeScope(Pass->name(), F.getName());
+      NewPassManagerPrettyStackEntry StackEntry(Pass->name(), F);
       PassPA = Pass->run(F, FAM);
     }
 
     PI.runAfterPass(*Pass, F, PassPA);
 
     // We know that the function pass couldn't have invalidated any other
     // function's analyses (that's the contract of a function pass), so
     // directly handle the function analysis manager's invalidation here.
PreservedAnalyses ModuleToFunctionPassAdaptor::run(Module &M,
   PA.preserveSet<AllAnalysesOn<Function>>();
   PA.preserve<FunctionAnalysisManagerModuleProxy>();
   return PA;
 }
 
 AnalysisSetKey CFGAnalyses::SetKey;
 
 AnalysisSetKey PreservedAnalyses::AllAnalysesKey;
+
+void NewPassManagerPrettyStackEntry::print(raw_ostream &OS) const {
+  OS << "Running pass '" << PassName << "'";
+
+  if (M) {
+    OS << " on module '" << M->getModuleIdentifier() << "'.\n";
+    return;
+  }
+
+  if (MF) {
+    OS << " on machine function \n"; //'" << MF->getName() << "'.\n";
+    return;
+  }
+
+  assert(V && "Either M, MF or V must be set");
+
+  OS << " on ";
+  if (isa<Function>(V))
+    OS << "function";
+  else if (isa<BasicBlock>(V))
+    OS << "basic block";
+  else
+    OS << "value";
+
+  OS << " '";
+  V->printAsOperand(OS, /*PrintType=*/false);
+  OS << "'\n";
+}