Merge pull request #30741 from gottesmm/pr-961dc8ea62390b2c78cdfafd88f3d04ebfcb7ade

[arc-opts] Teach IsAddressWrittenToDefUseAnalysis how to track "well behaved" writes but don't use it.

Author: gottesmm

include/swift/Basic/MultiMapCache.h

@@ -18,40 +18,35 @@
 
 namespace swift {
 
-/// A CRTP write-once multi-map cache that can be small. It uses a DenseMap
+/// A write-once multi-map cache that can be small. It uses a DenseMap
 /// internally, so it can be used as a cache without needing to be frozen like
-/// FrozenMultiMap (which uses only a vector internally). The Impl class
-/// implements the method constructValuesForKey that is used to compute the
-/// actual cache value.
-///
-/// NOTE: constructValuesForKeys is assumed to take a KeyTy and a
-/// SmallVectorImpl<ValueTy>. It must append all results to that accumulator and
-/// not read any contents of the accumulator.
+/// FrozenMultiMap (which uses only a vector internally). The cached value is
+/// computed by a passed in std::function. The std::function is able to map
+/// multiple values to a specific key via the out array.
 ///
 /// NOTE: We store the (size, length) of each ArrayRef<ValueTy> instead of
 /// storing the ArrayRef to avoid data invalidation issues caused by SmallVector
 /// switching from small to large representations.
 ///
 /// For an example of a subclass implementation see:
 /// unittests/Basic/MultiMapCacheTest.cpp.
-template <typename ImplType, typename KeyTy, typename ValueTy,
+template <typename KeyTy, typename ValueTy,
           typename MapTy =
               llvm::DenseMap<KeyTy, Optional<std::tuple<unsigned, unsigned>>>,
           typename VectorTy = std::vector<ValueTy>,
           typename VectorTyImpl = VectorTy>
 class MultiMapCache {
+  std::function<bool(const KeyTy &, VectorTyImpl &)> function;
   MapTy valueToDataOffsetIndexMap;
   VectorTy data;
 
   constexpr static unsigned ArrayStartOffset = 0;
   constexpr static unsigned ArrayLengthOffset = 1;
 
-  constexpr ImplType &asImpl() const {
-    auto *self = const_cast<MultiMapCache *>(this);
-    return reinterpret_cast<ImplType &>(*self);
-  }
-
 public:
+  MultiMapCache(std::function<bool(const KeyTy &, VectorTyImpl &)> function)
+      : function(function) {}
+
   void clear() {
     valueToDataOffsetIndexMap.clear();
     data.clear();
@@ -81,7 +76,7 @@ class MultiMapCache {
 
     // We assume that constructValuesForKey /only/ inserts to the end of data
     // and does not inspect any other values in the data array.
-    if (!asImpl().constructValuesForKey(key, data)) {
+    if (!function(key, data)) {
       return None;
     }
 
@@ -94,9 +89,9 @@ class MultiMapCache {
   }
 };
 
-template <typename ImplType, typename KeyTy, typename ValueTy>
+template <typename KeyTy, typename ValueTy>
 using SmallMultiMapCache = MultiMapCache<
-    ImplType, KeyTy, ValueTy,
+    KeyTy, ValueTy,
     llvm::SmallDenseMap<KeyTy, Optional<std::tuple<unsigned, unsigned>>, 8>,
     SmallVector<ValueTy, 32>, SmallVectorImpl<ValueTy>>;
 

lib/SILOptimizer/Transforms/SemanticARCOpts.cpp

@@ -13,6 +13,7 @@
 #define DEBUG_TYPE "sil-semantic-arc-opts"
 #include "swift/Basic/BlotSetVector.h"
 #include "swift/Basic/FrozenMultiMap.h"
+#include "swift/Basic/MultiMapCache.h"
 #include "swift/Basic/STLExtras.h"
 #include "swift/SIL/BasicBlockUtils.h"
 #include "swift/SIL/DebugUtils.h"
@@ -477,35 +478,13 @@ LiveRange::hasUnknownConsumingUse(bool assumingAtFixPoint) const {
 //                        Address Written To Analysis
 //===----------------------------------------------------------------------===//
 
-namespace {
-
-/// A simple analysis that checks if a specific def (in our case an inout
-/// argument) is ever written to. This is conservative, local and processes
-/// recursively downwards from def->use.
-struct IsAddressWrittenToDefUseAnalysis {
-  llvm::SmallDenseMap<SILValue, bool, 8> isWrittenToCache;
-
-  bool operator()(SILValue value) {
-    auto iter = isWrittenToCache.try_emplace(value, true);
-
-    // If we are already in the map, just return that.
-    if (!iter.second)
-      return iter.first->second;
-
-    // Otherwise, compute our value, cache it and return.
-    bool result = isWrittenToHelper(value);
-    iter.first->second = result;
-    return result;
-  }
-
-private:
-  bool isWrittenToHelper(SILValue value);
-};
-
-} // end anonymous namespace
-
-bool IsAddressWrittenToDefUseAnalysis::isWrittenToHelper(SILValue initialValue) {
+/// Returns true if we were able to ascertain that either the initialValue has
+/// no write uses or all of the write uses were writes that we could understand.
+static bool
+constructCacheValue(SILValue initialValue,
+                    SmallVectorImpl<Operand *> &wellBehavedWriteAccumulator) {
   SmallVector<Operand *, 8> worklist(initialValue->getUses());
+
   while (!worklist.empty()) {
     auto *op = worklist.pop_back_val();
     SILInstruction *user = op->getUser();
@@ -521,35 +500,41 @@ bool IsAddressWrittenToDefUseAnalysis::isWrittenToHelper(SILValue initialValue)
     if (auto *oeai = dyn_cast<OpenExistentialAddrInst>(user)) {
       // Mutable access!
       if (oeai->getAccessKind() != OpenedExistentialAccess::Immutable) {
-        return true;
+        wellBehavedWriteAccumulator.push_back(op);
       }
 
       //  Otherwise, look through it and continue.
       llvm::copy(oeai->getUses(), std::back_inserter(worklist));
       continue;
     }
 
+    // Add any destroy_addrs to the resultAccumulator.
+    if (isa<DestroyAddrInst>(user)) {
+      wellBehavedWriteAccumulator.push_back(op);
+      continue;
+    }
+
     // load_borrow and incidental uses are fine as well.
     if (isa<LoadBorrowInst>(user) || isIncidentalUse(user)) {
       continue;
     }
 
     // Look through immutable begin_access.
     if (auto *bai = dyn_cast<BeginAccessInst>(user)) {
-      // If we do not have a read, return true.
+      // If we do not have a read, mark this as a write.
       if (bai->getAccessKind() != SILAccessKind::Read) {
-        return true;
+        wellBehavedWriteAccumulator.push_back(op);
       }
 
       // Otherwise, add the users to the worklist and continue.
       llvm::copy(bai->getUses(), std::back_inserter(worklist));
       continue;
     }
 
-    // As long as we do not have a load [take], we are fine.
+    // If we have a load, we just need to mark the load [take] as a write.
     if (auto *li = dyn_cast<LoadInst>(user)) {
       if (li->getOwnershipQualifier() == LoadOwnershipQualifier::Take) {
-        return true;
+        wellBehavedWriteAccumulator.push_back(op);
       }
       continue;
     }
@@ -559,41 +544,45 @@ bool IsAddressWrittenToDefUseAnalysis::isWrittenToHelper(SILValue initialValue)
     // interprocedural analysis that we do not perform here.
     if (auto fas = FullApplySite::isa(user)) {
       if (fas.getArgumentConvention(*op) ==
-          SILArgumentConvention::Indirect_In_Guaranteed)
+          SILArgumentConvention::Indirect_In_Guaranteed) {
         continue;
+      }
 
-      // Otherwise, be conservative and return true.
-      return true;
+      // Otherwise, be conservative and return that we had a write that we did
+      // not understand.
+      return false;
     }
 
     // Copy addr that read are just loads.
     if (auto *cai = dyn_cast<CopyAddrInst>(user)) {
       // If our value is the destination, this is a write.
       if (cai->getDest() == op->get()) {
-        return true;
+        wellBehavedWriteAccumulator.push_back(op);
+        continue;
       }
 
       // Ok, so we are Src by process of elimination. Make sure we are not being
       // taken.
       if (cai->isTakeOfSrc()) {
-        return true;
+        wellBehavedWriteAccumulator.push_back(op);
+        continue;
       }
 
       // Otherwise, we are safe and can continue.
       continue;
     }
 
     // If we did not recognize the user, just return conservatively that it was
-    // written to.
+    // written to in a way we did not understand.
     LLVM_DEBUG(llvm::dbgs()
                << "Function: " << user->getFunction()->getName() << "\n");
     LLVM_DEBUG(llvm::dbgs() << "Value: " << op->get());
     LLVM_DEBUG(llvm::dbgs() << "Unknown instruction!: " << *user);
-    return true;
+    return false;
   }
 
   // Ok, we finished our worklist and this address is not being written to.
-  return false;
+  return true;
 }
 
 //===----------------------------------------------------------------------===//
@@ -623,7 +612,7 @@ struct SemanticARCOptVisitor
   SILFunction &F;
   Optional<DeadEndBlocks> TheDeadEndBlocks;
   ValueLifetimeAnalysis::Frontier lifetimeFrontier;
-  IsAddressWrittenToDefUseAnalysis isAddressWrittenToDefUseAnalysis;
+  SmallMultiMapCache<SILValue, Operand *> addressToExhaustiveWriteListCache;
 
   /// Are we assuming that we reached a fix point and are re-processing to
   /// prepare to use the phiToIncomingValueMultiMap.
@@ -658,7 +647,8 @@ struct SemanticARCOptVisitor
   using FrozenMultiMapRange =
       decltype(joinedOwnedIntroducerToConsumedOperands)::PairToSecondEltRange;
 
-  explicit SemanticARCOptVisitor(SILFunction &F) : F(F) {}
+  explicit SemanticARCOptVisitor(SILFunction &F)
+      : F(F), addressToExhaustiveWriteListCache(constructCacheValue) {}
 
   DeadEndBlocks &getDeadEndBlocks() {
     if (!TheDeadEndBlocks)
@@ -1663,7 +1653,8 @@ class StorageGuaranteesLoadVisitor
 
     // If we have a modify, check if our value is /ever/ written to. If it is
     // never actually written to, then we convert to a load_borrow.
-    return answer(ARCOpt.isAddressWrittenToDefUseAnalysis(access));
+    auto result = ARCOpt.addressToExhaustiveWriteListCache.get(access);
+    return answer(!result.hasValue() || result.getValue().size());
   }
 
   void visitArgumentAccess(SILFunctionArgument *arg) {
@@ -1676,7 +1667,8 @@ class StorageGuaranteesLoadVisitor
     // If we have an inout parameter that isn't ever actually written to, return
     // false.
     if (arg->getKnownParameterInfo().isIndirectMutating()) {
-      return answer(ARCOpt.isAddressWrittenToDefUseAnalysis(arg));
+      auto result = ARCOpt.addressToExhaustiveWriteListCache.get(arg);
+      return answer(!result.hasValue() || result.getValue().size());
     }
 
     // TODO: This should be extended:

unittests/Basic/MultiMapCacheTest.cpp

@@ -17,24 +17,49 @@
 
 using namespace swift;
 
-namespace {
-
-/// A multimap cache that caches the initial 4 powers of each key.
-struct PowerMultiMapCache
-    : public MultiMapCache<PowerMultiMapCache, unsigned, unsigned> {
-  bool constructValuesForKey(unsigned key, std::vector<unsigned> &data) {
-    // Construct the first 3 powers of key.
-    data.push_back(key);
-    data.push_back(key * key);
-    data.push_back(key * key * key);
-    return true;
+TEST(MultiMapCache, powersTest) {
+  std::function<bool(unsigned, std::vector<unsigned> &)> cacheCompute =
+      [&](unsigned key, std::vector<unsigned> &outArray) {
+        outArray.push_back(key);
+        outArray.push_back(key * key);
+        outArray.push_back(key * key * key);
+        return true;
+      };
+  MultiMapCache<unsigned, unsigned> cache(cacheCompute);
+
+  EXPECT_TRUE(cache.empty());
+  EXPECT_EQ(cache.size(), 0u);
+  for (unsigned index : range(1, 256)) {
+    auto array = *cache.get(index);
+    for (unsigned power : array) {
+      EXPECT_EQ(power % index, 0);
+    }
+  }
+  EXPECT_FALSE(cache.empty());
+  EXPECT_EQ(cache.size(), 255);
+  for (unsigned index : range(1, 256)) {
+    auto array = *cache.get(index);
+    for (unsigned power : array) {
+      EXPECT_EQ(power % index, 0);
+    }
   }
-};
+  EXPECT_FALSE(cache.empty());
+  EXPECT_EQ(cache.size(), 255);
 
-} // end anonymous namespace
+  cache.clear();
+  EXPECT_TRUE(cache.empty());
+  EXPECT_EQ(cache.size(), 0);
+}
 
-TEST(MultiMapCache, powersTest) {
-  PowerMultiMapCache cache;
+TEST(MultiMapCache, smallTest) {
+  std::function<bool(unsigned, SmallVectorImpl<unsigned> &)> cacheCompute =
+      [&](unsigned key, SmallVectorImpl<unsigned> &outArray) {
+        outArray.push_back(key);
+        outArray.push_back(key * key);
+        outArray.push_back(key * key * key);
+        return true;
+      };
+  SmallMultiMapCache<unsigned, unsigned> cache(cacheCompute);
 
   EXPECT_TRUE(cache.empty());
   EXPECT_EQ(cache.size(), 0u);