Merge pull request #32953 from atrick/generalize-memaccess

Generalize the MemAccessUtils API for use outside of access enforcement

Author: atrick

include/swift/SIL/MemAccessUtils.h

@@ -36,8 +36,13 @@ bool isExclusiveArgument(SILValue V);
 /// does not look through init/open_existential_addr.
 bool pointsToLocalObject(SILValue V);
 
-/// Returns true if \p V is a uniquely identified address or reference. It may
-/// be any of:
+/// Returns true if \p V is a uniquely identified address or reference. Two
+/// uniquely identified pointers with distinct roots cannot alias. However, a
+/// uniquely identified pointer may alias with unidentified pointers. For
+/// example, the uniquely identified pointer may escape to a call that returns an
+/// alias of that pointer.
+///
+/// It may be any of:
 ///
 /// - an address projection based on a locally allocated address with no
 /// indirection

include/swift/SILOptimizer/Analysis/ValueTracking.h

@@ -25,40 +25,17 @@ using namespace swift;
 //                            MARK: General Helpers
 //===----------------------------------------------------------------------===//
 
-SILValue swift::stripAccessMarkers(SILValue v) {
-  while (auto *bai = dyn_cast<BeginAccessInst>(v)) {
-    v = bai->getOperand();
-  }
-  return v;
-}
-
-// The resulting projection must have an address-type operand at index zero
-// representing the projected address.
-SingleValueInstruction *swift::isAccessProjection(SILValue v) {
-  switch (v->getKind()) {
-  default:
-    return nullptr;
-
-  case ValueKind::StructElementAddrInst:
-  case ValueKind::TupleElementAddrInst:
-  case ValueKind::UncheckedTakeEnumDataAddrInst:
-  case ValueKind::TailAddrInst:
-  case ValueKind::IndexAddrInst:
-    return cast<SingleValueInstruction>(v);
-  };
-}
-
 // TODO: When the optimizer stops stripping begin_access markers, then we should
 // be able to assert that the result is a BeginAccessInst and the default case
 // is unreachable.
 SILValue swift::getAddressAccess(SILValue v) {
   while (true) {
     assert(v->getType().isAddress());
-    auto *projection = isAccessProjection(v);
+    auto projection = AccessProjection(v);
     if (!projection)
       return v;
 
-    v = projection->getOperand(0);
+    v = projection.baseAddress();
   }
 }
 
