Merge pull request #79483 from eeckstein/fix-temp-lvalue-opt

TempLValueOpt: avoid creating invalid apply argument aliasing.

Author: eeckstein

lib/SILOptimizer/Transforms/TempLValueOpt.cpp

@@ -83,6 +83,8 @@ class TempLValueOptPass : public SILFunctionTransform {
 private:
   void tempLValueOpt(CopyAddrInst *copyInst);
   void combineCopyAndDestroy(CopyAddrInst *copyInst);
+  bool hasInvalidApplyArgumentAliasing(SILInstruction *inst, SILValue tempAddr,
+                                       SILValue destAddr, AliasAnalysis *aa);
 };
 
 void TempLValueOptPass::run() {
@@ -230,6 +232,11 @@ void TempLValueOptPass::tempLValueOpt(CopyAddrInst *copyInst) {
           projections.contains(inst) == 0)
         return;
 
+      // Check if replacing the temporary with destination would invalidate an applied
+      // function's alias rules of indirect arguments.
+      if (hasInvalidApplyArgumentAliasing(inst, temporary, destination, AA))
+        return;
+      
       if (needDestroyEarly && isDeinitBarrier(inst, bca))
         return;
     }
@@ -269,6 +276,45 @@ void TempLValueOptPass::tempLValueOpt(CopyAddrInst *copyInst) {
   invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
 }
 
+/// Returns true if after replacing tempAddr with destAddr an apply instruction
+/// would have invalid aliasing of indirect arguments.
+/// 
+/// An indirect argument (except `@inout_aliasable`) must not alias with another
+/// indirect argument. Now, if we would replace tempAddr with destAddr in
+/// 
+///   apply %f(%tempAddr, %destAddr) : (@in T) -> @out T
+///   
+/// we would invalidate this rule.
+/// This is even true if the called function does not read from destAddr.
+///
+bool TempLValueOptPass::hasInvalidApplyArgumentAliasing(SILInstruction *inst,
+                                                        SILValue tempAddr,
+                                                        SILValue destAddr,
+                                                        AliasAnalysis *aa) {
+  auto as = FullApplySite::isa(inst);
+  if (!as)
+    return false;
+  
+  bool tempAccessed = false;
+  bool destAccessed = false;
+  bool mutatingAccess = false;
+  for (Operand &argOp : as.getArgumentOperands()) {
+    auto conv = as.getArgumentConvention(argOp);
+    if (conv.isExclusiveIndirectParameter()) {
+      if (aa->mayAlias(tempAddr, argOp.get())) {
+        tempAccessed = true;
+        if (!conv.isGuaranteedConventionInCaller())
+          mutatingAccess = true;
+      } else if (aa->mayAlias(destAddr, argOp.get())) {
+        destAccessed = true;
+        if (!conv.isGuaranteedConventionInCaller())
+          mutatingAccess = true;
+      }
+    }
+  }
+  return mutatingAccess && tempAccessed && destAccessed;
+}
+
 void TempLValueOptPass::combineCopyAndDestroy(CopyAddrInst *copyInst) {
   if (copyInst->isTakeOfSrc())
     return;

test/SILOptimizer/templvalueopt_crash.swift

@@ -0,0 +1,24 @@
+// RUN: %target-swift-frontend %s -O -sil-verify-all -emit-sil -o /dev/null
+
+
+// Check that TempLValueOpt does not create invalid SIL which causes a verification error.
+
+public struct S<T> {
+  internal var a: T? = nil
+  public var b: T? {a}
+  public var c: T? = nil
+  
+  public mutating func set() {
+    c = b
+  }
+}
+
+public class Klass{}
+
+public func test() -> S<Klass> {
+  var s = S<Klass>()
+  s.set()
+  return s
+}
+
+

test/SILOptimizer/templvalueopt_ossa.sil

@@ -334,3 +334,72 @@ bb0(%0 : @guaranteed $Any, %1 : $*Any):
   %11 = tuple ()
   return %11 : $()
 }
+
+struct TwoFields {
+  var a: Child
+  var b: Child
+}
+
+sil @no_read : $@convention(thin) (@in_guaranteed TwoFields) -> @out Child {
+[%0: write v**]
+[global: ]
+}
+sil @no_reads : $@convention(thin) (@in_guaranteed Child, @in_guaranteed TwoFields) -> () {
+[global: ]
+}
+
+// CHECK-LABEL: sil [ossa] @argument_aliasing :
+// CHECK:         %1 = alloc_stack
+// CHECK:         apply %{{[0-9]+}}(%1, %0)
+// CHECK-LABEL: } // end sil function 'argument_aliasing'
+sil [ossa] @argument_aliasing : $@convention(thin) (@inout TwoFields) -> () {
+bb0(%0 : $*TwoFields):
+  %1 = alloc_stack $Child
+  %2 = function_ref @no_read : $@convention(thin) (@in_guaranteed TwoFields) -> @out Child
+  %3 = apply %2(%1, %0) : $@convention(thin) (@in_guaranteed TwoFields) -> @out Child
+  %4 = struct_element_addr %0, #TwoFields.a
+  copy_addr [take] %1 to %4
+  dealloc_stack %1
+  %7 = tuple ()
+  return %7
+}
+
+// CHECK-LABEL: sil [ossa] @no_argument_aliasing :
+// CHECK-NOT:     alloc_stack
+// CHECK:         [[A:%.*]] = struct_element_addr %0 : $*TwoFields, #TwoFields.a
+// CHECK-NEXT:    destroy_addr [[A]]
+// CHECK:         apply %{{[0-9]+}}([[A]], %1)
+// CHECK-NOT:     copy_addr
+// CHECK-LABEL: } // end sil function 'no_argument_aliasing'
+sil [ossa] @no_argument_aliasing : $@convention(thin) (@inout TwoFields, @inout TwoFields) -> () {
+bb0(%0 : $*TwoFields, %1 : $*TwoFields):
+  %2 = alloc_stack $Child
+  %3 = function_ref @no_read : $@convention(thin) (@in_guaranteed TwoFields) -> @out Child
+  %4 = apply %3(%2, %1) : $@convention(thin) (@in_guaranteed TwoFields) -> @out Child
+  %5 = struct_element_addr %0, #TwoFields.a
+  copy_addr [take] %2 to %5
+  dealloc_stack %2
+  %8 = tuple ()
+  return %8
+}
+
+// CHECK-LABEL: sil [ossa] @guaranteed_argument_aliasing :
+// CHECK-NOT:     alloc_stack
+// CHECK:         [[A:%.*]] = struct_element_addr %0 : $*TwoFields, #TwoFields.a
+// CHECK-NEXT:    destroy_addr [[A]]
+// CHECK:         apply %{{[0-9]+}}([[A]], %0)
+// CHECK-NOT:     copy_addr
+// CHECK-LABEL: } // end sil function 'guaranteed_argument_aliasing'
+sil [ossa] @guaranteed_argument_aliasing : $@convention(thin) (@inout TwoFields, @owned Child) -> () {
+bb0(%0 : $*TwoFields, %1 : @owned $Child):
+  %2 = alloc_stack $Child
+  store %1 to [init] %2
+  %3 = function_ref @no_reads : $@convention(thin) (@in_guaranteed Child, @in_guaranteed TwoFields) -> ()
+  %4 = apply %3(%2, %0) : $@convention(thin) (@in_guaranteed Child, @in_guaranteed TwoFields) -> ()
+  %5 = struct_element_addr %0, #TwoFields.a
+  copy_addr [take] %2 to %5
+  dealloc_stack %2
+  %8 = tuple ()
+  return %8
+}
+