TempRValueElimination: handle potential modifications of the copy-sou…

…rce in a called functions correctly. This fixes a miscompile in case the source of the optimized copy_addr is modified in a called function with to a not visible alias. This can happen with class properties or global variables. This fix removes the special handling of function parameters, which was just wrong. Instead it simply uses the alias analysis API to check for modifications of the source object. The fix makes TempRValueElimination more conservative and this can cause some performance regressions, but this is unavoidable. rdar://problem/69605657
hborla · Oct 9, 2020 · 9f85cb8 · 9f85cb8
1 parent d4a6bd3
commit 9f85cb8
Show file tree

Hide file tree

Showing 6 changed files with 96 additions and 58 deletions.
diff --git a/lib/SILOptimizer/Transforms/TempRValueElimination.cpp b/lib/SILOptimizer/Transforms/TempRValueElimination.cpp
@@ -79,9 +79,8 @@ class TempRValueOptPass : public SILFunctionTransform {
   checkTempObjectDestroy(AllocStackInst *tempObj, CopyAddrInst *copyInst,
                          ValueLifetimeAnalysis::Frontier &tempAddressFrontier);
 
-  bool checkNoTempObjectModificationInApply(Operand *tempObjUser,
-                                            SILInstruction *inst,
-                                            SILValue srcAddr);
+  bool canApplyBeTreatedAsLoad(Operand *tempObjUser, ApplySite apply,
+                               SILValue srcAddr);
 
   bool tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst);
   std::pair<SILBasicBlock::iterator, bool>
@@ -115,57 +114,39 @@ bool TempRValueOptPass::collectLoadsFromProjection(
   return true;
 }
 
