Address review

Author: nikic

llvm/include/llvm/Analysis/CaptureTracking.h

@@ -14,7 +14,6 @@
 #define LLVM_ANALYSIS_CAPTURETRACKING_H
 
 #include "llvm/ADT/DenseMap.h"
-#include "llvm/Support/ModRef.h"
 
 namespace llvm {
 
@@ -83,6 +82,21 @@ namespace llvm {
   /// addition to the interface here, you'll need to provide your own getters
   /// to see whether anything was captured.
   struct CaptureTracker {
+    /// Action returned from captures().
+    enum Action {
+      /// Stop the traversal.
+      Stop,
+      /// Continue traversal, and also follow the return value of the user if
+      /// it has additional capture components (that is, if it has capture
+      /// components in Ret that are not part of Other).
+      Continue,
+      /// Continue traversal, but do not follow the return value of the user,
+      /// even if it has additional capture components. Should only be used if
+      /// captures() has already taken the potential return captures into
+      /// account.
+      ContinueIgnoringReturn,
+    };
+
     virtual ~CaptureTracker();
 
     /// tooManyUses - The depth of traversal has breached a limit. There may be
@@ -96,38 +110,12 @@ namespace llvm {
     /// U->getUser() is always an Instruction.
     virtual bool shouldExplore(const Use *U);
 
-    /// When returned from captures(), stop the traversal.
-    static std::optional<CaptureComponents> stop() { return std::nullopt; }
-
-    /// When returned from captures(), continue traversal, but do not follow
-    /// the return value of this user, even if it has additional capture
-    /// components. Should only be used if captures() has already taken the
-    /// potential return caputres into account.
-    static std::optional<CaptureComponents> continueIgnoringReturn() {
-      return CaptureComponents::None;
-    }
-
-    /// When returned from captures(), continue traversal, and also follow
-    /// the return value of this user if it has additional capture components
-    /// (that is, capture components in Ret that are not part of Other).
-    static std::optional<CaptureComponents> continueDefault(CaptureInfo CI) {
-      CaptureComponents RetCC = CI.getRetComponents();
-      if (!capturesNothing(RetCC & ~CI.getOtherComponents()))
-        return RetCC;
-      return CaptureComponents::None;
-    }
-
     /// Use U directly captures CI.getOtherComponents() and additionally
     /// CI.getRetComponents() through the return value of the user of U.
     ///
-    /// Return std::nullopt to stop the traversal, or the CaptureComponents to
-    /// follow via the return value, which must be a subset of
-    /// CI.getRetComponents().
-    ///
-    /// For convenience, prefer returning one of stop(), continueDefault(CI) or
-    /// continueIgnoringReturn().
-    virtual std::optional<CaptureComponents> captured(const Use *U,
-                                                      CaptureInfo CI) = 0;
+    /// Return one of Stop, Continue or ContinueIgnoringReturn to control
+    /// further traversal.
+    virtual Action captured(const Use *U, CaptureInfo CI) = 0;
 
     /// isDereferenceableOrNull - Overload to allow clients with additional
     /// knowledge about pointer dereferenceability to provide it and thereby

llvm/lib/Analysis/CaptureTracking.cpp

@@ -81,16 +81,15 @@ struct SimpleCaptureTracker : public CaptureTracker {
     Captured = true;
   }
 
-  std::optional<CaptureComponents> captured(const Use *U,
-                                            CaptureInfo CI) override {
+  Action captured(const Use *U, CaptureInfo CI) override {
     // TODO(captures): Use CaptureInfo.
     if (isa<ReturnInst>(U->getUser()) && !ReturnCaptures)
-      return continueIgnoringReturn();
+      return ContinueIgnoringReturn;
 
     LLVM_DEBUG(dbgs() << "Captured by: " << *U->getUser() << "\n");
 
     Captured = true;
-    return stop();
+    return Stop;
   }
 
   bool ReturnCaptures;
@@ -124,22 +123,21 @@ struct CapturesBefore : public CaptureTracker {
     return !isPotentiallyReachable(I, BeforeHere, nullptr, DT, LI);
   }
 
-  std::optional<CaptureComponents> captured(const Use *U,
-                                            CaptureInfo CI) override {
+  Action captured(const Use *U, CaptureInfo CI) override {
     // TODO(captures): Use CaptureInfo.
     Instruction *I = cast<Instruction>(U->getUser());
     if (isa<ReturnInst>(I) && !ReturnCaptures)
-      return continueIgnoringReturn();
+      return ContinueIgnoringReturn;
 
     // Check isSafeToPrune() here rather than in shouldExplore() to avoid
     // an expensive reachability query for every instruction we look at.
     // Instead we only do one for actual capturing candidates.
     if (isSafeToPrune(I))
       // If the use is not reachable, the instruction result isn't either.
-      return continueIgnoringReturn();
+      return ContinueIgnoringReturn;
 
     Captured = true;
-    return stop();
+    return Stop;
   }
 
   const Instruction *BeforeHere;
@@ -171,12 +169,11 @@ struct EarliestCaptures : public CaptureTracker {
     EarliestCapture = &*F.getEntryBlock().begin();
   }
 
-  std::optional<CaptureComponents> captured(const Use *U,
-                                            CaptureInfo CI) override {
+  Action captured(const Use *U, CaptureInfo CI) override {
     // TODO(captures): Use CaptureInfo.
     Instruction *I = cast<Instruction>(U->getUser());
     if (isa<ReturnInst>(I) && !ReturnCaptures)
-      return continueIgnoringReturn();
+      return ContinueIgnoringReturn;
 
     if (!EarliestCapture)
       EarliestCapture = I;
@@ -187,7 +184,7 @@ struct EarliestCaptures : public CaptureTracker {
     // Continue analysis, as we need to see all potential captures. However,
     // we do not need to follow the instruction result, as this use will
     // dominate any captures made through the instruction result..
-    return continueIgnoringReturn();
+    return ContinueIgnoringReturn;
   }
 
   Instruction *EarliestCapture = nullptr;
@@ -451,17 +448,24 @@ void llvm::PointerMayBeCaptured(const Value *V, CaptureTracker *Tracker,
     CaptureInfo CI = DetermineUseCaptureKind(*U, IsDereferenceableOrNull);
     if (capturesNothing(CI))
       continue;
+    CaptureComponents OtherCC = CI.getOtherComponents();
     CaptureComponents RetCC = CI.getRetComponents();
-    if (!capturesNothing(CI.getOtherComponents())) {
-      std::optional<CaptureComponents> Res = Tracker->captured(U, CI);
-      if (!Res)
+    if (capturesAnything(OtherCC)) {
+      switch (Tracker->captured(U, CI)) {
+      case CaptureTracker::Stop:
         return;
-      assert(capturesNothing(*Res & ~RetCC) &&
-             "captures() result must be subset of getRetComponents()");
-      RetCC = *Res;
+      case CaptureTracker::ContinueIgnoringReturn:
+        continue;
+      case CaptureTracker::Continue:
+        // Fall through to passthrough handling, but only if RetCC contains
+        // additional components that OtherCC does not.
+        if (capturesNothing(RetCC & ~OtherCC))
+          continue;
+        break;
+      }
     }
     // TODO(captures): We could keep track of RetCC for the users.
-    if (!capturesNothing(RetCC) && !AddUses(U->getUser()))
+    if (capturesAnything(RetCC) && !AddUses(U->getUser()))
       return;
   }
 

llvm/lib/Analysis/InstructionSimplify.cpp

@@ -2788,8 +2788,7 @@ static Constant *computePointerICmp(CmpPredicate Pred, Value *LHS, Value *RHS,
       struct CustomCaptureTracker : public CaptureTracker {
         bool Captured = false;
         void tooManyUses() override { Captured = true; }
-        std::optional<CaptureComponents> captured(const Use *U,
-                                                  CaptureInfo CI) override {
+        Action captured(const Use *U, CaptureInfo CI) override {
           // TODO(captures): Use CaptureInfo.
           if (auto *ICmp = dyn_cast<ICmpInst>(U->getUser())) {
             // Comparison against value stored in global variable. Given the
@@ -2798,11 +2797,11 @@ static Constant *computePointerICmp(CmpPredicate Pred, Value *LHS, Value *RHS,
             unsigned OtherIdx = 1 - U->getOperandNo();
             auto *LI = dyn_cast<LoadInst>(ICmp->getOperand(OtherIdx));
             if (LI && isa<GlobalVariable>(LI->getPointerOperand()))
-              return continueDefault(CI);
+              return Continue;
           }
 
           Captured = true;
-          return stop();
+          return Stop;
         }
       };
       CustomCaptureTracker Tracker;

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

@@ -72,8 +72,8 @@ using namespace llvm;
 
 STATISTIC(NumMemoryAttr, "Number of functions with improved memory attribute");
 STATISTIC(NumCapturesNone, "Number of arguments marked captures(none)");
-STATISTIC(NumCapturesOther, "Number of arguments marked with captures "
-                            "attribute other than captures(none)");
+STATISTIC(NumCapturesPartial, "Number of arguments marked with captures "
+                              "attribute other than captures(none)");
 STATISTIC(NumReturned, "Number of arguments marked returned");
 STATISTIC(NumReadNoneArg, "Number of arguments marked readnone");
 STATISTIC(NumReadOnlyArg, "Number of arguments marked readonly");
@@ -114,7 +114,7 @@ static void addCapturesStat(CaptureInfo CI) {
   if (capturesNothing(CI))
     ++NumCapturesNone;
   else
-    ++NumCapturesOther;
+    ++NumCapturesPartial;
 }
 
 namespace {
@@ -549,18 +549,17 @@ struct ArgumentUsesTracker : public CaptureTracker {
 
   void tooManyUses() override { CI = CaptureInfo::all(); }
 
-  std::optional<CaptureComponents> captured(const Use *U,
-                                            CaptureInfo UseCI) override {
+  Action captured(const Use *U, CaptureInfo UseCI) override {
     if (updateCaptureInfo(U, UseCI.getOtherComponents())) {
       // Don't bother continuing if we already capture everything.
       if (capturesAll(CI.getOtherComponents()))
-        return stop();
-      return continueDefault(UseCI);
+        return Stop;
+      return Continue;
     }
 
     // For SCC argument tracking, we're not going to analyze other/ret
     // components separately, so don't follow the return value.
-    return continueIgnoringReturn();
+    return ContinueIgnoringReturn;
   }
 
   bool updateCaptureInfo(const Use *U, CaptureComponents CC) {
@@ -1329,7 +1328,8 @@ static void addArgumentAttrs(const SCCNodeSet &SCCNodes,
         }
 
         // Infer the access attributes given the new captures one
-        DetermineAccessAttrsForSingleton(A);
+        if (DetermineAccessAttrsForSingleton(A))
+          Changed.insert(A->getParent());
       }
       continue;
     }
@@ -1369,7 +1369,7 @@ static void addArgumentAttrs(const SCCNodeSet &SCCNodes,
     }
 
     // TODO(captures): Ignore address-only captures.
-    if (!capturesNothing(CC)) {
+    if (capturesAnything(CC)) {
       // As the pointer may be captured, determine the pointer attributes
       // looking at each argument invidivually.
       for (ArgumentGraphNode *N : ArgumentSCC) {

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

@@ -882,8 +882,7 @@ bool InstCombinerImpl::foldAllocaCmp(AllocaInst *Alloca) {
 
     void tooManyUses() override { Captured = true; }
 
-    std::optional<CaptureComponents> captured(const Use *U,
-                                              CaptureInfo CI) override {
+    Action captured(const Use *U, CaptureInfo CI) override {
       // TODO(captures): Use CaptureInfo.
       auto *ICmp = dyn_cast<ICmpInst>(U->getUser());
       // We need to check that U is based *only* on the alloca, and doesn't
@@ -894,11 +893,11 @@ bool InstCombinerImpl::foldAllocaCmp(AllocaInst *Alloca) {
         // Collect equality icmps of the alloca, and don't treat them as
         // captures.
         ICmps[ICmp] |= 1u << U->getOperandNo();
-        return continueDefault(CI);
+        return Continue;
       }
 
       Captured = true;
-      return stop();
+      return Stop;
     }
   };