[pmo] Add a few more asserts before I fix strong control equivalence issues

lib/SILOptimizer/Mandatory/PredictableMemOpt.cpp

@@ -567,6 +567,10 @@ SILValue AvailableValueAggregator::aggregateValues(SILType LoadTy,
     return aggregateStructSubElts(SD, LoadTy, Address, FirstElt);
 
   // Otherwise, we have a non-aggregate primitive. Load or extract the value.
+  //
+  // NOTE: We should never call this when taking since when taking we know that
+  // our underlying value is always fully available.
+  assert(!isTake());
   return handlePrimitiveValue(LoadTy, Address, FirstElt);
 }
 
@@ -633,8 +637,19 @@ AvailableValueAggregator::aggregateFullyAvailableValue(SILType loadTy,
 
   // If we only are tracking a singular value, we do not need to construct
   // SSA. Just return that value.
-  if (auto val = singularValue.getValueOr(SILValue()))
+  if (auto val = singularValue.getValueOr(SILValue())) {
+    // This assert documents that we are expecting that if we are in ossa, have
+    // a non-trivial value, and are not taking, we should never go down this
+    // code path. If we did, we would need to insert a copy here. The reason why
+    // we know we will never go down this code path is since we have been
+    // inserting copy_values implying that our potential singular value would be
+    // of the copy_values which are guaranteed to all be different.
+    assert((!B.hasOwnership() || isTake() ||
+            val->getType().isTrivial(*B.getInsertionBB()->getParent())) &&
+           "Should never reach this code path if we are in ossa and have a "
+           "non-trivial value");
     return val;
+  }
 
   // Finally, grab the value from the SSA updater.
   SILValue result = updater.GetValueInMiddleOfBlock(B.getInsertionBB());
@@ -696,17 +711,18 @@ SILValue AvailableValueAggregator::aggregateStructSubElts(StructDecl *sd,
   return B.createStruct(Loc, loadTy, resultElts);
 }
 
-// We have looked through all of the aggregate values and finally found a
-// "primitive value". If the value is available, use it (extracting if we need
-// to), otherwise emit a load of the value with the appropriate qualifier.
+// We have looked through all of the aggregate values and finally found a value
+// that is not available without transforming, i.e. a "primitive value". If the
+// value is available, use it (extracting if we need to), otherwise emit a load
+// of the value with the appropriate qualifier.
 SILValue AvailableValueAggregator::handlePrimitiveValue(SILType loadTy,
                                                         SILValue address,
                                                         unsigned firstElt) {
-  auto &val = AvailableValueList[firstElt];
+  assert(!isTake() && "Should only take fully available values?!");
 
   // If the value is not available, load the value and update our use list.
+  auto &val = AvailableValueList[firstElt];
   if (!val) {
-    assert(!isTake() && "Should only take fully available values?!");
     LoadInst *load = ([&]() {
       if (B.hasOwnership()) {
         return B.createTrivialLoadOr(Loc, address,
@@ -736,7 +752,10 @@ SILValue AvailableValueAggregator::handlePrimitiveValue(SILType loadTy,
   }
 
   // If we have an available value, then we want to extract the subelement from
-  // the borrowed aggregate before each insertion point.
+  // the borrowed aggregate before each insertion point. Note that since we have
+  // inserted copies at each of these insertion points, we know that we will
+  // never have the same value along all paths unless we have a trivial value
+  // meaning the SSA updater given a non-trivial value must /always/ be used.
   SILSSAUpdater updater;
   updater.Initialize(loadTy);
 
@@ -759,13 +778,25 @@ SILValue AvailableValueAggregator::handlePrimitiveValue(SILType loadTy,
     updater.AddAvailableValue(i->getParent(), eltVal);
   }
 
-  // If we only are tracking a singular value, we do not need to construct
-  // SSA. Just return that value.
-  if (auto val = singularValue.getValueOr(SILValue()))
+  SILBasicBlock *insertBlock = B.getInsertionBB();
+
+  // If we are not in ossa and have a singular value or if we are in ossa and
+  // have a trivial singular value, just return that value.
+  //
+  // This can never happen for non-trivial values in ossa since we never should
+  // visit this code path if we have a take implying that non-trivial values
+  // /will/ have a copy and thus are guaranteed (since each copy yields a
+  // different value) to not be singular values.
+  if (auto val = singularValue.getValueOr(SILValue())) {
+    assert((!B.hasOwnership() ||
+            val->getType().isTrivial(*insertBlock->getParent())) &&
+           "Should have inserted copies for each insertion point, so shouldn't "
+           "have a singular value if non-trivial?!");
     return val;
+  }
 
   // Finally, grab the value from the SSA updater.
-  SILValue eltVal = updater.GetValueInMiddleOfBlock(B.getInsertionBB());
+  SILValue eltVal = updater.GetValueInMiddleOfBlock(insertBlock);
   assert(!B.hasOwnership() ||
          eltVal.getOwnershipKind().isCompatibleWith(ValueOwnershipKind::Owned));
   assert(eltVal->getType() == loadTy && "Subelement types mismatch");

test/SILOptimizer/predictable_memaccess_opts.sil

@@ -16,7 +16,14 @@ struct NativeObjectPair {
   var y: Builtin.NativeObject
 }
 
+struct IntPair {
+  var x: Builtin.Int32
+  var y: Builtin.Int32
+}
+
 sil @guaranteed_object_user : $@convention(thin) (@guaranteed Builtin.NativeObject) -> ()
+sil @intpair_user : $@convention(thin) (IntPair) -> ()
+sil @inout_int32_user : $@convention(thin) (@inout Builtin.Int32) -> ()
 
 /// Needed to avoid tuple scalarization code in the use gatherer.
 struct NativeObjectAndTuple {
@@ -686,3 +693,131 @@ bb0(%0 : @owned $Builtin.NativeObject, %1 : @owned $Builtin.NativeObject):
   %9999 = tuple()
   return %9999 : $()
 }
+
+// CHECK-LABEL: sil [ossa] @multiple_available_values_diamond_followed_by_loop_trivial : $@convention(thin) (Builtin.Int32, Builtin.Int32) -> () {
+// CHECK: bb0(
+// CHECK-NOT: load [trivial] %{{[0-9][0-9]*}} : $*IntPair
+// CHECK-NOT: bb{{[0-9][0-9]*}}(
+// CHECK: } // end sil function 'multiple_available_values_diamond_followed_by_loop_trivial'
+sil [ossa] @multiple_available_values_diamond_followed_by_loop_trivial : $@convention(thin) (Builtin.Int32, Builtin.Int32) -> () {
+bb0(%0a : $Builtin.Int32, %0b : $Builtin.Int32):
+  %func = function_ref @intpair_user : $@convention(thin) (IntPair) -> ()
+  %1 = alloc_stack $IntPair
+  %1a = struct_element_addr %1 : $*IntPair, #IntPair.x
+  %1b = struct_element_addr %1 : $*IntPair, #IntPair.y
+  cond_br undef, bb1, bb2
+
+bb1:
+  store %0a to [trivial] %1a : $*Builtin.Int32
+  store %0b to [trivial] %1b : $*Builtin.Int32
+  br bb3
+
+bb2:
+  store %0a to [trivial] %1a : $*Builtin.Int32
+  store %0b to [trivial] %1b : $*Builtin.Int32
+  br bb3
+
+bb3:
+  br bb4
+
+bb4:
+  br bb5
+
+bb5:
+  %2 = load [trivial] %1 : $*IntPair
+  cond_br undef, bb6, bb7
+
+bb6:
+  apply %func(%2) : $@convention(thin) (IntPair) -> ()
+  br bb5
+
+bb7:
+  apply %func(%2) : $@convention(thin) (IntPair) -> ()
+  dealloc_stack %1 : $*IntPair
+  %9999 = tuple()
+  return %9999 : $()
+}
+
+// CHECK-LABEL: sil [ossa] @multiple_available_values_diamond_followed_by_loop_trivial_reload : $@convention(thin) (Builtin.Int32, Builtin.Int32, Builtin.Int32) -> () {
+// CHECK: bb0(
+// CHECK-NOT: load [trivial] %{{[0-9][0-9]*}} : $*IntPair
+// CHECK-NOT: bb{{[0-9][0-9]*}}(
+// CHECK: } // end sil function 'multiple_available_values_diamond_followed_by_loop_trivial_reload'
+sil [ossa] @multiple_available_values_diamond_followed_by_loop_trivial_reload : $@convention(thin) (Builtin.Int32, Builtin.Int32, Builtin.Int32) -> () {
+bb0(%0a : $Builtin.Int32, %0b : $Builtin.Int32, %0c : $Builtin.Int32):
+  %func = function_ref @intpair_user : $@convention(thin) (IntPair) -> ()
+  %1 = alloc_stack $IntPair
+  %1a = struct_element_addr %1 : $*IntPair, #IntPair.x
+  %1b = struct_element_addr %1 : $*IntPair, #IntPair.y
+  cond_br undef, bb1, bb2
+
+bb1:
+  store %0a to [trivial] %1a : $*Builtin.Int32
+  store %0c to [trivial] %1b : $*Builtin.Int32
+  br bb3
+
+bb2:
+  store %0a to [trivial] %1a : $*Builtin.Int32
+  store %0b to [trivial] %1b : $*Builtin.Int32
+  br bb3
+
+bb3:
+  br bb4
+
+bb4:
+  br bb5
+
+bb5:
+  %2 = load [trivial] %1 : $*IntPair
+  cond_br undef, bb6, bb7
+
+bb6:
+  apply %func(%2) : $@convention(thin) (IntPair) -> ()
+  br bb5
+
+bb7:
+  apply %func(%2) : $@convention(thin) (IntPair) -> ()
+  dealloc_stack %1 : $*IntPair
+  %9999 = tuple()
+  return %9999 : $()
+}
+
+sil [ossa] @multiple_available_values_diamond_followed_by_loop_trivial_store_in_loop : $@convention(thin) (Builtin.Int32, Builtin.Int32, Builtin.Int32) -> () {
+bb0(%0a : $Builtin.Int32, %0b : $Builtin.Int32, %0c : $Builtin.Int32):
+  %func = function_ref @intpair_user : $@convention(thin) (IntPair) -> ()
+  %1 = alloc_stack $IntPair
+  %1a = struct_element_addr %1 : $*IntPair, #IntPair.x
+  %1b = struct_element_addr %1 : $*IntPair, #IntPair.y
+  cond_br undef, bb1, bb2
+
+bb1:
+  store %0a to [trivial] %1a : $*Builtin.Int32
+  store %0b to [trivial] %1b : $*Builtin.Int32
+  br bb3
+
+bb2:
+  store %0a to [trivial] %1a : $*Builtin.Int32
+  store %0b to [trivial] %1b : $*Builtin.Int32
+  br bb3
+
+bb3:
+  br bb4
+
+bb4:
+  br bb5
+
+bb5:
+  %2 = load [trivial] %1 : $*IntPair
+  cond_br undef, bb6, bb7
+
+bb6:
+  apply %func(%2) : $@convention(thin) (IntPair) -> ()
+  store %0b to [trivial] %1b : $*Builtin.Int32
+  br bb5
+
+bb7:
+  apply %func(%2) : $@convention(thin) (IntPair) -> ()
+  dealloc_stack %1 : $*IntPair
+  %9999 = tuple()
+  return %9999 : $()
+}