Fix an AccessedStorage assert for SIL global variables.

Allow round-tripping access to global variables. Previously,
AccessedStorage asserted that global variables were always associated
with a VarDecl. This was to ensure that AccessEnforcmentWMO always
recognized the global. Failing to recognize access to a global will
cause a miscompile.

SILGlobalVariable now has all the information needed by
SIL. Particularly, the 'isLet' flag. Simply replace VarDecl with
SILGlobalVariable in AccessEnforcmentWMO to eliminate the need for the
assert.

Author: atrick

include/swift/SIL/MemAccessUtils.h

@@ -627,7 +627,7 @@ class AccessBase : public AccessRepresentation {
     return findReferenceRoot(getReference());
   }
 
-  /// Return the global variable being accessed.
+  /// Return the global variable being accessed. Always valid.
   ///
   /// Precondition: getKind() == Global
   SILGlobalVariable *getGlobal() const;

lib/SIL/Utils/MemAccessUtils.cpp

@@ -955,17 +955,10 @@ AccessStorage::AccessStorage(SILValue base, Kind kind)
   }
   if (getKind() == AccessBase::Global) {
     global = getReferencedGlobal(cast<SingleValueInstruction>(base));
-    // Require a decl for all formally accessed globals defined in this
-    // module. AccessEnforcementWMO requires this. Swift globals defined in
-    // another module either use an addressor, which has Unidentified
-    // storage. Imported non-Swift globals are accessed via global_addr but have
-    // no declaration.
-    assert(global->getDecl() || isa<GlobalAddrInst>(base));
-
     // It's unclear whether a global will ever be missing it's varDecl, but
     // technically we only preserve it for debug info. So if we don't have a
-    // decl, check the flag on SILGlobalVariable, which is guaranteed valid,
-    setLetAccess(getGlobal()->isLet());
+    // decl, check the flag on SILGlobalVariable, which is guaranteed valid.
+    setLetAccess(global->isLet());
     return;
   }
   value = base;
