Merge pull request #33713 from eeckstein/fix-lazy-property-cse

CSE: disable CSE of lazy property getters of struct

Author: eeckstein

lib/SILOptimizer/Transforms/CSE.cpp

@@ -831,6 +831,14 @@ static bool isLazyPropertyGetter(ApplyInst *ai) {
       !callee->isLazyPropertyGetter())
     return false;
 
+  // Only handle classes, but not structs.
+  // Lazy property getters of structs have an indirect inout self parameter.
+  // We don't know if the whole struct is overwritten between two getter calls.
+  // In such a case, the lazy property could be reset to an Optional.none.
+  // TODO: We could check this case with AliasAnalysis.
+  if (ai->getArgument(0)->getType().isAddress())
+    return false;
+
   // Check if the first block has a switch_enum of an Optional.
   // We don't handle getters of generic types, which have a switch_enum_addr.
   // This will be obsolete with opaque values anyway.

test/SILOptimizer/cse.sil

@@ -1306,3 +1306,53 @@ bb0(%0 : $@thick SpecialEnum.Type):
   %4 = struct $Bool (%3 : $Builtin.Int1)
   return %4 : $Bool
 }
+
+struct StructWithLazyProperty {
+  var lazyProperty: Int64 { mutating get set }
+  @_hasStorage @_hasInitialValue var lazyPropertyStorage : Int64? { get set }
+}
+
+sil private [lazy_getter] [noinline] @lazy_getter : $@convention(method) (@inout StructWithLazyProperty) -> Int64 {
+bb0(%0 : $*StructWithLazyProperty):
+  %2 = struct_element_addr %0 : $*StructWithLazyProperty, #StructWithLazyProperty.lazyPropertyStorage 
+  %3 = load %2 : $*Optional<Int64>
+  switch_enum %3 : $Optional<Int64>, case #Optional.some!enumelt: bb1, case #Optional.none!enumelt: bb2
+
+bb1(%5 : $Int64):
+  br bb3(%5 : $Int64)
+
+bb2:
+  %9 = integer_literal $Builtin.Int64, 27
+  %10 = struct $Int64 (%9 : $Builtin.Int64)
+  %12 = enum $Optional<Int64>, #Optional.some!enumelt, %10 : $Int64
+  store %12 to %2 : $*Optional<Int64>
+  br bb3(%10 : $Int64)
+
+bb3(%15 : $Int64):
+  return %15 : $Int64
+}
+
+sil @take_int : $@convention(thin) (Int64) -> ()
+
+// CHECK-LABEL: sil @dont_cse_lazy_property_of_overwritten_struct : $@convention(thin) () -> () {
+// CHECK:   [[GETTER:%[0-9]+]] = function_ref @lazy_getter
+// CHECK:   apply [[GETTER]]
+// CHECK:   apply [[GETTER]]
+// CHECK: } // end sil function 'dont_cse_lazy_property_of_overwritten_struct'
+sil @dont_cse_lazy_property_of_overwritten_struct : $@convention(thin) () -> () {
+bb0:
+  %0 = alloc_stack $StructWithLazyProperty
+  %4 = enum $Optional<Int64>, #Optional.none!enumelt
+  %5 = struct $StructWithLazyProperty (%4 : $Optional<Int64>)
+  store %5 to %0 : $*StructWithLazyProperty
+  %7 = function_ref @lazy_getter : $@convention(method) (@inout StructWithLazyProperty) -> Int64
+  %8 = apply %7(%0) : $@convention(method) (@inout StructWithLazyProperty) -> Int64
+  %9 = function_ref @take_int : $@convention(thin) (Int64) -> ()
+  %10 = apply %9(%8) : $@convention(thin) (Int64) -> ()
+  store %5 to %0 : $*StructWithLazyProperty
+  %18 = apply %7(%0) : $@convention(method) (@inout StructWithLazyProperty) -> Int64
+  %20 = apply %9(%18) : $@convention(thin) (Int64) -> ()
+  dealloc_stack %0 : $*StructWithLazyProperty
+  %22 = tuple ()
+  return %22 : $()
+}

test/SILOptimizer/lazy_property_getters.swift

@@ -88,20 +88,42 @@ func test_no_hoisting(_ c: Myclass, _ b: Bool) -> Int {
   return v
 }
 
+// This test is disabled, because for structs, it does not work yet.
+// CSE is too conservative to handle indirect getter arguments currently.
+
 // CHECK-LABEL: sil {{.*}} @$s4test0A7_structySiAA8MystructVF
+// CHECK-DISABLED:   [[GETTER:%[0-9]+]] = function_ref @$s4test8MystructV4lvarSivg
+// CHECK-DISABLED:   [[V1:%[0-9]+]] = apply [[GETTER]]({{.*}})
+// CHECK-DISABLED:   [[V2OPT:%[0-9]+]] = load
+// CHECK-DISABLED:   [[V2:%[0-9]+]] = unchecked_enum_data [[V2OPT]]
+// CHECK-DISABLED:   [[V1VAL:%[0-9]+]] = struct_extract [[V1]]
+// CHECK-DISABLED:   [[V2VAL:%[0-9]+]] = struct_extract [[V2]]
+// CHECK-DISABLED:   builtin "sadd{{.*}}"([[V1VAL]] {{.*}}, [[V2VAL]]
+// CHECK-DISABLED: } // end sil function '$s4test0A7_structySiAA8MystructVF'
+@inline(never)
+func test_struct(_ s: Mystruct) -> Int {
+  var sm = s
+  g = 42
+  let v1 = sm.lvar
+  let v2 = sm.lvar
+  return v1 &+ v2
+}
+
+// CHECK-LABEL: sil {{.*}} @$s4test0A19_overwritten_structySiAA8MystructVF
 // CHECK:   [[GETTER:%[0-9]+]] = function_ref @$s4test8MystructV4lvarSivg
 // CHECK:   [[V1:%[0-9]+]] = apply [[GETTER]]({{.*}})
-// CHECK:   [[V2OPT:%[0-9]+]] = load
-// CHECK:   [[V2:%[0-9]+]] = unchecked_enum_data [[V2OPT]]
+// CHECK:   [[V2:%[0-9]+]] = apply [[GETTER]]({{.*}})
 // CHECK:   [[V1VAL:%[0-9]+]] = struct_extract [[V1]]
 // CHECK:   [[V2VAL:%[0-9]+]] = struct_extract [[V2]]
 // CHECK:   builtin "sadd{{.*}}"([[V1VAL]] {{.*}}, [[V2VAL]]
-// CHECK: } // end sil function '$s4test0A7_structySiAA8MystructVF'
+// CHECK: } // end sil function '$s4test0A19_overwritten_structySiAA8MystructVF'
 @inline(never)
-func test_struct(_ s: Mystruct) -> Int {
+func test_overwritten_struct(_ s: Mystruct) -> Int {
   var sm = s
   g = 42
   let v1 = sm.lvar
+  sm = s
+  g = 43
   let v2 = sm.lvar
   return v1 &+ v2
 }
@@ -133,6 +155,13 @@ func calltests() {
   // CHECK-OUTPUT-NEXT:  lvar init
   // CHECK-OUTPUT-NEXT:  84
   print(test_struct(Mystruct()))
+
+  // CHECK-OUTPUT-LABEL: test_overwritten_struct
+  print("test_overwritten_struct")
+  // CHECK-OUTPUT-NEXT:  lvar init
+  // CHECK-OUTPUT-NEXT:  lvar init
+  // CHECK-OUTPUT-NEXT:  85
+  print(test_overwritten_struct(Mystruct()))
 }
 
 calltests()