@@ -240,26 +217,26 @@ void AccessedStorage::print(raw_ostream &os) const {
 void AccessedStorage::dump() const { print(llvm::dbgs()); }
 
 namespace {
-struct FindAccessedStorageVisitor
-  : public AccessUseDefChainVisitor<FindAccessedStorageVisitor>
-{
+template <typename Impl>
+struct FindAccessedStorageVisitorBase : public AccessUseDefChainVisitor<Impl> {
+protected:
   SmallVector<SILValue, 8> addressWorklist;
   SmallPtrSet<SILPhiArgument *, 4> visitedPhis;
   Optional<AccessedStorage> storage;
 
 public:
-  FindAccessedStorageVisitor(SILValue firstSourceAddr)
-    : addressWorklist({firstSourceAddr})
-  {}
-  
-  AccessedStorage doIt() {
+  FindAccessedStorageVisitorBase(SILValue firstSourceAddr)
+      : addressWorklist({firstSourceAddr}) {}
+
+  // Main entry point.
+  AccessedStorage findStorage() {
     while (!addressWorklist.empty()) {
-      visit(addressWorklist.pop_back_val());
+      this->visit(addressWorklist.pop_back_val());
     }
 
     return storage.getValueOr(AccessedStorage());
   }
-  
+
   void setStorage(AccessedStorage foundStorage) {
     if (!storage) {
       storage = foundStorage;
@@ -292,20 +269,32 @@ struct FindAccessedStorageVisitor
     addressWorklist.push_back(parentAddr);
   }
 };
+struct FindAccessedStorageVisitor
+    : public FindAccessedStorageVisitorBase<FindAccessedStorageVisitor> {
+
+  FindAccessedStorageVisitor(SILValue sourceAddr)
+      : FindAccessedStorageVisitorBase(sourceAddr) {}
+
+  void visitNestedAccess(BeginAccessInst *access) {
+    addressWorklist.push_back(access->getSource());
+  }
+};
+
+struct IdentifyAccessedStorageVisitor
+    : public FindAccessedStorageVisitorBase<IdentifyAccessedStorageVisitor> {
+
+  IdentifyAccessedStorageVisitor(SILValue sourceAddr)
+      : FindAccessedStorageVisitorBase(sourceAddr) {}
+};
+
 } // namespace
 
 AccessedStorage swift::findAccessedStorage(SILValue sourceAddr) {
-  return FindAccessedStorageVisitor(sourceAddr).doIt();
+  return FindAccessedStorageVisitor(sourceAddr).findStorage();
 }
 
-AccessedStorage swift::findAccessedStorageNonNested(SILValue sourceAddr) {
-  while (true) {
-    const AccessedStorage &storage = findAccessedStorage(sourceAddr);
-    if (!storage || storage.getKind() != AccessedStorage::Nested)
-      return storage;
-    
-    sourceAddr = cast<BeginAccessInst>(storage.getValue())->getSource();
-  }
+AccessedStorage swift::identifyAccessedStorageImpl(SILValue sourceAddr) {
+  return IdentifyAccessedStorageVisitor(sourceAddr).findStorage();
 }
 
 //===----------------------------------------------------------------------===//
@@ -485,10 +474,9 @@ bool swift::isPossibleFormalAccessBase(const AccessedStorage &storage,
   case AccessedStorage::Nested: {
     // A begin_access is considered a separate base for the purpose of conflict
     // checking. However, for the purpose of inserting unenforced markers and
-    // performaing verification, it needs to be ignored.
+    // performing verification, it needs to be ignored.
     auto *BAI = cast<BeginAccessInst>(storage.getValue());
-    const AccessedStorage &nestedStorage =
-      findAccessedStorage(BAI->getSource());
+    const AccessedStorage &nestedStorage = identifyFormalAccess(BAI);
     if (!nestedStorage)
       return false;
 

lib/SIL/Utils/MemAccessUtils.cpp

@@ -1948,20 +1948,20 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
     // also requires that Unidentified access fit a whitelist on known
     // non-internal globals or class properties.
     //
-    // First check that findAccessedStorage returns without asserting. For
+    // First check that identifyFormalAccess returns without asserting. For
     // Unsafe enforcement, that is sufficient. For any other enforcement
     // level also require that it returns a valid AccessedStorage object.
     // Unsafe enforcement is used for some unrecognizable access patterns,
     // like debugger variables. The compiler never cares about the source of
     // those accesses.
-    findAccessedStorage(BAI->getSource());
+    identifyFormalAccess(BAI);
     // FIXME: rdar://57291811 - the following check for valid storage will be
     // reenabled shortly. A fix is planned. In the meantime, the possiblity that
     // a real miscompilation could be caused by this failure is insignificant.
     // I will probably enable a much broader SILVerification of address-type
     // block arguments first to ensure we never hit this check again.
     /*
-    AccessedStorage storage = findAccessedStorage(BAI->getSource());
+    AccessedStorage storage = identifyFormalAccess(BAI);
     if (BAI->getEnforcement() != SILAccessEnforcement::Unsafe)
       require(storage, "Unknown formal access pattern");
     */
@@ -2000,8 +2000,8 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
       break;
     }
 
-    // First check that findAccessedStorage never asserts.
-    AccessedStorage storage = findAccessedStorage(BUAI->getSource());
+    // First check that identifyFormalAccess never asserts.
+    AccessedStorage storage = identifyFormalAccess(BUAI);
     // Only allow Unsafe and Builtin access to have invalid storage.
     if (BUAI->getEnforcement() != SILAccessEnforcement::Unsafe
         && !BUAI->isFromBuiltin()) {

lib/SIL/Verifier/SILVerifier.cpp

@@ -253,7 +253,7 @@ transformCalleeStorage(const StorageAccessInfo &storage,
     SILValue argVal = getCallerArg(fullApply, storage.getParamIndex());
     if (argVal) {
       // Remap the argument source value and inherit the old storage info.
-      auto calleeStorage = findAccessedStorageNonNested(argVal);
+      auto calleeStorage = findAccessedStorage(argVal);
       if (calleeStorage)
         return StorageAccessInfo(calleeStorage, storage);
     }
@@ -262,7 +262,7 @@ transformCalleeStorage(const StorageAccessInfo &storage,
     //
     // This is an untested bailout. It is only reachable if the call graph
     // contains an edge that getCallerArg is unable to analyze OR if
-    // findAccessedStorageNonNested returns an invalid SILValue, which won't
+    // findAccessedStorage returns an invalid SILValue, which won't
     // pass SIL verification.
     //
     // FIXME: In case argVal is invalid, support Unidentified access for invalid
@@ -299,7 +299,7 @@ void AccessedStorageResult::visitBeginAccess(B *beginAccess) {
     return;
 
   const AccessedStorage &storage =
-      findAccessedStorageNonNested(beginAccess->getSource());
+      findAccessedStorage(beginAccess->getSource());
 
   if (storage.getKind() == AccessedStorage::Unidentified) {
     // This also catches invalid storage.

lib/SILOptimizer/Analysis/AccessedStorageAnalysis.cpp

@@ -700,19 +700,18 @@ static bool analyzeBeginAccess(BeginAccessInst *BI,
                                InstSet &SideEffectInsts,
                                AccessedStorageAnalysis *ASA,
                                DominanceInfo *DT) {
-  const AccessedStorage &storage =
-      findAccessedStorageNonNested(BI->getSource());
+  const AccessedStorage &storage = findAccessedStorage(BI->getSource());
   if (!storage) {
     return false;
   }
 
-  auto BIAccessedStorageNonNested = findAccessedStorageNonNested(BI);
+  auto BIAccessedStorageNonNested = findAccessedStorage(BI);
   auto safeBeginPred = [&](BeginAccessInst *OtherBI) {
     if (BI == OtherBI) {
       return true;
     }
     return BIAccessedStorageNonNested.isDistinctFrom(
-        findAccessedStorageNonNested(OtherBI));
+        findAccessedStorage(OtherBI));
   };
 
   if (!std::all_of(BeginAccesses.begin(), BeginAccesses.end(), safeBeginPred))

lib/SILOptimizer/LoopTransforms/LICM.cpp

@@ -583,16 +583,6 @@ static void diagnoseExclusivityViolation(const ConflictingAccess &Violation,
       .highlight(NoteAccess.getAccessLoc().getSourceRange());
 }
 
-/// Look through a value to find the underlying storage accessed.
-static AccessedStorage findValidAccessedStorage(SILValue Source) {
-  const AccessedStorage &Storage = findAccessedStorage(Source);
-  if (!Storage) {
-    llvm::dbgs() << "Bad memory access source: " << Source;
-    llvm_unreachable("Unexpected access source.");
-  }
-  return Storage;
-}
-
 /// Returns true when the apply calls the Standard Library swap().
 /// Used for fix-its to suggest replacing with Collection.swapAt()
 /// on exclusivity violations.
@@ -700,7 +690,7 @@ struct AccessState {
 
 // Find conflicting access on each argument using AccessSummaryAnalysis.
 static void
-checkCaptureAccess(ApplySite Apply, AccessState &State,
+checkAccessSummary(ApplySite Apply, AccessState &State,
                    const AccessSummaryAnalysis::FunctionSummary &FS) {
   for (unsigned ArgumentIndex : range(Apply.getNumArguments())) {
 
@@ -721,7 +711,7 @@ checkCaptureAccess(ApplySite Apply, AccessState &State,
 
     // A valid AccessedStorage should always be found because Unsafe accesses
     // are not tracked by AccessSummaryAnalysis.
-    const AccessedStorage &Storage = findValidAccessedStorage(Argument);
+    const AccessedStorage &Storage = identifyCapturedStorage(Argument);
     auto AccessIt = State.Accesses->find(Storage);
 
     // Are there any accesses in progress at the time of the call?
@@ -743,7 +733,7 @@ static void checkCaptureAccess(ApplySite Apply, AccessState &State) {
   // dynamically replaceable.
   SILFunction *Callee = Apply.getCalleeFunction();
   if (Callee && !Callee->empty()) {
-    checkCaptureAccess(Apply, State, State.ASA->getOrCreateSummary(Callee));
+    checkAccessSummary(Apply, State, State.ASA->getOrCreateSummary(Callee));
     return;
   }
   // In the absence of AccessSummaryAnalysis, conservatively assume by-address
@@ -755,7 +745,7 @@ static void checkCaptureAccess(ApplySite Apply, AccessState &State) {
 
     // A valid AccessedStorage should always be found because Unsafe accesses
     // are not tracked by AccessSummaryAnalysis.
-    const AccessedStorage &Storage = findValidAccessedStorage(argOper.get());
+    const AccessedStorage &Storage = identifyCapturedStorage(argOper.get());
 
     // Are there any accesses in progress at the time of the call?
     auto AccessIt = State.Accesses->find(Storage);
@@ -845,8 +835,8 @@ static void checkForViolationsAtInstruction(SILInstruction &I,
       return;
 
     SILAccessKind Kind = BAI->getAccessKind();
-    const AccessedStorage &Storage =
-      findValidAccessedStorage(BAI->getSource());
+    const AccessedStorage &Storage = identifyFormalAccess(BAI);
+    assert(Storage && "unidentified formal access");
     // Storage may be associated with a nested access where the outer access is
     // "unsafe". That's ok because the outer access can itself be treated like a
     // valid source, as long as we don't ask for its source.
@@ -862,14 +852,15 @@ static void checkForViolationsAtInstruction(SILInstruction &I,
   }
 
   if (auto *EAI = dyn_cast<EndAccessInst>(&I)) {
-    if (EAI->getBeginAccess()->getEnforcement() == SILAccessEnforcement::Unsafe)
+    BeginAccessInst *BAI = EAI->getBeginAccess();
+    if (BAI->getEnforcement() == SILAccessEnforcement::Unsafe)
       return;
 
-    auto It =
-      State.Accesses->find(findValidAccessedStorage(EAI->getSource()));
+    const AccessedStorage &Storage = identifyFormalAccess(BAI);
+    assert(Storage && "unidentified formal access");
+    auto It = State.Accesses->find(identifyFormalAccess(BAI));
     AccessInfo &Info = It->getSecond();
 
-    BeginAccessInst *BAI = EAI->getBeginAccess();
     const IndexTrieNode *SubPath = State.ASA->findSubPathAccessed(BAI);
     Info.endAccess(EAI, SubPath);
 
@@ -973,7 +964,7 @@ static void checkStaticExclusivity(SILFunction &Fn, PostOrderFunctionInfo *PO,
 // Check that the given address-type operand is guarded by begin/end access
 // markers.
 static void checkAccessedAddress(Operand *memOper, StorageMap &Accesses) {
-  SILValue address = memOper->get();
+  SILValue address = getAddressAccess(memOper->get());
   SILInstruction *memInst = memOper->getUser();
 
   auto error = [address, memInst]() {
@@ -985,6 +976,21 @@ static void checkAccessedAddress(Operand *memOper, StorageMap &Accesses) {
     abort();
   };
 
+  // Check if this address is guarded by an access.
+  if (auto *BAI = dyn_cast<BeginAccessInst>(address)) {
+    if (BAI->getEnforcement() == SILAccessEnforcement::Unsafe)
+      return;
+
+    const AccessedStorage &Storage = identifyFormalAccess(BAI);
+    assert(Storage && "unidentified formal access");
+    AccessInfo &Info = Accesses[Storage];
+    if (Info.hasAccessesInProgress())
+      return;
+
+    error();
+  }
+  // --- This address is not guarded by a begin_access.
+
   // If the memory instruction is only used for initialization, it doesn't need
   // an access marker.
   if (memInstMustInitialize(memOper))
@@ -1011,7 +1017,6 @@ static void checkAccessedAddress(Operand *memOper, StorageMap &Accesses) {
       return;
   }
 
-  // Strip off address projections, but not ref_element_addr.
   const AccessedStorage &storage = findAccessedStorage(address);
   // findAccessedStorage may return an invalid storage object if the address
   // producer is not recognized by its whitelist. For the purpose of
@@ -1040,18 +1045,6 @@ static void checkAccessedAddress(Operand *memOper, StorageMap &Accesses) {
     return;
   }
 
-  // Otherwise, the address base should be an in-scope begin_access.
-  if (storage.getKind() == AccessedStorage::Nested) {
-    auto *BAI = cast<BeginAccessInst>(storage.getValue());
-    if (BAI->getEnforcement() == SILAccessEnforcement::Unsafe)
-      return;
-
-    const AccessedStorage &Storage = findValidAccessedStorage(BAI->getSource());
-    AccessInfo &Info = Accesses[Storage];
-    if (!Info.hasAccessesInProgress())
-      error();
-    return;
-  }
   error();
 }