Remove macros and visitors from AccessedStorage.

Cleaning up in preparation for making changes that improve
compile-time issues in AccessEnforcementOpts.

This is a simple but important enum with a handful of cases. The cases
need to be easily referenced from the header. Don't define them in a
separate .def. Remove the visitor biolerplate because it doesn't serve
any purpose.

This enum is meant to be used with covered switches. The enum cases do
not have their own types, so there's no performance reason to use a
Visitor pattern.

It should not be possible to add a case to this enum without carefully
considering the impact on the encoding of this class and the impact on
each and every one of the uses. We always want covered switches at the
use sites.

This is a major improvement in readability and usability both in the
definition of the class and in the one place where a visitor was used.

Author: atrick

include/swift/SIL/AccessedStorage.def

@@ -106,11 +106,15 @@ class AccessedStorage {
   /// Enumerate over all valid begin_access bases. Clients can use a covered
   /// switch to warn if findAccessedAddressBase ever adds a case.
   enum Kind : uint8_t {
-#define ACCESSED_STORAGE(Name) Name,
-#define ACCESSED_STORAGE_RANGE(Name, Start, End)                               \
-  First_##Name = Start, Last_##Name = End,
-#include "swift/SIL/AccessedStorage.def"
-    NumKindBits = countBitsUsed(unsigned(Last_AccessedStorageKind))
+    Box,
+    Stack,
+    Global,
+    Class,
+    Argument,
+    Yield,
+    Nested,
+    Unidentified,
+    NumKindBits = countBitsUsed(static_cast<unsigned>(Unidentified))
   };
 
   static const char *getKindName(Kind k);
@@ -322,26 +326,6 @@ class AccessedStorage {
   bool operator==(const AccessedStorage &) const = delete;
   bool operator!=(const AccessedStorage &) const = delete;
 };
-
-template <class ImplTy, class ResultTy = void, typename... ArgTys>
-class AccessedStorageVisitor {
-  ImplTy &asImpl() { return static_cast<ImplTy &>(*this); }
-
-public:
-#define ACCESSED_STORAGE(Name)                                                 \
-  ResultTy visit##Name(const AccessedStorage &storage, ArgTys &&... args);
-#include "swift/SIL/AccessedStorage.def"
-
-  ResultTy visit(const AccessedStorage &storage, ArgTys &&... args) {
-    switch (storage.getKind()) {
-#define ACCESSED_STORAGE(Name)                                                 \
-  case AccessedStorage::Name:                                                  \
-    return asImpl().visit##Name(storage, std::forward<ArgTys>(args)...);
-#include "swift/SIL/AccessedStorage.def"
-    }
-  }
-};
-
 } // end namespace swift
 
 namespace llvm {

include/swift/SIL/MemAccessUtils.h

@@ -140,10 +140,22 @@ const ValueDecl *AccessedStorage::getDecl(SILFunction *F) const {
 
 const char *AccessedStorage::getKindName(AccessedStorage::Kind k) {
   switch (k) {
-#define ACCESSED_STORAGE(NAME)                                                 \
-  case AccessedStorage::NAME:                                                  \
-    return #NAME;
-#include "swift/SIL/AccessedStorage.def"
+  case Box:
+    return "Box";
+  case Stack:
+    return "Stack";
+  case Nested:
+    return "Nested";
+  case Unidentified:
+    return "Unidentified";
+  case Argument:
+    return "Argument";
+  case Yield:
+    return "Yield";
+  case Global:
+    return "Global";
+  case Class:
+    return "Class";
   }
   llvm_unreachable("unhandled kind");
 }

lib/SIL/MemAccessUtils.cpp

@@ -339,56 +339,29 @@ bool SemanticARCOptVisitor::visitCopyValueInst(CopyValueInst *cvi) {
 //                         load [copy] Optimizations
 //===----------------------------------------------------------------------===//
 
-/// A flow insensitive analysis that tells the load [copy] analysis if the
-/// storage has 0, 1, >1 writes to it.
-///
-/// In the case of 0 writes, we return CanOptimizeLoadCopyResult::Always.
-///
-/// In the case of 1 write, we return OnlyIfStorageIsLocal. We are taking
-/// advantage of definite initialization implying that an alloc_stack must be
-/// written to once before any loads from the memory location. Thus if we are
-/// local and see 1 write, we can still change to load_borrow if all other uses
-/// check out.
-///
-/// If there is 2+ writes, we can not optimize = (.
-namespace {
-
-struct CanOptimizeLoadCopyFromAccessVisitor
-    : AccessedStorageVisitor<CanOptimizeLoadCopyFromAccessVisitor, bool> {
-  SILFunction &f;
-
-  CanOptimizeLoadCopyFromAccessVisitor(SILFunction &f) : f(f) {}
-
-  // Stubs
-  bool visitBox(const AccessedStorage &boxStorage) { return false; }
-  bool visitStack(const AccessedStorage &stackStorage) { return false; }
-  bool visitGlobal(const AccessedStorage &globalStorage) { return false; }
-  bool visitClass(const AccessedStorage &classStorage) { return false; }
-  bool visitYield(const AccessedStorage &yieldStorage) { return false; }
-  bool visitUnidentified(const AccessedStorage &unidentifiedStorage) {
-    return false;
-  }
-  bool visitNested(const AccessedStorage &nested) {
-    llvm_unreachable("Visitor should never see nested since we lookup our "
-                     "address storage using lookup non nested");
-  }
-
-  bool visitArgument(const AccessedStorage &argumentStorage);
-};
-
-} // namespace
+// A flow insensitive analysis that tells the load [copy] analysis if the
+// storage has 0, 1, >1 writes to it.
+//
+// In the case of 0 writes, we return CanOptimizeLoadCopyResult::Always.
+//
+// In the case of 1 write, we return OnlyIfStorageIsLocal. We are taking
+// advantage of definite initialization implying that an alloc_stack must be
+// written to once before any loads from the memory location. Thus if we are
+// local and see 1 write, we can still change to load_borrow if all other uses
+// check out.
+//
+// If there is 2+ writes, we can not optimize = (.
 
-bool CanOptimizeLoadCopyFromAccessVisitor::visitArgument(
-    const AccessedStorage &storage) {
+bool mayFunctionMutateArgument(const AccessedStorage &storage, SILFunction &f) {
   auto *arg = cast<SILFunctionArgument>(storage.getArgument(&f));
 
   // Then check if we have an in_guaranteed argument. In this case, we can
   // always optimize load [copy] from this.
   if (arg->hasConvention(SILArgumentConvention::Indirect_In_Guaranteed))
-    return true;
+    return false;
 
   // For now just return false.
-  return false;
+  return true;
 }
 
 static bool isWrittenTo(SILFunction &f, SILValue value) {
@@ -402,7 +375,19 @@ static bool isWrittenTo(SILFunction &f, SILValue value) {
   // way (ignoring stores that are obviously the only initializer to
   // memory). We have to do this since load_borrow assumes that the
   // underlying memory is never written to.
-  return !CanOptimizeLoadCopyFromAccessVisitor(f).visit(storage);
+  switch (storage.getKind()) {
+  case AccessedStorage::Box:
+  case AccessedStorage::Stack:
+  case AccessedStorage::Global:
+  case AccessedStorage::Class:
+  case AccessedStorage::Yield:
+  case AccessedStorage::Nested:
+  case AccessedStorage::Unidentified:
+    return true;
+
+  case AccessedStorage::Argument:
+    return mayFunctionMutateArgument(storage, f);
+  }
 }
 
 // Convert a load [copy] from unique storage [read] that has all uses that can