[Attributor] Split the Attributor::run() into multiple functions.

[kuter]

Summary

This patch splits the Attributor::run() function into multiple functions.

Simple Logic changes to make this possible:

1. Moved iteration count verification earlier.
2. NumFinalAAs get set a little bit later.

[jdoerfert]

Nice cleanup. I basically have only comments on the documentation.

llvm/include/llvm/Transforms/IPO/Attributor.h

1220

I find the name of this not convincing. Maybe, runTillFixpoint or something along those lines?
The documentation should also be a bit more descriptive, e.g., in case of a time out of what? At the same time we don't need to talk about the dep graph but could just say, "on attributes that haven't reached a known valid fixpoint yet".

[kuter]

1. rename ::runScheduler to ::runTillFixpoint
2. improve comments for ::runTillFixpoint

[jdoerfert]

LGTM, one nit.

llvm/include/llvm/Transforms/IPO/Attributor.h

1230

Nit: Newline.

[kuter]

add newline after function declaration

[jdoerfert]

@sstefan1 I saw you committed and reverted this. What happened?

[sstefan1]

In D81022#2086242, @jdoerfert wrote:

@sstefan1 I saw you committed and reverted this. What happened?

Forgot to change the author...sorry for the mess.

[jdoerfert]

In D81022#2086857, @sstefan1 wrote:

In D81022#2086242, @jdoerfert wrote:

@sstefan1 I saw you committed and reverted this. What happened?

Forgot to change the author...sorry for the mess.

Happens. No worries and thanks for helping out!

llvm/include/llvm/Transforms/IPO/Attributor.h

struct Attributor {
 
   /// Return the data layout associated with the anchor scope.
   const DataLayout &getDataLayout() const { return InfoCache.DL; }
 
   /// The allocator used to allocate memory, e.g. for `AbstractAttribute`s.
   BumpPtrAllocator &Allocator;
 
 private:
+  /// This method will do fixpoint iteration until fixpoint or the
+  /// maximum iteration count is reached.
+  ///
+  /// If the maximum iteration count is reached, This method will
+  /// indicate pessimistic fixpoint on attributes that transitively depend
+  /// on attributes that were scheduled for an update.
+  void runTillFixpoint();
+
+  /// Gets called after scheduling, manifests attributes to the LLVM IR.
+  ChangeStatus manifestAttributes();
+
+  /// Gets called after attributes have been manifested, cleans up the IR.
+  /// Deletes dead functions, blocks and instructions.
+  /// Rewrites function signitures and updates the call graph.
+  ChangeStatus cleanupIR();
+
   /// Run `::update` on \p AA and track the dependences queried while doing so.
   /// Also adjust the state if we know further updates are not necessary.
   ChangeStatus updateAA(AbstractAttribute &AA);
 
   /// Remember the dependences on the top of the dependence stack such that they
   /// may trigger further updates. (\see DependenceStack)
   void rememberDependences();
 

llvm/lib/Transforms/IPO/Attributor.cpp

for (Instruction *I :
 
     if (!Pred(*I))
       return false;
   }
 
   return true;
 }
 
