SILOptimizer: Fix potential crashes with uses of replaceLoadSequence()

This utility is generally a horrible idea but even worse the
callers were not doing anything to ensure the required
invariants actually held.

Add a new canReplaceLoadSequence() method and chek it in the
right places.

Author: slavapestov

include/swift/SILOptimizer/Utils/Local.h

@@ -535,13 +535,15 @@ bool isSimpleType(SILType SILTy, SILModule& Module);
 bool analyzeStaticInitializer(SILValue V,
                               SmallVectorImpl<SILInstruction *> &Insns);
 
+/// Returns true if the below operation will succeed.
+bool canReplaceLoadSequence(SILInstruction *I);
+
 /// Replace load sequence which may contain
 /// a chain of struct_element_addr followed by a load.
 /// The sequence is traversed inside out, i.e.
 /// starting with the innermost struct_element_addr
 void replaceLoadSequence(SILInstruction *I,
-                         SILValue Value,
-                         SILBuilder &B);
+                         SILValue Value);
 
 
 /// Do we have enough information to determine all callees that could

lib/SILOptimizer/IPO/GlobalOpt.cpp

@@ -651,15 +651,23 @@ replaceLoadsByKnownValue(BuiltinInst *CallToOnce, SILFunction *AddrF,
   // Generate a getter from InitF which returns the value of the global.
   auto *GetterF = genGetterFromInit(FunctionBuilder, InitF, SILG->getDecl());
 
-  // Replace all calls of an addressor by calls of a getter .
+  // Replace all calls of an addressor by calls of a getter.
   for (int i = 0, e = Calls.size(); i < e; ++i) {
     auto *Call = Calls[i];
 
-    // Now find all uses of Call. They all should be loads, so that
-    // we can replace it.
+    // Make sure that we can go ahead and replace all uses of the
+    // address with the value.
     bool isValid = true;
     for (auto Use : Call->getUses()) {
-      if (!isa<PointerToAddressInst>(Use->getUser())) {
+      if (auto *PTAI = dyn_cast<PointerToAddressInst>(Use->getUser())) {
+        for (auto PTAIUse : PTAI->getUses()) {
+          SILInstruction *Use = PTAIUse->getUser();
+          if (!canReplaceLoadSequence(Use)) {
+            isValid = false;
+            break;
+          }
+        }
+      } else {
         isValid = false;
         break;
       }
@@ -668,6 +676,8 @@ replaceLoadsByKnownValue(BuiltinInst *CallToOnce, SILFunction *AddrF,
     if (!isValid)
       continue;
 
+    // Now find all uses of Call. They all should be loads, so that
+    // we can replace it.
     SILBuilderWithScope B(Call);
     SmallVector<SILValue, 1> Args;
     auto *GetterRef = B.createFunctionRef(Call->getLoc(), GetterF);
@@ -681,16 +691,10 @@ replaceLoadsByKnownValue(BuiltinInst *CallToOnce, SILFunction *AddrF,
       auto *PTAI = dyn_cast<PointerToAddressInst>(Use->getUser());
       assert(PTAI && "All uses should be pointer_to_address");
       for (auto PTAIUse : PTAI->getUses()) {
-        SILInstruction *Load = PTAIUse->getUser();
-        if (auto *CA = dyn_cast<CopyAddrInst>(Load)) {
-          // The result of the initializer is stored to another location.
-          SILBuilder B(CA);
-          B.createStore(CA->getLoc(), NewAI, CA->getDest(),
-                        StoreOwnershipQualifier::Unqualified);
-        } else {
-          // The result of the initializer is used as a value.
-          replaceLoadSequence(Load, NewAI, B);
-        }
+        SILInstruction *Use = PTAIUse->getUser();
+
+        // The result of the getter is used as a value.
+        replaceLoadSequence(Use, NewAI);
       }
     }
 

lib/SILOptimizer/IPO/LetPropertiesOpts.cpp

@@ -210,22 +210,18 @@ void LetPropertiesOpt::optimizeLetPropertyAccess(VarDecl *Property,
       // Replace the access to a let property by the value
       // computed by this initializer.
       SILValue clonedInit = cloneInitAt(proj);
-      SILBuilderWithScope B(proj);
       for (auto UI = proj->use_begin(), E = proj->use_end(); UI != E;) {
         auto *User = UI->getUser();
         ++UI;
 
-        if (isIncidentalUse(User))
-          continue;
-
         // A nested begin_access will be mapped as a separate "Load".
         if (isa<BeginAccessInst>(User))
           continue;
 
-        if (isa<StoreInst>(User))
+        if (!canReplaceLoadSequence(User))
           continue;
 
-        replaceLoadSequence(User, clonedInit, B);
+        replaceLoadSequence(User, clonedInit);
         eraseUsesOfInstruction(User);
         User->eraseFromParent();
         ++NumReplaced;

lib/SILOptimizer/Utils/Local.cpp

@@ -1624,6 +1624,47 @@ bool swift::analyzeStaticInitializer(
   return StaticInitializerAnalysis(forwardInstructions).analyze(V);
 }
 
+/// FIXME: This must be kept in sync with replaceLoadSequence()
+/// below. What a horrible design.
+bool swift::canReplaceLoadSequence(SILInstruction *I) {
+  if (auto *CAI = dyn_cast<CopyAddrInst>(I))
+    return true;
+
+  if (auto *LI = dyn_cast<LoadInst>(I))
+    return true;
+
+  if (auto *SEAI = dyn_cast<StructElementAddrInst>(I)) {
+    for (auto SEAIUse : SEAI->getUses()) {
+      if (!canReplaceLoadSequence(SEAIUse->getUser()))
+        return false;
+    }
+    return true;
+  }
+
+  if (auto *TEAI = dyn_cast<TupleElementAddrInst>(I)) {
+    for (auto TEAIUse : TEAI->getUses()) {
+      if (!canReplaceLoadSequence(TEAIUse->getUser()))
+        return false;
+    }
+    return true;
+  }
+
+  if (auto *BA = dyn_cast<BeginAccessInst>(I)) {
+    for (auto Use : BA->getUses()) {
+      if (!canReplaceLoadSequence(Use->getUser()))
+        return false;
+    }
+    return true;
+  }
+
+  // Incidental uses of an address are meaningless with regard to the loaded
+  // value.
+  if (isIncidentalUse(I) || isa<BeginUnpairedAccessInst>(I))
+    return true;
+
+  return false;
+}
+
 /// Replace load sequence which may contain
 /// a chain of struct_element_addr followed by a load.
 /// The sequence is traversed inside out, i.e.
@@ -1634,33 +1675,40 @@ bool swift::analyzeStaticInitializer(
 /// guarantee that the only uses of `I` are struct_element_addr and
 /// tuple_element_addr?
 void swift::replaceLoadSequence(SILInstruction *I,
-                                SILValue Value,
-                                SILBuilder &B) {
+                                SILValue Value) {
+  if (auto *CAI = dyn_cast<CopyAddrInst>(I)) {
+    SILBuilder B(CAI);
+    B.createStore(CAI->getLoc(), Value, CAI->getDest(),
+                  StoreOwnershipQualifier::Unqualified);
+    return;
+  }
+
   if (auto *LI = dyn_cast<LoadInst>(I)) {
     LI->replaceAllUsesWith(Value);
     return;
   }
 
-  // It is a series of struct_element_addr followed by load.
   if (auto *SEAI = dyn_cast<StructElementAddrInst>(I)) {
+    SILBuilder B(SEAI);
     auto *SEI = B.createStructExtract(SEAI->getLoc(), Value, SEAI->getField());
     for (auto SEAIUse : SEAI->getUses()) {
-      replaceLoadSequence(SEAIUse->getUser(), SEI, B);
+      replaceLoadSequence(SEAIUse->getUser(), SEI);
     }
     return;
   }
 
   if (auto *TEAI = dyn_cast<TupleElementAddrInst>(I)) {
+    SILBuilder B(TEAI);
     auto *TEI = B.createTupleExtract(TEAI->getLoc(), Value, TEAI->getFieldNo());
     for (auto TEAIUse : TEAI->getUses()) {
-      replaceLoadSequence(TEAIUse->getUser(), TEI, B);
+      replaceLoadSequence(TEAIUse->getUser(), TEI);
     }
     return;
   }
 
   if (auto *BA = dyn_cast<BeginAccessInst>(I)) {
     for (auto Use : BA->getUses()) {
-      replaceLoadSequence(Use->getUser(), Value, B);
+      replaceLoadSequence(Use->getUser(), Value);
     }
     return;
   }

test/SILOptimizer/globalopt_resilience.swift

@@ -15,5 +15,20 @@ public struct ResilientStruct {
   var rawValue: Int
 
   public static let staticVal = ResilientStruct(rawValue: 27)
+
+  @_optimize(none)
+  public func method() {}
+}
+
+public func cannotConvertToValueUse() {
+  // We can't optimize this because the method takes the resilient value as
+  // @in_guaranteed
+  ResilientStruct.staticVal.method()
 }
 
+// CHECK-LABEL: sil @$s4test23cannotConvertToValueUseyyF : $@convention(thin) () -> ()
+// CHECK: [[ADDR:%.*]] = global_addr @$s4test15ResilientStructV9staticValACvpZ : $*ResilientStruct
+// CHECK: [[METHOD:%.*]] = function_ref @$s4test15ResilientStructV6methodyyF : $@convention(method) (@in_guaranteed ResilientStruct) -> ()
+// CHECK: apply [[METHOD]]([[ADDR]]) : $@convention(method) (@in_guaranteed ResilientStruct) -> ()
+// CHECK: [[RESULT:%.*]] = tuple ()
+// CHECK: return [[RESULT]] : $()