Fix SIL MemBehavior queries with access markers.

This is in prepration for other bug fixes.

Clarify the SIL utilities that return canonical address values for
formal access given the address used by some memory operation:

- stripAccessMarkers
- getAddressAccess
- getAccessedAddress

These are closely related to the code in MemAccessUtils.

Make sure passes use these utilities consistently so that
optimizations aren't defeated by normal variations in SIL patterns.

Create an isLetAddress() utility alongside these basic utilities to
make sure it is used consistently with the address corresponding to
formal access. When this query is used inconsistently, it defeats
optimization. It can also cause correctness bugs because some
optimizations assume that 'let' initialization is only performed on a
unique address value.

Functional changes to Memory Behavior:

- An instruction with side effects now conservatively still has side
  effects even when the queried value is a 'let'. Let values are
  certainly sensitive to side effects, such as the parent object being
  deallocated.

- Return the correct MemBehavior for begin/end_access markers.

Author: atrick

include/swift/SIL/InstructionUtils.h

@@ -25,13 +25,6 @@ namespace swift {
 /// nothing left to strip.
 SILValue getUnderlyingObject(SILValue V);
 
-/// Strip off indexing and address projections.
-///
-/// This is similar to getUnderlyingObject, except that it does not strip any
-/// object-to-address projections, like ref_element_addr. In other words, the
-/// result is always an address value.
-SILValue getUnderlyingAddressRoot(SILValue V);
-
 SILValue getUnderlyingObjectStopAtMarkDependence(SILValue V);
 
 SILValue stripSinglePredecessorArgs(SILValue V);
@@ -56,11 +49,6 @@ SILValue stripUpCasts(SILValue V);
 /// upcasts and downcasts.
 SILValue stripClassCasts(SILValue V);
 
-/// Return the underlying SILValue after stripping off non-projection address
-/// casts. The result will still be an address--this does not look through
-/// pointer-to-address.
-SILValue stripAddressAccess(SILValue V);
-
 /// Return the underlying SILValue after stripping off all address projection
 /// instructions.
 SILValue stripAddressProjections(SILValue V);

include/swift/SIL/MemAccessUtils.h

@@ -46,7 +46,50 @@
 
 namespace swift {
 
-// stripAddressAccess() is declared in InstructionUtils.h.
+/// Get the base address of a formal access by stripping access markers and
+/// borrows.
+///
+/// If \p v is an address, then the returned value is also an address
+/// (pointer-to-address is not stripped).
+SILValue stripAccessMarkers(SILValue v);
+
+/// Attempt to return the address corresponding to a variable's formal access
+/// by stripping indexing and address projections.
+///
+/// \p v must be an address.
+///
+/// Returns an address. If the a formal access was successfully identified, and
+/// access markers have not yet been removed, then the returned address is
+/// produced by a begin_access marker.
+///
+/// To get the base address of the formal access behind the access marker,
+/// either call stripAccessMarkers() on the returned value, or call
+/// getAccessedAddress() on \p v.
+///
+/// To identify the underlying storage object of the access, use
+/// findAccessedStorage() on either \p v or the returned address.
+SILValue getAddressAccess(SILValue v);
+
+/// Convenience for stripAccessMarkers(getAddressAccess(V)).
+SILValue getAccessedAddress(SILValue v);
+
+/// Return true if \p accessedAddress points to a let-variable.
+///
+/// Precondition: \p accessedAddress must be an address-type value representing
+/// the base of a formal access (not a projection within the access).
+///
+/// let-variables are only written during let-variable initialization, which is
+/// assumed to store directly to the same, unaliased accessedAddress.
+///
+/// The address of a let-variable must be the base of a formal access . A 'let'
+/// member of a struct is *not* a let-variable, because it's memory may be
+/// written when formally modifying the outer struct. A let-variable is either
+/// an entire local variable, global variable, or class property (this is the
+/// definition of the base address of a formal access).
+///
+/// The caller should derive the accessed address using
+/// stripAccessMarkers(getAccessedAddress(ptr)).
+bool isLetAddress(SILValue accessedAddress);
 
 inline bool accessKindMayConflict(SILAccessKind a, SILAccessKind b) {
   return !(a == SILAccessKind::Read && b == SILAccessKind::Read);
@@ -104,7 +147,7 @@ inline bool accessKindMayConflict(SILAccessKind a, SILAccessKind b) {
 class AccessedStorage {
 public:
   /// Enumerate over all valid begin_access bases. Clients can use a covered
-  /// switch to warn if findAccessedAddressBase ever adds a case.
+  /// switch to warn if AccessedStorage ever adds a case.
   enum Kind : uint8_t {
     Box,
     Stack,

include/swift/SIL/SILNodes.def

@@ -591,6 +591,9 @@ ABSTRACT_VALUE_AND_INST(SingleValueInstruction, ValueBase, SILInstruction)
                     SingleValueInstruction, MayHaveSideEffects, DoesNotRelease)
   SINGLE_VALUE_INST(StoreBorrowInst, store_borrow,
                     SILInstruction, MayWrite, DoesNotRelease)
+  // begin_access may trap. Trapping is unordered with respect to memory access,
+  // and with respect to other traps, but it is still conservatively considered
+  // a side effect.
   SINGLE_VALUE_INST(BeginAccessInst, begin_access,
                     SingleValueInstruction, MayHaveSideEffects, DoesNotRelease)
 #define NEVER_OR_SOMETIMES_LOADABLE_CHECKED_REF_STORAGE(Name, name, ...) \
@@ -803,6 +806,10 @@ NON_VALUE_INST(DestroyValueInst, destroy_value,
                SILInstruction, MayHaveSideEffects, MayRelease)
 NON_VALUE_INST(EndBorrowInst, end_borrow,
                SILInstruction, MayHaveSideEffects, DoesNotRelease)
+// end_access is considered to have side effects because it modifies runtime
+// state outside of the memory modified by the access that is visible to
+// Swift. This "side effect" only needs to creates a dependency on begin_access
+// instructions.
 NON_VALUE_INST(EndAccessInst, end_access,
                SILInstruction, MayHaveSideEffects, DoesNotRelease)
 NON_VALUE_INST(BeginUnpairedAccessInst, begin_unpaired_access,

include/swift/SILOptimizer/Analysis/AliasAnalysis.h

@@ -299,10 +299,6 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
 /// Otherwise, return an empty type.
 SILType computeTBAAType(SILValue V);
 
-/// Check if \p V points to a let-member.
-/// Nobody can write into let members.
-bool isLetPointer(SILValue V);
-
 } // end namespace swift
 
 namespace llvm {

include/swift/SILOptimizer/Analysis/ValueTracking.h

@@ -17,7 +17,7 @@
 #ifndef SWIFT_SILOPTIMIZER_ANALYSIS_VALUETRACKING_H
 #define SWIFT_SILOPTIMIZER_ANALYSIS_VALUETRACKING_H
 
-#include "swift/SIL/InstructionUtils.h"
+#include "swift/SIL/MemAccessUtils.h"
 #include "swift/SIL/SILArgument.h"
 #include "swift/SIL/SILInstruction.h"
 
@@ -50,7 +50,8 @@ bool pointsToLocalObject(SILValue V);
 /// indirection (e.g. ref_element_addr, project_box, etc.).
 inline bool isUniquelyIdentified(SILValue V) {
   return pointsToLocalObject(V)
-         || isExclusiveArgument(getUnderlyingAddressRoot(V));
+         || (V->getType().isAddress()
+             && isExclusiveArgument(getAccessedAddress(V)));
 }
 
 enum class IsZeroKind {

lib/IRGen/IRGenSIL.cpp

@@ -16,41 +16,42 @@
 //===----------------------------------------------------------------------===//
 
 #define DEBUG_TYPE "irgensil"
-#include "llvm/IR/DIBuilder.h"
-#include "llvm/IR/Function.h"
-#include "llvm/IR/Module.h"
-#include "llvm/IR/Instructions.h"
-#include "llvm/IR/IntrinsicInst.h"
-#include "llvm/IR/InlineAsm.h"
-#include "llvm/IR/Intrinsics.h"
-#include "llvm/ADT/MapVector.h"
-#include "llvm/ADT/SmallBitVector.h"
-#include "llvm/ADT/TinyPtrVector.h"
-#include "llvm/Support/SaveAndRestore.h"
-#include "llvm/Support/Debug.h"
-#include "llvm/Transforms/Utils/Local.h"
-#include "clang/AST/ASTContext.h"
-#include "clang/Basic/TargetInfo.h"
-#include "swift/Basic/ExternalUnion.h"
-#include "swift/Basic/Range.h"
-#include "swift/Basic/STLExtras.h"
 #include "swift/AST/ASTContext.h"
 #include "swift/AST/IRGenOptions.h"
-#include "swift/AST/Pattern.h"
 #include "swift/AST/ParameterList.h"
+#include "swift/AST/Pattern.h"
 #include "swift/AST/SubstitutionMap.h"
 #include "swift/AST/Types.h"
+#include "swift/Basic/ExternalUnion.h"
+#include "swift/Basic/Range.h"
+#include "swift/Basic/STLExtras.h"
 #include "swift/SIL/ApplySite.h"
 #include "swift/SIL/Dominance.h"
+#include "swift/SIL/InstructionUtils.h"
+#include "swift/SIL/MemAccessUtils.h"
 #include "swift/SIL/PrettyStackTrace.h"
 #include "swift/SIL/SILDebugScope.h"
 #include "swift/SIL/SILDeclRef.h"
 #include "swift/SIL/SILLinkage.h"
 #include "swift/SIL/SILModule.h"
 #include "swift/SIL/SILType.h"
 #include "swift/SIL/SILVisitor.h"
-#include "swift/SIL/InstructionUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/Basic/TargetInfo.h"
 #include "clang/CodeGen/CodeGenABITypes.h"
+#include "llvm/ADT/MapVector.h"
+#include "llvm/ADT/SmallBitVector.h"
+#include "llvm/ADT/TinyPtrVector.h"
+#include "llvm/IR/DIBuilder.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/InlineAsm.h"
+#include "llvm/IR/Instructions.h"
+#include "llvm/IR/IntrinsicInst.h"
+#include "llvm/IR/Intrinsics.h"
+#include "llvm/IR/Module.h"
+#include "llvm/Support/Debug.h"
+#include "llvm/Support/SaveAndRestore.h"
+#include "llvm/Transforms/Utils/Local.h"
 
 #include "CallEmission.h"
 #include "Explosion.h"
@@ -3567,8 +3568,11 @@ void IRGenSILFunction::visitRefTailAddrInst(RefTailAddrInst *i) {
 }
 
 static bool isInvariantAddress(SILValue v) {
-  auto root = getUnderlyingAddressRoot(v);
-  if (auto ptrRoot = dyn_cast<PointerToAddressInst>(root)) {
+  SILValue accessedAddress = getAccessedAddress(v);
+  if (accessedAddress->getType().isAddress() && isLetAddress(accessedAddress)) {
+    return true;
+  }
+  if (auto *ptrRoot = dyn_cast<PointerToAddressInst>(accessedAddress)) {
     return ptrRoot->isInvariant();
   }
   // TODO: We could be more aggressive about considering addresses based on

lib/SIL/InstructionUtils.cpp

@@ -52,28 +52,6 @@ SILValue swift::getUnderlyingObject(SILValue v) {
   }
 }
 
-/// Strip off casts and address projections into the interior of a value. Unlike
-/// getUnderlyingObject, this does not find the root of a heap object--a class
-/// property is itself an address root.
-SILValue swift::getUnderlyingAddressRoot(SILValue v) {
-  while (true) {
-    SILValue v2 = stripIndexingInsts(stripCasts(v));
-    v2 = stripOwnershipInsts(v2);
-    switch (v2->getKind()) {
-    case ValueKind::StructElementAddrInst:
-    case ValueKind::TupleElementAddrInst:
-    case ValueKind::UncheckedTakeEnumDataAddrInst:
-      v2 = cast<SingleValueInstruction>(v2)->getOperand(0);
-      break;
-    default:
-      break;
-    }
-    if (v2 == v)
-      return v2;
-    v = v2;
-  }
-}
-
 SILValue swift::getUnderlyingObjectStopAtMarkDependence(SILValue v) {
   while (true) {
     SILValue v2 = stripCastsWithoutMarkDependence(v);
@@ -224,18 +202,6 @@ SILValue swift::stripClassCasts(SILValue v) {
   }
 }
 
-SILValue swift::stripAddressAccess(SILValue V) {
-  while (true) {
-    switch (V->getKind()) {
-    default:
-      return V;
-    case ValueKind::BeginBorrowInst:
-    case ValueKind::BeginAccessInst:
-      V = cast<SingleValueInstruction>(V)->getOperand(0);
-    }
-  }
-}
-
 SILValue swift::stripAddressProjections(SILValue V) {
   while (true) {
     V = stripSinglePredecessorArgs(V);

lib/SIL/MemAccessUtils.cpp

@@ -21,6 +21,56 @@
 
 using namespace swift;
 
+SILValue swift::stripAccessMarkers(SILValue v) {
+  while (true) {
+    switch (v->getKind()) {
+    default:
+      return v;
+
+    case ValueKind::BeginBorrowInst:
+    case ValueKind::BeginAccessInst:
+      v = cast<SingleValueInstruction>(v)->getOperand(0);
+    }
+  }
+}
+
+// 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::getAccessedAddress(SILValue v) {
+  assert(v->getType().isAddress());
+  while (true) {
+    switch (v->getKind()) {
+    default:
+      return v;
+
+    case ValueKind::BeginBorrowInst:
+    case ValueKind::StructElementAddrInst:
+    case ValueKind::TupleElementAddrInst:
+    case ValueKind::UncheckedTakeEnumDataAddrInst:
+    case ValueKind::TailAddrInst:
+    case ValueKind::IndexAddrInst:
+      v = cast<SingleValueInstruction>(v)->getOperand(0);
+      continue;
+    };
+  }
+}
+
+bool swift::isLetAddress(SILValue accessedAddress) {
+  assert(accessedAddress == getAccessedAddress(accessedAddress)
+         && "caller must find the address root");
+  // Is this an address of a "let" class member?
+  if (auto *rea = dyn_cast<RefElementAddrInst>(accessedAddress))
+    return rea->getField()->isLet();
+
+  // Is this an address of a global "let"?
+  if (auto *gai = dyn_cast<GlobalAddrInst>(accessedAddress)) {
+    auto *globalDecl = gai->getReferencedGlobal()->getDecl();
+    return globalDecl && globalDecl->isLet();
+  }
+  return false;
+}
+
 AccessedStorage::AccessedStorage(SILValue base, Kind kind) {
   assert(base && "invalid storage base");
   initKind(kind);

lib/SILOptimizer/Analysis/AliasAnalysis.cpp

@@ -330,14 +330,18 @@ static bool isTypedAccessOracle(SILInstruction *I) {
 /// given value is directly derived from a memory location, it cannot
 /// alias. Call arguments also cannot alias because they must follow \@in, @out,
 /// @inout, or \@in_guaranteed conventions.
-static bool isAddressRootTBAASafe(SILValue V) {
-  if (isa<SILFunctionArgument>(V))
+static bool isAccessedAddressTBAASafe(SILValue V) {
+  if (!V->getType().isAddress())
+    return false;
+
+  SILValue accessedAddress = getAccessedAddress(V);
+  if (isa<SILFunctionArgument>(accessedAddress))
     return true;
 
-  if (auto *PtrToAddr = dyn_cast<PointerToAddressInst>(V))
+  if (auto *PtrToAddr = dyn_cast<PointerToAddressInst>(accessedAddress))
     return PtrToAddr->isStrict();
 
-  switch (V->getKind()) {
+  switch (accessedAddress->getKind()) {
   default:
     return false;
   case ValueKind::AllocStackInst:
@@ -352,6 +356,8 @@ static bool isAddressRootTBAASafe(SILValue V) {
 /// TypedAccessOracle which enable one to ascertain via undefined behavior the
 /// "true" type of the instruction.
 static SILType findTypedAccessType(SILValue V) {
+  assert(V->getType().isAddress());
+
   // First look at the origin of V and see if we have any instruction that is a
   // typed oracle.
   // TODO: MultiValueInstruction
@@ -370,7 +376,7 @@ static SILType findTypedAccessType(SILValue V) {
 }
 
 SILType swift::computeTBAAType(SILValue V) {
-  if (isAddressRootTBAASafe(getUnderlyingAddressRoot(V)))
+  if (isAccessedAddressTBAASafe(V))
     return findTypedAccessType(V);
 
   // FIXME: add ref_element_addr check here. TBAA says that objects cannot be
@@ -743,34 +749,6 @@ bool AliasAnalysis::mayValueReleaseInterfereWithInstruction(
   return EA->mayReleaseContent(releasedReference, accessedPointer);
 }
 
-bool swift::isLetPointer(SILValue V) {
-  // Traverse the "access" path for V and check that it starts with "let"
-  // and everything along this path is a value-type (i.e. struct or tuple).
-
-  // Is this an address of a "let" class member?
-  if (auto *REA = dyn_cast<RefElementAddrInst>(V))
-    return REA->getField()->isLet();
-
-  // Is this an address of a global "let"?
-  if (auto *GAI = dyn_cast<GlobalAddrInst>(V)) {
-    auto *GlobalDecl = GAI->getReferencedGlobal()->getDecl();
-    return GlobalDecl && GlobalDecl->isLet();
-  }
-
-  // Is this an address of a struct "let" member?
-  if (auto *SEA = dyn_cast<StructElementAddrInst>(V))
-    // Check if it is a "let" in the parent struct.
-    // Check if its parent is a "let".
-    return isLetPointer(SEA->getOperand());
-
-
-  // Check if a parent of a tuple is a "let"
-  if (auto *TEA = dyn_cast<TupleElementAddrInst>(V))
-    return isLetPointer(TEA->getOperand());
-
-  return false;
-}
-
 void AliasAnalysis::initialize(SILPassManager *PM) {
   SEA = PM->getAnalysis<SideEffectAnalysis>();
   EA = PM->getAnalysis<EscapeAnalysis>();