[PM] Introduce a devirtualization iteration layer for the new PM.

[chandlerc]

Summary

This is an orthogonal and separated layer instead of being embedded
inside the pass manager. While it adds a small amount of complexity, it
is fairly minimal and the composability and control seems worth the
cost.

The logic for this ends up being nicely isolated and targeted. It should
be easy to experiment with different iteration strategies wrapped around
the CGSCC bottom-up walk using this kind of facility.

The mechanism used to track devirtualization is the simplest one I came
up with. I think it handles most of the cases the existing iteration
machinery handles, but I haven't done a *very* in depth analysis. It
does however match the basic intended semantics, and we can tweak or
tune its exact behavior incrementally as necessary. One thing that we
may want to revisit is freshly building the value handle set on each
iteration. While I don't think this will be a significant cost (it is
strictly fewer value handles but more churn of value handes than the old
call graph), it is conceivable that we'll want a somewhat more clever
tracking mechanism. My hope is to layer that on as a follow up patch
with data supporting any implementation complexity it adds.

Note, I haven't written a lot of tests for this yet, but it already fixes the
FIXME in the existing test that motivates it. I'll write more detailed tests
for it, but wanted to go ahead and post the core idea behind this.

Depends on D21462

[silvas]

include/llvm/Analysis/CGSCCPassManager.h

528

If you update C in this assignment, where do you call AM.invalidate(...) on the original C? (this is needed to downward invalidate any function analyses that might depend on SCC analyses on the original C). Is this something you expect the SCC transformation being run by DevirtIteratingWrapper itself to do? It would be nice to see a unittest exhibiting this.

[chandlerc]

include/llvm/Analysis/CGSCCPassManager.h

528