-ChangeStatus Attributor::run() {
+void Attributor::runTillFixpoint() {
   LLVM_DEBUG(dbgs() << "[Attributor] Identified and initialized "
                     << AllAbstractAttributes.size()
                     << " abstract attributes.\n");
 
   // Now that all abstract attributes are collected and initialized we start
   // the abstract analysis.
 
   unsigned IterationCounter = 1;
void Attributor::runTillFixpoint() {
 
   } while (!Worklist.empty() && (IterationCounter++ < MaxFixpointIterations ||
                                  VerifyMaxFixpointIterations));
 
   LLVM_DEBUG(dbgs() << "\n[Attributor] Fixpoint iteration done after: "
                     << IterationCounter << "/" << MaxFixpointIterations
                     << " iterations\n");
 
-  size_t NumFinalAAs = AllAbstractAttributes.size();
-
   // Reset abstract arguments not settled in a sound fixpoint by now. This
   // happens when we stopped the fixpoint iteration early. Note that only the
   // ones marked as "changed" *and* the ones transitively depending on them
   // need to be reverted to a pessimistic state. Others might not be in a
   // fixpoint state but we can use the optimistic results for them anyway.
   SmallPtrSet<AbstractAttribute *, 32> Visited;
   for (unsigned u = 0; u < ChangedAAs.size(); u++) {
     AbstractAttribute *ChangedAA = ChangedAAs[u];
void Attributor::runTillFixpoint() {
   }
 
   LLVM_DEBUG({
     if (!Visited.empty())
       dbgs() << "\n[Attributor] Finalized " << Visited.size()
              << " abstract attributes.\n";
   });
 
+  if (VerifyMaxFixpointIterations &&
+      IterationCounter != MaxFixpointIterations) {
+    errs() << "\n[Attributor] Fixpoint iteration done after: "
+           << IterationCounter << "/" << MaxFixpointIterations
+           << " iterations\n";
+    llvm_unreachable("The fixpoint was not reached with exactly the number of "
+                     "specified iterations!");
+  }
+}
+
+ChangeStatus Attributor::manifestAttributes() {
+  size_t NumFinalAAs = AllAbstractAttributes.size();
+
   unsigned NumManifested = 0;
   unsigned NumAtFixpoint = 0;
   ChangeStatus ManifestChange = ChangeStatus::UNCHANGED;
   for (AbstractAttribute *AA : AllAbstractAttributes) {
     AbstractState &State = AA->getState();
 
     // If there is not already a fixpoint reached, we can now take the
     // optimistic state. This is correct because we enforced a pessimistic one
if (NumFinalAAs != AllAbstractAttributes.size()) {
     for (unsigned u = NumFinalAAs; u < AllAbstractAttributes.size(); ++u)
       errs() << "Unexpected abstract attribute: " << *AllAbstractAttributes[u]
              << " :: "
              << AllAbstractAttributes[u]->getIRPosition().getAssociatedValue()
              << "\n";
     llvm_unreachable("Expected the final number of abstract attributes to "
                      "remain unchanged!");
   }
+  return ManifestChange;
+}
 
+ChangeStatus Attributor::cleanupIR() {
   // Delete stuff at the end to avoid invalid references and a nice order.
-  {
     LLVM_DEBUG(dbgs() << "\n[Attributor] Delete at least "
                       << ToBeDeletedFunctions.size() << " functions and "
                       << ToBeDeletedBlocks.size() << " blocks and "
                       << ToBeDeletedInsts.size() << " instructions and "
                       << ToBeChangedUses.size() << " uses\n");
 
     SmallVector<WeakTrackingVH, 32> DeadInsts;
     SmallVector<Instruction *, 32> TerminatorsToFold;
while (FoundDeadFn) {
                 *F, true, nullptr, AllCallSitesKnown))
           continue;
 
         ToBeDeletedFunctions.insert(F);
         InternalFns[u] = nullptr;
         FoundDeadFn = true;
       }
     }
-  }
 
   // Rewrite the functions as requested during manifest.
-  ManifestChange =
-      ManifestChange | rewriteFunctionSignatures(CGModifiedFunctions);
+    ChangeStatus ManifestChange =
+        rewriteFunctionSignatures(CGModifiedFunctions);
 
     for (Function *Fn : CGModifiedFunctions)
       CGUpdater.reanalyzeFunction(*Fn);
 
     for (Function *Fn : ToBeDeletedFunctions)
       CGUpdater.removeFunction(*Fn);
 
     NumFnDeleted += ToBeDeletedFunctions.size();
 
-  if (VerifyMaxFixpointIterations &&
-      IterationCounter != MaxFixpointIterations) {
-    errs() << "\n[Attributor] Fixpoint iteration done after: "
-           << IterationCounter << "/" << MaxFixpointIterations
-           << " iterations\n";
-    llvm_unreachable("The fixpoint was not reached with exactly the number of "
-                     "specified iterations!");
-  }
-
 #ifdef EXPENSIVE_CHECKS
   for (Function *F : Functions) {
     if (ToBeDeletedFunctions.count(F))
       continue;
     assert(!verifyFunction(*F, &errs()) && "Module verification failed!");
   }
 #endif
 
   return ManifestChange;
 }
 
+ChangeStatus Attributor::run() {
+  runTillFixpoint();
+  ChangeStatus ManifestChange = manifestAttributes();
+  ChangeStatus CleanupChange = cleanupIR();
+  return ManifestChange | CleanupChange;
+}
+
 ChangeStatus Attributor::updateAA(AbstractAttribute &AA) {
   // Use a new dependence vector for this update.
   DependenceVector DV;
   DependenceStack.push_back(&DV);
 
   auto &AAState = AA.getState();
   ChangeStatus CS = ChangeStatus::UNCHANGED;
   if (!isAssumedDead(AA, nullptr, /* CheckBBLivenessOnly */ true))