Update opt-bisect code to avoid skipping AlwaysInliner

[andrew.w.kaylor]

Summary

While working on an improved test for the opt bisect code I discovered that the AlwaysInliner pass was being skipped because it inherits its runOnSCC implementation from the Inliner base class. This causes failures for AMDGPU targets.

This patch works around that problem by making the skipSCC method virtual and overriding it in AlwaysInliner. There doesn't seem to be a good existing way to determine from the run function whether or not the derived pass can be skipped. Alternatives I considered were either adding a virtual method specific to this purpose (which seemed equivalent to the approach taken in this patch but with extra baggage) and removing the skipSCC check in the base Inliner class but adding case checks for individual function inlining and checking for the "always-inline" attribute at the callsite (which would be useful but leaks a bit of the attribute handling into the base class).

[MatzeB]

I think we can achieve this without a virtual function by refactoring all the code inside Inliner::runOnScc() that comes below the skipSCC() check into an own function. The AlwaysInliner pass can then just override runOnSCC() which calls this common function but contrary to Inliner::runOnSCC() without checking skipSCC().

[andrew.w.kaylor]

In D19640#436283, @MatzeB wrote:

I think we can achieve this without a virtual function by refactoring all the code inside Inliner::runOnScc() that comes below the skipSCC() check into an own function. The AlwaysInliner pass can then just override runOnSCC() which calls this common function but contrary to Inliner::runOnSCC() without checking skipSCC().

So you're thinking something like this?

bool Inliner::runOnSCC(CallGraphSCC &SCC) {
  if (skipSCC(SCC))
    return false;

  return inlineCalls(SCC);
}

bool AlwaysInliner::runOnSCC(CallGraphSCC &SCC) {
  return inlineCalls(SCC);
}

Alternatively, I could just add a "canSkipPass()" virtual method to Inliner, but I suppose the above is consistent with the way that passes are being migrated to the new pass manager.

[andrew.w.kaylor]

Refactored implementation as suggested.

[MatzeB]

LGTM. Nitpick below.

lib/Transforms/IPO/InlineAlways.cpp

48 ↗

(On Diff #58122)

Use three slashes.

llvm/trunk/include/llvm/Transforms/IPO/InlinerPass.h

struct Inliner : public CallGraphSCCPass {
   /// removeDeadFunctions - Remove dead functions.
   ///
   /// This also includes a hack in the form of the 'AlwaysInlineOnly' flag
   /// which restricts it to deleting functions with an 'AlwaysInline'
   /// attribute. This is useful for the InlineAlways pass that only wants to
   /// deal with that subset of the functions.
   bool removeDeadFunctions(CallGraph &CG, bool AlwaysInlineOnly = false);
 
+  /// This function performs the main work of the pass.  The default
+  /// of Inlinter::runOnSCC() calls skipSCC() before calling this method, but
+  /// derived classes which cannot be skipped can override that method and
+  /// call this function unconditionally.
+  bool inlineCalls(CallGraphSCC &SCC);
+
 private:
   // InsertLifetime - Insert @llvm.lifetime intrinsics.
   bool InsertLifetime;
 
   /// shouldInline - Return true if the inliner should attempt to
   /// inline at the given CallSite.
   bool shouldInline(CallSite CS);
   /// Return true if inlining of CS can block the caller from being

llvm/trunk/lib/Transforms/IPO/InlineAlways.cpp

public:
   AlwaysInliner() : Inliner(ID, /*InsertLifetime*/ true) {
     initializeAlwaysInlinerPass(*PassRegistry::getPassRegistry());
   }
 
   AlwaysInliner(bool InsertLifetime) : Inliner(ID, InsertLifetime) {
     initializeAlwaysInlinerPass(*PassRegistry::getPassRegistry());
   }
 
+  /// Main run interface method.  We override here to avoid calling skipSCC().
+  bool runOnSCC(CallGraphSCC &SCC) override { return inlineCalls(SCC); }
+
   static char ID; // Pass identification, replacement for typeid
 
   InlineCost getInlineCost(CallSite CS) override;
 
   using llvm::Pass::doFinalization;
   bool doFinalization(CallGraph &CG) override {
     return removeDeadFunctions(CG, /*AlwaysInlineOnly=*/ true);
   }

llvm/trunk/lib/Transforms/IPO/Inliner.cpp

while (InlineHistoryID != -1) {
     InlineHistoryID = InlineHistory[InlineHistoryID].second;
   }
   return false;
 }
 
 bool Inliner::runOnSCC(CallGraphSCC &SCC) {
   if (skipSCC(SCC))
     return false;
+  return inlineCalls(SCC);
+}
 
+bool Inliner::inlineCalls(CallGraphSCC &SCC) {
   CallGraph &CG = getAnalysis<CallGraphWrapperPass>().getCallGraph();
   ACT = &getAnalysis<AssumptionCacheTracker>();
   auto &TLI = getAnalysis<TargetLibraryInfoWrapperPass>().getTLI();
 
   SmallPtrSet<Function*, 8> SCCFunctions;
   DEBUG(dbgs() << "Inliner visiting SCC:");
   for (CallGraphNode *Node : SCC) {
     Function *F = Node->getFunction();