The pass is required to preserve the call graph (that's part of passing it by argument) and if it updates it to reflect that in the UpdateResult and invalidate the analysis manager.

But all of this should really be spelled out in the prior patch that switches to this API, I'll update that patch with better documentation of this fact.

I certainly want to add verification of these properties as well, but I'd like to do that in a follow-up patch rather than put more into the existing patches out for review. (There is some verification already, but with some API changes and work I think we can get more verification into place.)

[chandlerc]

Update to use a new name (stemming from a conversation with Mehdi on IRC) and
include a more interesting and thorough test case that exhausts the iteration
space.

Also simplify some naming and other bits of the code. Nothing substantive
changed here though.

Should be in a good state at this point.

[silvas]

include/llvm/Analysis/CGSCCPassManager.h

528

But all of this should really be spelled out in the prior patch that switches to this API, I'll update that patch with better documentation of this fact.

Please add a test case as well.

[silvas]

include/llvm/Analysis/CGSCCPassManager.h

475

Typo: MaxRepeatitions -> MaxRepetitions

557

Can you just avoid keeping WeakVH's to intrinsics? The langref says you can't take their address. Therefore, a devirtualized call will never be to an intrinsic.
http://llvm.org/docs/LangRef.html#intrinsic-functions

[chandlerc]

include/llvm/Analysis/CGSCCPassManager.h

528

I'm not sure what you mean...

I think there are already test cases for invalidation, but I can double check. But that shouldn't be in *this* patch, that should be in the big CG update patch. I haven't gone back to that one to update it substantially (this iteration utility was requested and I wanted to get it out for review) but I'm happy to add test cases there, or add more testing in subsequent patches for that stack as problems are discovered.

557

It's not that we could create a VH to a call to an instrinsic.

Above, we create a VH to an indirect call. That indirect call gets devirtualized into a direct call. At some point, some pass may turn that direct call into a call to an intrinsic, and if it does that this code needs to defensively prepare for it.

Sadly, there is no pass in tree that does this, but without this logic the pass would crash if anything ever conspired to trigger this.

[silvas]

include/llvm/Analysis/CGSCCPassManager.h

528

I think there are already test cases for invalidation, but I can double check. But that shouldn't be in *this* patch, that should be in the big CG update patch.

Totally agreed. Sorry if it wan't clear from context, but that's what I meant.

I haven't gone back to that one to update it substantially (this iteration utility was requested and I wanted to get it out for review) but I'm happy to add test cases there, or add more testing in subsequent patches for that stack as problems are discovered.

That sounds reasonable. I'll add any comments there once the discussion reopens there.

557

I'm not sure I understand. In the situation you described regarding the intrinsic, we did in fact devirtualize something. So why are we avoiding setting Devirt = true in that case?
Is it because that doesn't actually add extra call edges to the lazy call graph? If that's the case, you need the same treatment for calls to external function declarations and calls to inlineasm too, right?

You may want to add some more comments about this (and maybe a test case (unittest?) if it isn't too verbose).

[chandlerc]

include/llvm/Analysis/CGSCCPassManager.h

557

Sorry, I explained this really poorly.

The idea I had was that if we end up with a call to an intrinsic, there is no longer any call graph or IPO refinement to motivate iteration: scalar passes can already fully reason about the intrinsic.

I've added a bunch of comments and restructured the code to try to make this more clear.

I'm not sure I can craft a test case that does this, but I'll take a stab at it...

[chandlerc]

Rebase and address review comments.

[chandlerc]

include/llvm/Analysis/CGSCCPassManager.h

557

Actually, got a test for this and it convinced me we shouldn't be special casing intrinsics. We can actually do very useful function attrs work here even when "devirtualizing" to an intrinsic that won't be inlined.

Even better, that test pointed out that this was missing a heuristic from the legacy PM's version: we don't track the change of count and have that also trigger iteration.

I've added the heuristic, removed any special casing of intrinsics, and have a nice test case that covers this logic.

[chandlerc]

Add a test case that covers devirtualization that actually turns into an
intrinsic.

Also improved the testing to be more precise and robust.

Added support for iterating even if we turn an indirect call into a direct call
to an intrinsic and added support for the heuristic of changing the call count
that the old PM used.

[chandlerc]

Rebase and ping.

I've also now updated one of the devirtualization test cases for the old
inliner to run with this combined with the inliner in the new pass manager and
it passes now.

There is another devirt test case for the old pass manager that this can't
handle though, and I'm pondering the best way to handle that scenario.

[mehdi_amini]

LGTM.

include/llvm/Analysis/CGSCCPassManager.h

483

Wasn't it a limitation from MSVC2013 that is lifted now that we don't support it anymore?

549

What about something more explicit than i? IterationNum or something like that.
(Two reasons: 1) this is a long loop body and 2) i is used in the body (and also there is another nested loop also using an index i)

579

We could have some debug logging about "Found a devirtualized call from CS->getParent()->getParent()->getName() to F->getName()".

582

On the first iteration, it is weird to check all the value handles to find an impossible devirtualized call, but I don't have a better suggestion to reorganize the code that does not make it equally annoying somewhere else...

test/Other/cgscc-devirt-iteration.ll

36 ↗

(On Diff #82529)

s/repeatitions/repetitions/ (same in many other places in this patch)

43 ↗

(On Diff #82529)

readonce? Do you mean readnone?

60 ↗

(On Diff #82529)

If I follow correctly: this is the devirtualization that will trigger the first restart.

82 ↗

(On Diff #82529)

And this is devirtualized during the second iteration, enabling the third one to deduce the readnone.

100 ↗

(On Diff #82529)

Should you check that BEFORE contains the call to llvm.memcpy and not just devirtualize regularly to memcpy?

test/Other/cgscc-observe-devirt.ll

5

The comment here suggested that this RUN line was useful outside of the repetition?

[chandlerc]

Thanks! Submitting with fixes for essentially all of this. There is one question below that I'm happy to continue discussing and if any of the fixes didn't match what you were expecting just lemme know!

include/llvm/Analysis/CGSCCPassManager.h

582

It doesn't look impossible to me? We have run the pass once at this point, and had handles held across it. Let me know if I'm missing something here though, happy to look for a better way to structure the code.

test/Other/cgscc-devirt-iteration.ll

43 ↗

(On Diff #82529)

Er, readonly. I've added a sentence to clarify. We first deduce readonly, and then readnone.

No idea where 'readonce' came from. Fixed that.

60 ↗

(On Diff #82529)

Yes, and commented.

82 ↗

(On Diff #82529)

Correct, and again, comments added for a future reader.

100 ↗

(On Diff #82529)

Not just before, but all of them. Added.

test/Other/cgscc-observe-devirt.ll

5

Good point. I've restored it.

[mehdi_amini]

include/llvm/Analysis/CGSCCPassManager.h

582

Sorry, I figured after writing this comment but somehow didn't delete it (likely clicked the X instead, which just hide it).

It's all good :)

include/llvm/Analysis/CGSCCPassManager.h

 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_ANALYSIS_CGSCCPASSMANAGER_H
 #define LLVM_ANALYSIS_CGSCCPASSMANAGER_H
 
 #include "llvm/ADT/PriorityWorklist.h"
 #include "llvm/Analysis/LazyCallGraph.h"
+#include "llvm/IR/CallSite.h"
+#include "llvm/IR/InstIterator.h"
 #include "llvm/IR/PassManager.h"
+#include "llvm/IR/ValueHandle.h"
 
 namespace llvm {
 
 struct CGSCCUpdateResult;
 
 extern template class AnalysisManager<LazyCallGraph::SCC, LazyCallGraph &>;
 /// \brief The CGSCC analysis manager.
 ///

 /// \brief A function to deduce a function pass type and wrap it in the
 /// templated adaptor.
 template <typename FunctionPassT>
 CGSCCToFunctionPassAdaptor<FunctionPassT>
 createCGSCCToFunctionPassAdaptor(FunctionPassT Pass, bool DebugLogging = false) {
   return CGSCCToFunctionPassAdaptor<FunctionPassT>(std::move(Pass),
                                                    DebugLogging);
 }
+
+/// A helper that iterates an SCC pass each time an indirect call is refined to
+/// a direct call.
+///
+/// While the CGSCC pass manager works to re-visit SCCs and RefSCCs as they
+/// change shape, we may also want to iterate an SCC pass if it simply refines
+/// an indirect call to a direct call, even if doing so does not alter the
+/// shape of the graph.
+///
+/// This iteration has the potential to be very large however, as each
+/// iteration might refine a single call site. As a consequence, in practice we
+/// use a simple upper bound to limit things.
+template <typename PassT>
+class DevirtIteratingWrapper
+    : public PassInfoMixin<DevirtIteratingWrapper<PassT>> {
+public:
+  explicit DevirtIteratingWrapper(PassT Pass, int MaxIterations,
+                                  bool DebugLogging = false)
+      : Pass(std::move(Pass)), MaxIterations(MaxIterations),
+        DebugLogging(DebugLogging) {}
+  // We have to explicitly define all the special member functions because MSVC
+  // refuses to generate them.
+  DevirtIteratingWrapper(const DevirtIteratingWrapper &Arg)
+      : Pass(Arg.Pass), MaxIterations(Arg.MaxIterations),
+        DebugLogging(Arg.DebugLogging) {}
+  DevirtIteratingWrapper(DevirtIteratingWrapper &&Arg)
+      : Pass(std::move(Arg.Pass)), MaxIterations(Arg.MaxIterations),
+        DebugLogging(Arg.DebugLogging) {}
+  friend void swap(DevirtIteratingWrapper &LHS,
+                   DevirtIteratingWrapper &RHS) {
+    using std::swap;
+    swap(LHS.Pass, RHS.Pass);
+    swap(LHS.MaxIterations, RHS.MaxIterations);
+    swap(LHS.DebugLogging, RHS.DebugLogging);
+  }
+  DevirtIteratingWrapper &operator=(DevirtIteratingWrapper RHS) {
+    swap(*this, RHS);
+    return *this;
+  }
+
+  /// Runs the wrapped pass up to \c MaxIterations on the SCC, iterating
+  /// whenever an indirect call is refined.
+  PreservedAnalyses run(LazyCallGraph::SCC &InitialC, CGSCCAnalysisManager &AM,
+                        LazyCallGraph &CG, CGSCCUpdateResult &UR) {
+    PreservedAnalyses PA = PreservedAnalyses::all();
+
+    // The SCC may be refined while we are running passes over it, so set up
+    // a pointer that we can update.
+    LazyCallGraph::SCC *C = &InitialC;
+
+    // Collect value handles for all of the indirect call sites.
+    SmallVector<WeakVH, 8> CallHandles;
+    SmallPtrSet<Instruction *, 8> CallSet;
+
+    for (int Iterations = 1;; ++Iterations) {
+      // Collect handles for all indirect calls in the SCC.
+      assert(CallHandles.empty() && "Must start with a clear set of handles.");
+      for (LazyCallGraph::Node &N : *C) {
+        CallSet.clear();
+        for (Instruction &I : instructions(N.getFunction()))
+          if (auto CS = CallSite(&I))
+            if (!CS.getCalledFunction())
+              if (CallSet.insert(&I).second)
+                CallHandles.push_back(WeakVH(&I));
+      }
+
+      PreservedAnalyses PassPA = Pass.run(*C, AM, CG, UR);
+
+      // Update the SCC if necessary.
+      C = UR.UpdatedC ? UR.UpdatedC : C;
+
+      // Check that we didn't miss any update scenario.
+      assert(!UR.InvalidatedSCCs.count(C) && "Processing an invalid SCC!");
+      assert(C->begin() != C->end() && "Cannot have an empty SCC!");
+
+      // Update the analysis manager with each iteration and intersect the
+      // total set of preserved analyses.
+      PassPA = AM.invalidate(*C, std::move(PassPA));
+      PA.intersect(std::move(PassPA));
+
+
+      // Check whether any of the handles were devirtualized.
+      bool Devirt = false;
+      for (WeakVH &CallH : CallHandles) {
+        if (!CallH)
+          continue;
+        auto CS = CallSite(CallH);
+        if (!CS)
+          continue;
+
+        // If the call is still indirect, leave it alone.
+        Function *F = CS.getCalledFunction();
+        if (!F)
+          continue;
+
+        // Otherwise, we no longer track it, but if it is
+        // not an intrinsic, we've actually devirtualized
+        // something.
+        if (!F->isIntrinsic()) {
+          Devirt = true;
+          break;
+        }
+      }
+      CallHandles.clear();
+      // If nothing got devirtualized, we're done.
+      if (!Devirt)
+        break;
+      // Otherwise, if we've already hit our max, we're done.
+      if (Iterations >= MaxIterations) {
+        if (DebugLogging)
+          dbgs() << "Found another devirtualization after hitting the max "
+                    "number of iterations ("
+                 << MaxIterations << ") on SCC: " << *C << "\n";
+        break;
+      }
+
+      if (DebugLogging)
+        dbgs() << "Iterating an SCC pass after finding a devirtualization in: "
+               << *C << "\n";
+    }
+
+    return PA;
+  }
+
+private:
+  PassT Pass;
+  int MaxIterations;
+  bool DebugLogging;
+};
+
+/// \brief A function to deduce a function pass type and wrap it in the
+/// templated adaptor.
+template <typename PassT>
+DevirtIteratingWrapper<PassT>
+createDevirtIteratingWrapper(PassT Pass, int MaxIterations,
+                             bool DebugLogging = false) {
+  return DevirtIteratingWrapper<PassT>(std::move(Pass), MaxIterations,
+                                       DebugLogging);
+}
 }
 
 #endif

lib/Passes/PassBuilder.cpp

static Optional<int> parseRepeatPassName(StringRef Name) {
   if (!Name.consume_front("repeat<") || !Name.consume_back(">"))
     return None;
   int Count;
   if (Name.getAsInteger(0, Count) || Count <= 0)
     return None;
   return Count;
 }
 
+static Optional<int> parseDevirtPassName(StringRef Name) {
+  if (!Name.consume_front("devirt<") || !Name.consume_back(">"))
+    return None;
+  int Count;
+  if (Name.getAsInteger(0, Count) || Count <= 0)
+    return None;
+  return Count;
+}
+
 static bool isModulePassName(StringRef Name) {
   // Manually handle aliases for pre-configured pipeline fragments.
   if (Name.startswith("default") || Name.startswith("lto"))
     return DefaultAliasRegex.match(Name);
 
   // Explicitly handle pass manager names.
   if (Name == "module")
     return true;
static bool isCGSCCPassName(StringRef Name) {
   if (Name == "cgscc")
     return true;
   if (Name == "function")
     return true;
 
   // Explicitly handle custom-parsed pass names.
   if (parseRepeatPassName(Name))
     return true;
+  if (parseDevirtPassName(Name))
+    return true;
 
 #define CGSCC_PASS(NAME, CREATE_PASS)                                          \
   if (Name == NAME)                                                            \
     return true;
 #define CGSCC_ANALYSIS(NAME, CREATE_PASS)                                      \
   if (Name == "require<" NAME ">" || Name == "invalidate<" NAME ">")           \
     return true;
 #include "PassRegistry.def"
if (!InnerPipeline.empty()) {
     if (auto Count = parseRepeatPassName(Name)) {
       CGSCCPassManager NestedCGPM(DebugLogging);
       if (!parseCGSCCPassPipeline(NestedCGPM, InnerPipeline, VerifyEachPass,
                                   DebugLogging))
         return false;
       CGPM.addPass(createRepeatingPassWrapper(*Count, std::move(NestedCGPM)));
       return true;
     }
+    if (auto MaxIterations = parseDevirtPassName(Name)) {
+      CGSCCPassManager NestedCGPM(DebugLogging);
+      if (!parseCGSCCPassPipeline(NestedCGPM, InnerPipeline, VerifyEachPass,
+                                  DebugLogging))
+        return false;
+      CGPM.addPass(createDevirtIteratingWrapper(std::move(NestedCGPM),
+                                                *MaxIterations, DebugLogging));
+      return true;
+    }
     // Normal passes can't have pipelines.
     return false;
   }
 
   // Now expand the basic registered passes from the .inc file.
 #define CGSCC_PASS(NAME, CREATE_PASS)                                          \
   if (Name == NAME) {                                                          \
     CGPM.addPass(CREATE_PASS);                                                 \

test/Other/cgscc-observe-devirt.ll

 ; RUN: opt -aa-pipeline=basic-aa -passes='cgscc(function-attrs)' -S < %s | FileCheck %s --check-prefix=BEFORE
-; RUN: opt -aa-pipeline=basic-aa -passes='cgscc(function-attrs,function(gvn))' -S < %s | FileCheck %s --check-prefix=AFTER
-;
-; Also check that adding an extra CGSCC pass after the function update but
-; without requiring the outer manager to iterate doesn't break any invariant.
-; RUN: opt -aa-pipeline=basic-aa -passes='cgscc(function-attrs,function(gvn),function-attrs)' -S < %s | FileCheck %s --check-prefix=AFTER2
+; RUN: opt -aa-pipeline=basic-aa -passes='cgscc(devirt<4>(function-attrs,function(gvn)))' -S < %s | FileCheck %s --check-prefix=AFTER
 
 declare void @readnone() readnone
 declare void @unknown()
 
 ; The @test1_* functions check that when we refine an indirect call to a direct
 ; call, even if it doesn't change the call graph structure, we revisit the SCC
 ; passes to reflect the more precise information.
-; FIXME: Currently, this isn't implemented in the new pass manager and so we
-; only get this with AFTER2, not with AFTER.
 
 ; BEFORE: define void @test1_a() {
 ; AFTER: define void @test1_a() {
-; AFTER2: define void @test1_a() {
 define void @test1_a() {
   %fptr = alloca void()*
   store void()* @unknown, void()** %fptr
   %f = load void()*, void()** %fptr
   call void %f()
   ret void
 }
 
 ; BEFORE: define void @test1_b() {
-; AFTER: define void @test1_b() {
-; AFTER2: define void @test1_b() #0 {
+; AFTER: define void @test1_b() #0 {
 define void @test1_b() {
   %fptr = alloca void()*
   store void()* @readnone, void()** %fptr
   %f = load void()*, void()** %fptr
   call void %f()
   ret void
 }
 
 ; The @test2_* checks that if we refine an indirect call to a direct call and
 ; in the process change the very structure of the call graph we also revisit
 ; that component of the graph and do so in an up-to-date fashion.
 
 ; BEFORE: define void @test2_a1() {
 ; AFTER: define void @test2_a1() {
-; AFTER2: define void @test2_a1() {
 define void @test2_a1() {
   %fptr = alloca void()*
   store void()* @test2_b2, void()** %fptr
   store void()* @test2_b1, void()** %fptr
   %f = load void()*, void()** %fptr
   call void %f()
   ret void
 }
 
 ; BEFORE: define void @test2_b1() {
 ; AFTER: define void @test2_b1() {
-; AFTER2: define void @test2_b1() {
 define void @test2_b1() {
   call void @unknown()
   call void @test2_a1()
   ret void
 }
 
 ; BEFORE: define void @test2_a2() {
 ; AFTER: define void @test2_a2() #0 {
-; AFTER2: define void @test2_a2() #0 {
 define void @test2_a2() {
   %fptr = alloca void()*
   store void()* @test2_b1, void()** %fptr
   store void()* @test2_b2, void()** %fptr
   %f = load void()*, void()** %fptr
   call void %f()
   ret void
 }
 
 ; BEFORE: define void @test2_b2() {
 ; AFTER: define void @test2_b2() #0 {
-; AFTER2: define void @test2_b2() #0 {
 define void @test2_b2() {
   call void @readnone()
   call void @test2_a2()
   ret void
 }
 
 
 ; The @test3_* set of functions exercise a case where running function passes