@@ -1001,7 +994,7 @@ const ValueDecl *AccessStorage::getDecl() const {
     return nullptr;
   case Class: {
     // The property index is relative to the VarDecl in ref_element_addr, and
-    // can only be reliably determined when the base is avaiable. Without the
+    // can only be reliably determined when the base is available. Without the
     // base, we can only make a best effort to extract it from the object type,
     // which might not even be a class in the case of bridge objects.
     if (ClassDecl *classDecl =

lib/SILOptimizer/Transforms/AccessEnforcementWMO.cpp

@@ -66,22 +66,28 @@ using namespace swift;
 using llvm::DenseMap;
 using llvm::SmallDenseSet;
 
+using DisjointAccessLocationKey =
+    llvm::PointerUnion<const VarDecl *, const SILGlobalVariable *>;
+
 // Get the VarDecl that represents the DisjointAccessLocation for the given
 // storage and access base. Returns nullptr for any storage that can't be
 // partitioned into a disjoint location.
 //
 // Global storage is expected to be disjoint because identifyFormalAccess may
 // only return Unidentified storage for a global variable access if the global
 // is defined in a different module.
-const VarDecl *
+static DisjointAccessLocationKey
 getDisjointAccessLocation(AccessStorageWithBase storageAndBase) {
   auto storage = storageAndBase.storage;
   switch (storage.getKind()) {
+  case AccessStorage::Class: {
+    auto *varDecl = cast<VarDecl>(storageAndBase.getDecl());
+    // For class properties, a VarDecl can always be derived from AccessBase.
+    assert(varDecl && "no VarDecl for class property");
+    return varDecl;
+  }
   case AccessStorage::Global:
-  case AccessStorage::Class:
-    // Class and Globals are always a VarDecl, but the global decl may have a
-    // null value for global_addr -> phi.
-    return cast_or_null<VarDecl>(storageAndBase.getDecl());
+    return storageAndBase.getAccessBase().getGlobal();
   case AccessStorage::Box:
   case AccessStorage::Stack:
   case AccessStorage::Tail:
@@ -95,6 +101,21 @@ getDisjointAccessLocation(AccessStorageWithBase storageAndBase) {
   llvm_unreachable("unhandled kind");
 }
 
+static bool isVisibleExternally(DisjointAccessLocationKey key, SILModule *mod) {
+  if (auto *decl = key.dyn_cast<const VarDecl *>())
+    return mod->isVisibleExternally(decl);
+
+  auto *global = key.get<const SILGlobalVariable *>();
+  return isPossiblyUsedExternally(global->getLinkage(), mod->isWholeModule());
+}
+
+static StringRef getName(DisjointAccessLocationKey key) {
+  if (auto *decl = key.dyn_cast<const VarDecl *>())
+    return decl->getNameStr();
+
+  return key.get<const SILGlobalVariable *>()->getName();
+}
+
 namespace {
 // Implements an optimization to remove access markers on disjoint memory
 // locations that are never reentrantly accessed. For a given memory location,
@@ -134,7 +155,8 @@ class GlobalAccessRemoval {
     BeginAccessSet beginAccessSet;
   };
 
-  DenseMap<const VarDecl *, DisjointAccessLocationInfo> disjointAccessMap;
+  DenseMap<DisjointAccessLocationKey, DisjointAccessLocationInfo>
+      disjointAccessMap;
 
 public:
   GlobalAccessRemoval(SILModule &module) : module(module) {}
@@ -143,9 +165,8 @@ class GlobalAccessRemoval {
 
 protected:
   void visitInstruction(SILInstruction *I);
-  void recordAccess(SILInstruction *beginAccess, const VarDecl *decl,
-                    AccessStorage::Kind storageKind,
-                    bool hasNoNestedConflict);
+  void recordAccess(SILInstruction *beginAccess, DisjointAccessLocationKey key,
+                    AccessStorage::Kind storageKind, bool hasNoNestedConflict);
   void removeNonreentrantAccess();
 };
 } // namespace
@@ -169,15 +190,15 @@ void GlobalAccessRemoval::perform() {
 void GlobalAccessRemoval::visitInstruction(SILInstruction *I) {
   if (auto *BAI = dyn_cast<BeginAccessInst>(I)) {
     auto storageAndBase = AccessStorageWithBase::compute(BAI->getSource());
-    const VarDecl *decl = getDisjointAccessLocation(storageAndBase);
-    recordAccess(BAI, decl, storageAndBase.storage.getKind(),
+    auto key = getDisjointAccessLocation(storageAndBase);
+    recordAccess(BAI, key, storageAndBase.storage.getKind(),
                  BAI->hasNoNestedConflict());
     return;
   }
   if (auto *BUAI = dyn_cast<BeginUnpairedAccessInst>(I)) {
     auto storageAndBase = AccessStorageWithBase::compute(BUAI->getSource());
-    const VarDecl *decl = getDisjointAccessLocation(storageAndBase);
-    recordAccess(BUAI, decl, storageAndBase.storage.getKind(),
+    auto key = getDisjointAccessLocation(storageAndBase);
+    recordAccess(BUAI, key, storageAndBase.storage.getKind(),
                  BUAI->hasNoNestedConflict());
     return;
   }
@@ -213,21 +234,21 @@ void GlobalAccessRemoval::visitInstruction(SILInstruction *I) {
 // key_path instruction somewhere else in the same module (or it must be dead
 // code, or only access public properties).
 //
-// `decl` may be nullptr if the declaration can't be determined from the
+// `key` may be nullptr if the variable's identity cannot be determined from the
 // access. This is only legal when the access is known to be a local access, not
 // a class property or global.
 void GlobalAccessRemoval::recordAccess(SILInstruction *beginAccess,
-                                       const VarDecl *decl,
+                                       DisjointAccessLocationKey key,
                                        AccessStorage::Kind storageKind,
                                        bool hasNoNestedConflict) {
-  if (!decl || module.isVisibleExternally(decl))
+  if (key.isNull() || isVisibleExternally(key, &module))
     return;
 
   LLVM_DEBUG(if (!hasNoNestedConflict) llvm::dbgs()
-             << "Nested conflict on " << decl->getName() << " at"
-             << *beginAccess << "\n");
+             << "Nested conflict on " << getName(key) << " at" << *beginAccess
+             << "\n");
 
-  auto accessLocIter = disjointAccessMap.find(decl);
+  auto accessLocIter = disjointAccessMap.find(key);
   if (accessLocIter != disjointAccessMap.end()) {
     // Add this begin_access to an existing DisjointAccessLocationInfo.
     DisjointAccessLocationInfo &info = accessLocIter->second;
@@ -245,22 +266,21 @@ void GlobalAccessRemoval::recordAccess(SILInstruction *beginAccess,
   info.noNestedConflict = hasNoNestedConflict;
   if (auto *BA = dyn_cast<BeginAccessInst>(beginAccess))
     info.beginAccessSet.insert(BA);
-  disjointAccessMap.insert(std::make_pair(decl, info));
+  disjointAccessMap.insert(std::make_pair(key, info));
 }
 
 // For each unique storage within this function that is never reentrantly
 // accessed, promote all access checks for that storage to static enforcement.
 void GlobalAccessRemoval::removeNonreentrantAccess() {
-  for (auto &declAndInfo : disjointAccessMap) {
-    const DisjointAccessLocationInfo &info = declAndInfo.second;
+  for (auto &keyAndInfo : disjointAccessMap) {
+    const DisjointAccessLocationInfo &info = keyAndInfo.second;
     if (!info.noNestedConflict)
       continue;
 
-    const VarDecl *decl = declAndInfo.first;
-    LLVM_DEBUG(llvm::dbgs() << "Eliminating all formal access on "
-                            << decl->getName() << "\n");
-    assert(!module.isVisibleExternally(decl));
-    (void)decl;
+    auto key = keyAndInfo.first;
+    LLVM_DEBUG(llvm::dbgs()
+               << "Eliminating all formal access on " << getName(key) << "\n");
+    assert(!isVisibleExternally(key, &module));
 
     // Non-deterministic iteration, only used to set a flag.
     for (BeginAccessInst *beginAccess : info.beginAccessSet) {

test/SILOptimizer/access_storage_analysis.sil

@@ -723,3 +723,25 @@ bb3(%7 : $Builtin.RawPointer):
   end_access %9 : $*Int64
   return %10 : $Int64
 }
+
+// Test storage for SIL global variable declations.
+
+sil_global hidden @testGlobal : $Builtin.Int64
+
+sil hidden [global_init] @testAddressor : $@convention(thin) () -> Builtin.RawPointer {
+bb0:
+  %4 = global_addr @testGlobal : $*Builtin.Int64
+  %5 = address_to_pointer %4 : $*Builtin.Int64 to $Builtin.RawPointer
+  return %5 : $Builtin.RawPointer
+}
+
+sil hidden [transparent] @testGetter : $@convention(thin) () -> Builtin.Int64 {
+bb0:
+  %2 = function_ref @testAddressor : $@convention(thin) () -> Builtin.RawPointer
+  %3 = apply %2() : $@convention(thin) () -> Builtin.RawPointer
+  %4 = pointer_to_address %3 : $Builtin.RawPointer to [strict] $*Builtin.Int64
+  %5 = begin_access [read] [dynamic] %4 : $*Builtin.Int64
+  %6 = load %5 : $*Builtin.Int64
+  end_access %5 : $*Builtin.Int64
+  return %6 : $Builtin.Int64
+}