Merge pull request #24625 from atrick/canonicalize-stores

Author: swift-ci

lib/SILOptimizer/SILCombiner/SILCombiner.h

@@ -235,7 +235,6 @@ class SILCombiner :
   SILInstruction *visitUpcastInst(UpcastInst *UCI);
   SILInstruction *optimizeLoadFromStringLiteral(LoadInst *LI);
   SILInstruction *visitLoadInst(LoadInst *LI);
-  SILInstruction *visitStoreInst(StoreInst *si);
   SILInstruction *visitIndexAddrInst(IndexAddrInst *IA);
   SILInstruction *visitAllocStackInst(AllocStackInst *AS);
   SILInstruction *visitAllocRefInst(AllocRefInst *AR);

lib/SILOptimizer/SILCombiner/SILCombinerMiscVisitors.cpp

@@ -586,79 +586,6 @@ SILInstruction *SILCombiner::optimizeLoadFromStringLiteral(LoadInst *LI) {
   return Builder.createIntegerLiteral(LI->getLoc(), LI->getType(), str[index]);
 }
 
-SILInstruction *SILCombiner::visitStoreInst(StoreInst *si) {
-  auto *f = si->getFunction();
-  assert(f->getConventions().useLoweredAddresses() &&
-         "These optimizations assume that opaque values are not enabled");
-
-  // (store (struct_element_addr addr) object)
-  //   ->
-  // (store addr (struct object))
-
-  // If our store's destination is not a struct_element_addr, bail early.
-  auto *sea = dyn_cast<StructElementAddrInst>(si->getDest());
-  if (!sea)
-    return nullptr;
-
-  // Ok, we have at least one struct_element_addr. Canonicalize the underlying
-  // store.
-  Builder.setInsertionPoint(si);
-  SILLocation loc = si->getLoc();
-
-  auto &mod = si->getModule();
-  SILValue result = si->getSrc();
-  SILValue iterAddr = sea->getOperand();
-  SILValue storeAddr;
-  while (true) {
-    SILType iterAddrType = iterAddr->getType();
-
-    // If our aggregate has unreferenced storage then we can never prove if it
-    // actually has a single field.
-    if (iterAddrType.aggregateHasUnreferenceableStorage())
-      break;
-
-    auto *decl = iterAddrType.getStructOrBoundGenericStruct();
-    assert(
-        !decl->isResilient(mod.getSwiftModule(), f->getResilienceExpansion()) &&
-        "This code assumes resilient structs can not have fragile fields. If "
-        "this assert is hit, this has been changed. Please update this code.");
-
-    // NOTE: If this is ever changed to support enums, we must check for address
-    // only types here. For structs we do not have to check since a single
-    // element struct with a loadable element can never be address only. We
-    // additionally do not have to worry about our input value being address
-    // only since we are storing into it.
-    auto props = decl->getStoredProperties();
-    if (std::next(props.begin()) != props.end())
-      break;
-
-    // Update the store location now that we know it is safe.
-    storeAddr = iterAddr;
-
-    // Otherwise, create the struct.
-    result = Builder.createStruct(loc, iterAddrType.getObjectType(), result);
-
-    // See if we have another struct_element_addr we can strip off. If we don't
-    // then this as much as we can promote.
-    sea = dyn_cast<StructElementAddrInst>(sea->getOperand());
-    if (!sea)
-      break;
-    iterAddr = sea->getOperand();
-  }
-
-  // If we failed to create any structs, bail.
-  if (result == si->getSrc())
-    return nullptr;
-
-  // Then create a new store, storing the value into the relevant computed
-  // address.
-  Builder.createStore(loc, result, storeAddr,
-                      StoreOwnershipQualifier::Unqualified);
-
-  // Then eliminate the original store.
-  return eraseInstFromFunction(*si);
-}
-
 SILInstruction *SILCombiner::visitLoadInst(LoadInst *LI) {
   // (load (upcast-ptr %x)) -> (upcast-ref (load %x))
   Builder.setCurrentDebugScope(LI->getDebugScope());

lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp

@@ -309,6 +309,84 @@ splitAggregateLoad(LoadInst *loadInst, CanonicalizeInstruction &pass) {
   return killInstAndIncidentalUses(loadInst, nextII, pass);
 }
 
+// Given a store within a single property struct, recursively form the parent
+// struct values and promote the store to the outer struct type.
+//
+// (store (struct_element_addr %base) object)
+//   ->
+// (store %base (struct object))
+static SILBasicBlock::iterator
+broadenSingleElementStores(StoreInst *storeInst,
+                           CanonicalizeInstruction &pass) {
+  // Keep track of the next iterator after any newly added or to-be-deleted
+  // instructions. This must be valid regardless of whether the pass immediately
+  // deletes the instructions or simply records them for later deletion.
+  auto nextII = std::next(storeInst->getIterator());
+
+  // Bail if the store's destination is not a struct_element_addr.
+  auto *sea = dyn_cast<StructElementAddrInst>(storeInst->getDest());
+  if (!sea)
+    return nextII;
+
+  auto *f = storeInst->getFunction();
+
+  // Continue up the struct_element_addr chain, as long as each struct only has
+  // a single property, creating StoreInsts along the way.
+  SILBuilderWithScope builder(storeInst);
+
+  SILValue result = storeInst->getSrc();
+  SILValue baseAddr = sea->getOperand();
+  SILValue storeAddr;
+  while (true) {
+    SILType baseAddrType = baseAddr->getType();
+
+    // If our aggregate has unreferenced storage then we can never prove if it
+    // actually has a single field.
+    if (baseAddrType.aggregateHasUnreferenceableStorage())
+      break;
+
+    auto *decl = baseAddrType.getStructOrBoundGenericStruct();
+    assert(
+      !decl->isResilient(f->getModule().getSwiftModule(),
+                         f->getResilienceExpansion()) &&
+        "This code assumes resilient structs can not have fragile fields. If "
+        "this assert is hit, this has been changed. Please update this code.");
+
+    // NOTE: If this is ever changed to support enums, we must check for address
+    // only types here. For structs we do not have to check since a single
+    // element struct with a loadable element can never be address only. We
+    // additionally do not have to worry about our input value being address
+    // only since we are storing into it.
+    auto props = decl->getStoredProperties();
+    if (std::next(props.begin()) != props.end())
+      break;
+
+    // Update the store location now that we know it is safe.
+    storeAddr = baseAddr;
+
+    // Otherwise, create the struct.
+    result = builder.createStruct(storeInst->getLoc(),
+                                  baseAddrType.getObjectType(), result);
+
+    // See if we have another struct_element_addr we can strip off. If we don't
+    // then this as much as we can promote.
+    sea = dyn_cast<StructElementAddrInst>(sea->getOperand());
+    if (!sea)
+      break;
+    baseAddr = sea->getOperand();
+  }
+  // If we failed to create any structs, bail.
+  if (result == storeInst->getSrc())
+    return nextII;
+
+  // Store the new struct-wrapped value into the final base address.
+  builder.createStore(storeInst->getLoc(), result, storeAddr,
+                      storeInst->getOwnershipQualifier());
+
+  // Erase the original store.
+  return killInstruction(storeInst, nextII, pass);
+}
+
 //===----------------------------------------------------------------------===//
 //                            Top-Level Entry Point
 //===----------------------------------------------------------------------===//
@@ -321,6 +399,9 @@ CanonicalizeInstruction::canonicalize(SILInstruction *inst) {
   if (auto *loadInst = dyn_cast<LoadInst>(inst))
     return splitAggregateLoad(loadInst, *this);
 
+  if (auto *storeInst = dyn_cast<StoreInst>(inst))
+    return broadenSingleElementStores(storeInst, *this);
+
   // Skip ahead.
   return std::next(inst->getIterator());
 }

test/SILOptimizer/silgen_cleanup.sil

@@ -11,10 +11,16 @@ import SwiftShims
 // CHECK: bb0([[IN:%[0-9]+]] : $*UInt8):
 // CHECK-NEXT:  [[IN_GEP:%[0-9]+]] = struct_element_addr [[IN]] : $*UInt8, #UInt8._value
 // CHECK-NEXT:  [[IN_LOADED:%[0-9]+]] = load [trivial] [[IN_GEP]] : $*Builtin.Int8
+// CHECK-NEXT:  [[LITERAL:%[0-9]+]] = integer_literal $Builtin.Int8, 1
+// CHECK-NEXT:  [[UINT8:%.*]] = struct $UInt8 ([[LITERAL]] : $Builtin.Int8)
+// CHECK-NEXT:  store [[UINT8]] to [trivial] [[IN]] : $*UInt8
 // CHECK-NEXT:  return [[IN_LOADED]] : $Builtin.Int8
 sil [ossa] @struct_extract_load_to_load_struct_element_addr : $@convention(thin) (@inout UInt8) -> (Builtin.Int8) {
 bb0(%0 : $*UInt8):
   %1 = load [trivial] %0 : $*UInt8
+  %2 = integer_literal $Builtin.Int8, 1
+  %3 = struct_element_addr %0 : $*UInt8, #UInt8._value
+  store %2 to [trivial] %3 : $*Builtin.Int8
   %5 = struct_extract %1 : $UInt8, #UInt8._value
   return %5 : $Builtin.Int8
 }
@@ -23,10 +29,16 @@ bb0(%0 : $*UInt8):
 // CHECK: bb0([[IN:%[0-9]+]] : $*(Builtin.Int8, Builtin.Int8)):
 // CHECK-NEXT:  [[IN_GEP:%[0-9]+]] = tuple_element_addr [[IN]] : $*(Builtin.Int8, Builtin.Int8), 0
 // CHECK-NEXT:  [[IN_LOADED:%[0-9]+]] = load [trivial] [[IN_GEP]] : $*Builtin.Int8
+// CHECK-NEXT:  [[LITERAL:%[0-9]+]] = integer_literal $Builtin.Int8, 1
+// CHECK-NEXT:  [[IN_STORE_GEP:%[0-9]+]] = tuple_element_addr %0 : $*(Builtin.Int8, Builtin.Int8), 0
+// CHECK-NEXT:  store [[LITERAL]] to [trivial] [[IN_STORE_GEP]] : $*Builtin.Int8
 // CHECK-NEXT:  return [[IN_LOADED]] : $Builtin.Int8
 sil [ossa] @tuple_extract_load_to_load_tuple_element_addr : $@convention(thin) (@inout (Builtin.Int8, Builtin.Int8)) -> (Builtin.Int8) {
 bb0(%0 : $*(Builtin.Int8, Builtin.Int8)):
   %1 = load [trivial] %0 : $*(Builtin.Int8, Builtin.Int8)
+  %2 = integer_literal $Builtin.Int8, 1
+  %3 = tuple_element_addr %0 : $*(Builtin.Int8, Builtin.Int8), 0
+  store %2 to [trivial] %3 : $*Builtin.Int8
   %5 = tuple_extract %1 : $(Builtin.Int8, Builtin.Int8), 0
   return %5 : $Builtin.Int8
 }
@@ -36,12 +48,18 @@ bb0(%0 : $*(Builtin.Int8, Builtin.Int8)):
 // CHECK-LABEL: sil [ossa] @multiple_use_struct_extract_load_to_load_struct_element_addr
 // CHECK: bb0([[IN:%[0-9]+]] : $*UInt8):
 // CHECK-NEXT: load
+// CHECK-NEXT: integer_literal
+// CHECK-NEXT: struct
+// CHECK-NEXT: store
 // CHECK-NEXT: struct_extract
 // CHECK-NEXT: tuple
 // CHECK-NEXT: return
 sil [ossa] @multiple_use_struct_extract_load_to_load_struct_element_addr : $@convention(thin) (@inout UInt8) -> (UInt8, Builtin.Int8) {
 bb0(%0 : $*UInt8):
   %1 = load [trivial] %0 : $*UInt8
+  %2 = integer_literal $Builtin.Int8, 1
+  %3 = struct_element_addr %0 : $*UInt8, #UInt8._value
+  store %2 to [trivial] %3 : $*Builtin.Int8
   %5 = struct_extract %1 : $UInt8, #UInt8._value
   %6 = tuple (%1 : $UInt8, %5 : $Builtin.Int8)
   return %6 : $(UInt8, Builtin.Int8)
@@ -52,12 +70,18 @@ bb0(%0 : $*UInt8):
 // CHECK-LABEL: sil [ossa] @multiple_use_tuple_extract_load_to_load_tuple_element_addr
 // CHECK: bb0
 // CHECK-NEXT: load
+// CHECK-NEXT: integer_literal
+// CHECK-NEXT: tuple_element_addr
+// CHECK-NEXT: store
 // CHECK-NEXT: tuple_extract
 // CHECK-NEXT: tuple
 // CHECK-NEXT: return
 sil [ossa] @multiple_use_tuple_extract_load_to_load_tuple_element_addr : $@convention(thin) (@inout (Builtin.Int8, Builtin.Int8)) -> ((Builtin.Int8, Builtin.Int8), Builtin.Int8) {
 bb0(%0 : $*(Builtin.Int8, Builtin.Int8)):
   %1 = load [trivial] %0 : $*(Builtin.Int8, Builtin.Int8)
+  %2 = integer_literal $Builtin.Int8, 1
+  %3 = tuple_element_addr %0 : $*(Builtin.Int8, Builtin.Int8), 0
+  store %2 to [trivial] %3 : $*Builtin.Int8
   %5 = tuple_extract %1 : $(Builtin.Int8, Builtin.Int8), 0
   %6 = tuple (%1 : $(Builtin.Int8, Builtin.Int8), %5 : $Builtin.Int8)
   return %6 : $((Builtin.Int8, Builtin.Int8), Builtin.Int8)
@@ -115,3 +139,32 @@ bb0(%0 : $*X1):
   %result = tuple (%a : $Int, %copy1 : $AnyObject, %copy2 : $AnyObject)
   return %result : $(Int, AnyObject, AnyObject)
 }
+
+struct X2 {
+  @_hasStorage @_hasInitialValue var obj: AnyObject { get set }
+}
+
+struct X3 {
+  @_hasStorage @_hasInitialValue var x2: X2 { get set }
+}
+
+// CHECK-LABEL: sil private [ossa] @testStoreNontrivial : $@convention(thin) (@inout X3, @guaranteed AnyObject) -> () {
+// CHECK-LABEL: bb0(%0 : $*X3, %1 : @guaranteed $AnyObject):
+// CHECK: [[CP:%.*]] = copy_value %1 : $AnyObject
+// CHECK: [[ACCESS:%.*]] = begin_access [modify] [unknown] %0 : $*X3
+// CHECK: [[X2:%.*]] = struct $X2 ([[CP]] : $AnyObject)
+// CHECK: [[X3:%.*]] = struct $X3 ([[X2]] : $X2)
+// CHECK: store [[X3]] to [assign] [[ACCESS]] : $*X3
+// CHECK: end_access [[ACCESS]] : $*X3
+// CHECK-LABEL: } // end sil function 'testStoreNontrivial'
+sil private [ossa] @testStoreNontrivial : $@convention(thin) (@inout X3, @guaranteed AnyObject) -> () {
+bb0(%0 : $*X3, %1 : $AnyObject):
+  %4 = copy_value %1 : $AnyObject
+  %5 = begin_access [modify] [unknown] %0 : $*X3
+  %6 = struct_element_addr %5 : $*X3, #X3.x2
+  %7 = struct_element_addr %6 : $*X2, #X2.obj
+  store %4 to [assign] %7 : $*AnyObject
+  end_access %5 : $*X3
+  %12 = tuple ()
+  return %12 : $()
+}