Merge pull request swiftlang#30173 from atrick/fix-temp-rvalue-access

Fix MemBehavior and TempRValueOpt to handle 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>();