-/// Check if 'tempObjUser' passed to the apply instruction can be modified by it
-bool TempRValueOptPass::checkNoTempObjectModificationInApply(
-    Operand *tempObjUser, SILInstruction *applyInst, SILValue srcAddr) {
-  ApplySite apply(applyInst);
-
+/// Check if \p tempObjUser, passed to the apply instruction, is only loaded,
+/// but not modified and if \p srcAddr is not modified as well.
+bool TempRValueOptPass::canApplyBeTreatedAsLoad(
+    Operand *tempObjUser, ApplySite apply, SILValue srcAddr) {
   // Check if the function can just read from tempObjUser.
   auto convention = apply.getArgumentConvention(*tempObjUser);
   if (!convention.isGuaranteedConvention()) {
     LLVM_DEBUG(llvm::dbgs() << "  Temp consuming use may write/destroy "
                                "its source"
-                            << *applyInst);
-    return false;
-  }
-
-  // If we do not have an src address, but are indirect, bail. We would need
-  // to perform function signature specialization to change the functions
-  // signature to pass something direct.
-  if (!srcAddr && convention.isIndirectConvention()) {
-    LLVM_DEBUG(
-        llvm::dbgs()
-        << "  Temp used to materialize value for indirect convention?! Can "
-           "not remove temporary without func sig opts"
-        << *applyInst);
+                            << *apply.getInstruction());
     return false;
   }
 
-  // Check if there is another function argument, which is inout which might
-  // modify the source address if we have one.
-  //
-  // When a use of the temporary is an apply, then we need to prove that the
-  // function called by the apply cannot modify the temporary's source
-  // value. By design, this should be handled by
-  // `checkNoSourceModification`. However, this would be too conservative
-  // since it's common for the apply to have an @out argument, and alias
-  // analysis cannot prove that the @out does not alias with `src`. Instead,
-  // `checkNoSourceModification` always avoids analyzing the current use, so
-  // applies need to be handled here. We already know that an @out cannot
-  // alias with `src` because the `src` value must be initialized at the point
-  // of the call. Hence, it is sufficient to check specifically for another
-  // @inout that might alias with `src`.
   if (srcAddr) {
-    auto calleeConv = apply.getSubstCalleeConv();
-    unsigned calleeArgIdx = apply.getCalleeArgIndexOfFirstAppliedArg();
-    for (const auto &operand : apply.getArgumentOperands()) {
-      auto argConv = calleeConv.getSILArgumentConvention(calleeArgIdx);
-      if (argConv.isInoutConvention()) {
-        if (!aa->isNoAlias(operand.get(), srcAddr)) {
-          return false;
-        }
-      }
-      ++calleeArgIdx;
+    // If the function may write to the source of the copy_addr, the apply
+    // cannot be treated as a load: all (potential) writes of the source must
+    // appear _after_ all loads of the temporary. But in case of a function call
+    // we don't know in which order the writes and loads are executed inside the
+    // called function. The source may be written before the temporary is
+    // loaded, which would make the optization invalid.
+    if (aa->mayWriteToMemory(apply.getInstruction(), srcAddr))
+      return false;
+  } else {
+    // If we do not have an src address, but are indirect, bail. We would need
+    // to perform function signature specialization to change the functions
+    // signature to pass something direct.
+    if (convention.isIndirectConvention()) {
+      LLVM_DEBUG(
+          llvm::dbgs()
+          << "  Temp used to materialize value for indirect convention?! Can "
+             "not remove temporary without func sig opts"
+          << *apply.getInstruction());
+      return false;
     }
   }
   return true;
@@ -189,7 +170,8 @@ bool TempRValueOptPass::collectLoads(
     SILValue srcAddr, SmallPtrSetImpl<SILInstruction *> &loadInsts) {
   // All normal uses (loads) must be in the initialization block.
   // (The destroy and dealloc are commonly in a different block though.)
-  if (user->getParent() != address->getParent())
+  SILBasicBlock *block = address->getParent();
+  if (user->getParent() != block)
     return false;
 
   // Only allow uses that cannot destroy their operand. We need to be sure
@@ -232,22 +214,25 @@ bool TempRValueOptPass::collectLoads(
      LLVM_FALLTHROUGH;
   case SILInstructionKind::ApplyInst:
   case SILInstructionKind::TryApplyInst: {
-    if (!checkNoTempObjectModificationInApply(userOp, user, srcAddr))
+    if (!canApplyBeTreatedAsLoad(userOp, ApplySite(user), srcAddr))
       return false;
     // Everything is okay with the function call. Register it as a "load".
     loadInsts.insert(user);
     return true;
   }
   case SILInstructionKind::BeginApplyInst: {
-    if (!checkNoTempObjectModificationInApply(userOp, user, srcAddr))
+    if (!canApplyBeTreatedAsLoad(userOp, ApplySite(user), srcAddr))
       return false;
 
     auto beginApply = cast<BeginApplyInst>(user);
     // Register 'end_apply'/'abort_apply' as loads as well
     // 'checkNoSourceModification' should check instructions until
     // 'end_apply'/'abort_apply'.
-    for (auto tokenUses : beginApply->getTokenResult()->getUses()) {
-      loadInsts.insert(tokenUses->getUser());
+    for (auto tokenUse : beginApply->getTokenResult()->getUses()) {
+      SILInstruction *user = tokenUse->getUser();
+      if (user->getParent() != block)
+        return false;
+      loadInsts.insert(tokenUse->getUser());
     }
     return true;
   }
@@ -285,7 +270,8 @@ bool TempRValueOptPass::collectLoads(
     return collectLoadsFromProjection(utedai, srcAddr, loadInsts);
   }
   case SILInstructionKind::StructElementAddrInst:
-  case SILInstructionKind::TupleElementAddrInst: {
+  case SILInstructionKind::TupleElementAddrInst:
+  case SILInstructionKind::UncheckedAddrCastInst: {
     return collectLoadsFromProjection(cast<SingleValueInstruction>(user),
                                       srcAddr, loadInsts);
   }

diff --git a/test/SILOptimizer/bridged_casts_folding.swift b/test/SILOptimizer/bridged_casts_folding.swift
@@ -904,8 +904,12 @@ var anyHashable: AnyHashable = 0
 
 // CHECK-LABEL: $s21bridged_casts_folding29testUncondCastSwiftToSubclassAA08NSObjectI0CyF
 // CHECK: [[GLOBAL:%[0-9]+]] = global_addr @$s21bridged_casts_folding11anyHashables03AnyE0Vv
+// CHECK: [[TMP:%[0-9]+]] = alloc_stack $AnyHashable
+// CHECK: [[ACCESS:%[0-9]+]] = begin_access [read] [static] [no_nested_conflict] [[GLOBAL]]
+// CHECK: copy_addr [[ACCESS]] to [initialization] [[TMP]]
 // CHECK: [[FUNC:%.*]] = function_ref @$ss11AnyHashableV10FoundationE19_bridgeToObjectiveCSo8NSObjectCyF
-// CHECK-NEXT: apply [[FUNC]]([[GLOBAL]])
+// CHECK-NEXT: apply [[FUNC]]([[TMP]])
+// CHECK-NEXT: destroy_addr [[TMP]]
 // CHECK-NEXT: unconditional_checked_cast {{%.*}} : $NSObject to NSObjectSubclass
 // CHECK: } // end sil function '$s21bridged_casts_folding29testUncondCastSwiftToSubclassAA08NSObjectI0CyF'
 @inline(never)

diff --git a/test/SILOptimizer/opened_archetype_operands_tracking.sil b/test/SILOptimizer/opened_archetype_operands_tracking.sil
@@ -210,12 +210,10 @@ sil hidden_external @use : $@convention(thin) <τ_0_0> (@in τ_0_0) -> ()
 // It should contain alloc_ref and alloc_stack instructions using opened archetypes.
 // CHECK-LABEL: sil @bar
 // CHECK: open_existential{{.*}}C08045E0-2779-11E7-970E-A45E60E99281
-// CHECK: alloc_stack{{.*}}C08045E0-2779-11E7-970E-A45E60E99281
 // CHECK: alloc_ref{{.*}}C08045E0-2779-11E7-970E-A45E60E99281
 // CHECK-NOT: function_ref @use
 // CHECK: function_ref @use 
 // CHECK-NOT: function_ref
-// CHECK: dealloc_stack{{.*}}C08045E0-2779-11E7-970E-A45E60E99281
 // CHECK: strong_release{{.*}}C08045E0-2779-11E7-970E-A45E60E99281
 // CHECK: dealloc_ref{{.*}}C08045E0-2779-11E7-970E-A45E60E99281
 // CHECK: end sil function 'bar' 

diff --git a/test/SILOptimizer/sil_combine_protocol_conf.swift b/test/SILOptimizer/sil_combine_protocol_conf.swift
@@ -245,8 +245,10 @@ internal class OtherClass {
 // CHECK: load [[S11]] 
 // CHECK: [[R2:%.*]] = ref_element_addr [[ARG]] : $OtherClass, #OtherClass.arg2
 // CHECK: [[O2:%.*]] = open_existential_addr immutable_access [[R2]] : $*GenericPropProtocol to $*@opened("{{.*}}") GenericPropProtocol
+// CHECK: [[T1:%.*]] = alloc_stack $@opened
+// CHECK: copy_addr [[O2]] to [initialization] [[T1]]
 // CHECK: [[W2:%.*]] = witness_method $@opened("{{.*}}") GenericPropProtocol, #GenericPropProtocol.val!getter : <Self where Self : GenericPropProtocol> (Self) -> () -> Int, [[O2]] : $*@opened("{{.*}}") GenericPropProtocol : $@convention(witness_method: GenericPropProtocol) <τ_0_0 where τ_0_0 : GenericPropProtocol> (@in_guaranteed τ_0_0) -> Int
-// CHECK: apply [[W2]]<@opened("{{.*}}") GenericPropProtocol>([[O2]]) : $@convention(witness_method: GenericPropProtocol) <τ_0_0 where τ_0_0 : GenericPropProtocol> (@in_guaranteed τ_0_0) -> Int
+// CHECK: apply [[W2]]<@opened("{{.*}}") GenericPropProtocol>([[T1]]) : $@convention(witness_method: GenericPropProtocol) <τ_0_0 where τ_0_0 : GenericPropProtocol> (@in_guaranteed τ_0_0) -> Int
 // CHECK: struct_extract
 // CHECK: integer_literal
 // CHECK: builtin
@@ -265,8 +267,10 @@ internal class OtherClass {
 // CHECK: cond_fail
 // CHECK: [[R5:%.*]] = ref_element_addr [[ARG]] : $OtherClass, #OtherClass.arg4
 // CHECK: [[O5:%.*]] = open_existential_addr immutable_access [[R5]] : $*GenericNestedPropProtocol to $*@opened("{{.*}}") GenericNestedPropProtocol
+// CHECK: [[T2:%.*]] = alloc_stack $@opened
+// CHECK: copy_addr [[O5]] to [initialization] [[T2]]
 // CHECK: [[W5:%.*]] = witness_method $@opened("{{.*}}") GenericNestedPropProtocol, #GenericNestedPropProtocol.val!getter : <Self where Self : GenericNestedPropProtocol> (Self) -> () -> Int, [[O5:%.*]] : $*@opened("{{.*}}") GenericNestedPropProtocol : $@convention(witness_method: GenericNestedPropProtocol) <τ_0_0 where τ_0_0 : GenericNestedPropProtocol> (@in_guaranteed τ_0_0) -> Int 
-// CHECK: apply [[W5]]<@opened("{{.*}}") GenericNestedPropProtocol>([[O5]]) : $@convention(witness_method: GenericNestedPropProtocol) <τ_0_0 where τ_0_0 : GenericNestedPropProtocol> (@in_guaranteed τ_0_0) -> Int
+// CHECK: apply [[W5]]<@opened("{{.*}}") GenericNestedPropProtocol>([[T2]]) : $@convention(witness_method: GenericNestedPropProtocol) <τ_0_0 where τ_0_0 : GenericNestedPropProtocol> (@in_guaranteed τ_0_0) -> Int
 // CHECK: struct_extract
 // CHECK: builtin
 // CHECK: tuple_extract
@@ -333,8 +337,10 @@ internal class OtherKlass {
 // CHECK: integer_literal
 // CHECK: [[R1:%.*]] = ref_element_addr [[ARG]] : $OtherKlass, #OtherKlass.arg2
 // CHECK: [[O1:%.*]] = open_existential_addr immutable_access [[R1]] : $*AGenericProtocol to $*@opened("{{.*}}") AGenericProtocol
+// CHECK: [[T1:%.*]] = alloc_stack $@opened
+// CHECK: copy_addr [[O1]] to [initialization] [[T1]]
 // CHECK: [[W1:%.*]] = witness_method $@opened("{{.*}}") AGenericProtocol, #AGenericProtocol.val!getter : <Self where Self : AGenericProtocol> (Self) -> () -> Int, [[O1]] : $*@opened("{{.*}}") AGenericProtocol : $@convention(witness_method: AGenericProtocol) <τ_0_0 where τ_0_0 : AGenericProtocol> (@in_guaranteed τ_0_0) -> Int
-// CHECK: apply [[W1]]<@opened("{{.*}}") AGenericProtocol>([[O1]]) : $@convention(witness_method: AGenericProtocol) <τ_0_0 where τ_0_0 : AGenericProtocol> (@in_guaranteed τ_0_0) -> Int
+// CHECK: apply [[W1]]<@opened("{{.*}}") AGenericProtocol>([[T1]]) : $@convention(witness_method: AGenericProtocol) <τ_0_0 where τ_0_0 : AGenericProtocol> (@in_guaranteed τ_0_0) -> Int
 // CHECK: struct_extract
 // CHECK: integer_literal
 // CHECK: builtin

diff --git a/test/SILOptimizer/temp_rvalue_opt.sil b/test/SILOptimizer/temp_rvalue_opt.sil
@@ -14,14 +14,17 @@ struct GS<Base> {
   var _value: Builtin.Int64
 }
 
-class Klass {}
+class Klass {
+  @_hasStorage var a: String
+}
 
 struct Str {
   var kl: Klass
   var _value: Builtin.Int64
 }
 
 sil @unknown : $@convention(thin) () -> ()
+sil @load_string : $@convention(thin) (@in_guaranteed String) -> String
 
 sil @inguaranteed_user_without_result : $@convention(thin) (@in_guaranteed Klass) -> () {
 bb0(%0 : $*Klass):
@@ -333,6 +336,23 @@ bb0(%0 : $*GS<B>, %1 : $*GS<B>, %2 : $Builtin.Int64):
   return %9999 : $()
 }
 
+// CHECK-LABEL: sil [ossa] @consider_implicit_aliases_in_callee
+// CHECK:         alloc_stack
+// CHECK-NEXT:    copy_addr
+// CHECK:       } // end sil function 'consider_implicit_aliases_in_callee'
+
+sil [ossa] @consider_implicit_aliases_in_callee : $@convention(thin) (@guaranteed Klass) -> String {
+bb0(%0 : @guaranteed $Klass):
+  %1 = ref_element_addr %0 : $Klass, #Klass.a
+  %2 = alloc_stack $String
+  copy_addr %1 to [initialization] %2 : $*String
+  %f = function_ref @load_string : $@convention(thin) (@in_guaranteed String) -> String
+  %a = apply %f(%2) : $@convention(thin) (@in_guaranteed String) -> String
+  destroy_addr %2 : $*String
+  dealloc_stack %2 : $*String
+  return %a : $String
+}
+
 // Test temp RValue elimination on switches.
 // CHECK-LABEL: sil @rvalueSwitch
 // CHECK:      bb1:

diff --git a/test/SILOptimizer/temp_rvalue_opt.swift b/test/SILOptimizer/temp_rvalue_opt.swift
@@ -0,0 +1,24 @@
+// RUN: %empty-directory(%t) 
+// RUN: %target-build-swift -O %s -o %t/a.out
+// RUN: %target-codesign %t/a.out
+// RUN: %target-run %t/a.out | %FileCheck %s
+
+// REQUIRES: executable_test
+
+var global: Any = 1
+func withValue(action: (Any) -> Void) {
+  action(global)
+}
+
+@inline(never)
+public func test_global_side_effect() {
+  withValue { value in
+      print(value)
+      global = 24
+      print(value)
+  }
+}
+
+// CHECK:      1
+// CHECK-NEXT: 1
+test_global_side_effect()