Skip to content

Commit 7004322

Browse files
swift-ciswift-ci
authored
Merge pull request swiftlang#28196 from gottesmm/pr-a5b32ece5ba488e8d35fdb4460bb3e64a1fc716e
2 parents 28e4f52 + 64eeed8 commit 7004322

File tree

2 files changed

+177
-11
lines changed

2 files changed

+177
-11
lines changed

‎lib/SILOptimizer/Mandatory/PredictableMemOpt.cpp

Lines changed: 42 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -567,6 +567,10 @@ SILValue AvailableValueAggregator::aggregateValues(SILType LoadTy,
567567
return aggregateStructSubElts(SD, LoadTy, Address, FirstElt);
568568

569569
// Otherwise, we have a non-aggregate primitive. Load or extract the value.
570+
//
571+
// NOTE: We should never call this when taking since when taking we know that
572+
// our underlying value is always fully available.
573+
assert(!isTake());
570574
return handlePrimitiveValue(LoadTy, Address, FirstElt);
571575
}
572576

@@ -633,8 +637,19 @@ AvailableValueAggregator::aggregateFullyAvailableValue(SILType loadTy,
633637

634638
// If we only are tracking a singular value, we do not need to construct
635639
// SSA. Just return that value.
636-
if (auto val = singularValue.getValueOr(SILValue()))
640+
if (auto val = singularValue.getValueOr(SILValue())) {
641+
// This assert documents that we are expecting that if we are in ossa, have
642+
// a non-trivial value, and are not taking, we should never go down this
643+
// code path. If we did, we would need to insert a copy here. The reason why
644+
// we know we will never go down this code path is since we have been
645+
// inserting copy_values implying that our potential singular value would be
646+
// of the copy_values which are guaranteed to all be different.
647+
assert((!B.hasOwnership() || isTake() ||
648+
val->getType().isTrivial(*B.getInsertionBB()->getParent())) &&
649+
"Should never reach this code path if we are in ossa and have a "
650+
"non-trivial value");
637651
return val;
652+
}
638653

639654
// Finally, grab the value from the SSA updater.
640655
SILValue result = updater.GetValueInMiddleOfBlock(B.getInsertionBB());
@@ -696,17 +711,18 @@ SILValue AvailableValueAggregator::aggregateStructSubElts(StructDecl *sd,
696711
return B.createStruct(Loc, loadTy, resultElts);
697712
}
698713

699-
// We have looked through all of the aggregate values and finally found a
700-
// "primitive value". If the value is available, use it (extracting if we need
701-
// to), otherwise emit a load of the value with the appropriate qualifier.
714+
// We have looked through all of the aggregate values and finally found a value
715+
// that is not available without transforming, i.e. a "primitive value". If the
716+
// value is available, use it (extracting if we need to), otherwise emit a load
717+
// of the value with the appropriate qualifier.
702718
SILValue AvailableValueAggregator::handlePrimitiveValue(SILType loadTy,
703719
SILValue address,
704720
unsigned firstElt) {
705-
auto &val = AvailableValueList[firstElt];
721+
assert(!isTake() && "Should only take fully available values?!");
706722

707723
// If the value is not available, load the value and update our use list.
724+
auto &val = AvailableValueList[firstElt];
708725
if (!val) {
709-
assert(!isTake() && "Should only take fully available values?!");
710726
LoadInst *load = ([&]() {
711727
if (B.hasOwnership()) {
712728
return B.createTrivialLoadOr(Loc, address,
@@ -736,7 +752,10 @@ SILValue AvailableValueAggregator::handlePrimitiveValue(SILType loadTy,
736752
}
737753

738754
// If we have an available value, then we want to extract the subelement from
739-
// the borrowed aggregate before each insertion point.
755+
// the borrowed aggregate before each insertion point. Note that since we have
756+
// inserted copies at each of these insertion points, we know that we will
757+
// never have the same value along all paths unless we have a trivial value
758+
// meaning the SSA updater given a non-trivial value must /always/ be used.
740759
SILSSAUpdater updater;
741760
updater.Initialize(loadTy);
742761

@@ -759,13 +778,25 @@ SILValue AvailableValueAggregator::handlePrimitiveValue(SILType loadTy,
759778
updater.AddAvailableValue(i->getParent(), eltVal);
760779
}
761780

762-
// If we only are tracking a singular value, we do not need to construct
763-
// SSA. Just return that value.
764-
if (auto val = singularValue.getValueOr(SILValue()))
781+
SILBasicBlock *insertBlock = B.getInsertionBB();
782+
783+
// If we are not in ossa and have a singular value or if we are in ossa and
784+
// have a trivial singular value, just return that value.
785+
//
786+
// This can never happen for non-trivial values in ossa since we never should
787+
// visit this code path if we have a take implying that non-trivial values
788+
// /will/ have a copy and thus are guaranteed (since each copy yields a
789+
// different value) to not be singular values.
790+
if (auto val = singularValue.getValueOr(SILValue())) {
791+
assert((!B.hasOwnership() ||
792+
val->getType().isTrivial(*insertBlock->getParent())) &&
793+
"Should have inserted copies for each insertion point, so shouldn't "
794+
"have a singular value if non-trivial?!");
765795
return val;
796+
}
766797

767798
// Finally, grab the value from the SSA updater.
768-
SILValue eltVal = updater.GetValueInMiddleOfBlock(B.getInsertionBB());
799+
SILValue eltVal = updater.GetValueInMiddleOfBlock(insertBlock);
769800
assert(!B.hasOwnership() ||
770801
eltVal.getOwnershipKind().isCompatibleWith(ValueOwnershipKind::Owned));
771802
assert(eltVal->getType() == loadTy && "Subelement types mismatch");

‎test/SILOptimizer/predictable_memaccess_opts.sil

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,14 @@ struct NativeObjectPair {
1616
var y: Builtin.NativeObject
1717
}
1818

19+
struct IntPair {
20+
var x: Builtin.Int32
21+
var y: Builtin.Int32
22+
}
23+
1924
sil @guaranteed_object_user : $@convention(thin) (@guaranteed Builtin.NativeObject) -> ()
25+
sil @intpair_user : $@convention(thin) (IntPair) -> ()
26+
sil @inout_int32_user : $@convention(thin) (@inout Builtin.Int32) -> ()
2027

2128
/// Needed to avoid tuple scalarization code in the use gatherer.
2229
struct NativeObjectAndTuple {
@@ -686,3 +693,131 @@ bb0(%0 : @owned $Builtin.NativeObject, %1 : @owned $Builtin.NativeObject):
686693
%9999 = tuple()
687694
return %9999 : $()
688695
}
696+
697+
// CHECK-LABEL: sil [ossa] @multiple_available_values_diamond_followed_by_loop_trivial : $@convention(thin) (Builtin.Int32, Builtin.Int32) -> () {
698+
// CHECK: bb0(
699+
// CHECK-NOT: load [trivial] %{{[0-9][0-9]*}} : $*IntPair
700+
// CHECK-NOT: bb{{[0-9][0-9]*}}(
701+
// CHECK: } // end sil function 'multiple_available_values_diamond_followed_by_loop_trivial'
702+
sil [ossa] @multiple_available_values_diamond_followed_by_loop_trivial : $@convention(thin) (Builtin.Int32, Builtin.Int32) -> () {
703+
bb0(%0a : $Builtin.Int32, %0b : $Builtin.Int32):
704+
%func = function_ref @intpair_user : $@convention(thin) (IntPair) -> ()
705+
%1 = alloc_stack $IntPair
706+
%1a = struct_element_addr %1 : $*IntPair, #IntPair.x
707+
%1b = struct_element_addr %1 : $*IntPair, #IntPair.y
708+
cond_br undef, bb1, bb2
709+
710+
bb1:
711+
store %0a to [trivial] %1a : $*Builtin.Int32
712+
store %0b to [trivial] %1b : $*Builtin.Int32
713+
br bb3
714+
715+
bb2:
716+
store %0a to [trivial] %1a : $*Builtin.Int32
717+
store %0b to [trivial] %1b : $*Builtin.Int32
718+
br bb3
719+
720+
bb3:
721+
br bb4
722+
723+
bb4:
724+
br bb5
725+
726+
bb5:
727+
%2 = load [trivial] %1 : $*IntPair
728+
cond_br undef, bb6, bb7
729+
730+
bb6:
731+
apply %func(%2) : $@convention(thin) (IntPair) -> ()
732+
br bb5
733+
734+
bb7:
735+
apply %func(%2) : $@convention(thin) (IntPair) -> ()
736+
dealloc_stack %1 : $*IntPair
737+
%9999 = tuple()
738+
return %9999 : $()
739+
}
740+
741+
// CHECK-LABEL: sil [ossa] @multiple_available_values_diamond_followed_by_loop_trivial_reload : $@convention(thin) (Builtin.Int32, Builtin.Int32, Builtin.Int32) -> () {
742+
// CHECK: bb0(
743+
// CHECK-NOT: load [trivial] %{{[0-9][0-9]*}} : $*IntPair
744+
// CHECK-NOT: bb{{[0-9][0-9]*}}(
745+
// CHECK: } // end sil function 'multiple_available_values_diamond_followed_by_loop_trivial_reload'
746+
sil [ossa] @multiple_available_values_diamond_followed_by_loop_trivial_reload : $@convention(thin) (Builtin.Int32, Builtin.Int32, Builtin.Int32) -> () {
747+
bb0(%0a : $Builtin.Int32, %0b : $Builtin.Int32, %0c : $Builtin.Int32):
748+
%func = function_ref @intpair_user : $@convention(thin) (IntPair) -> ()
749+
%1 = alloc_stack $IntPair
750+
%1a = struct_element_addr %1 : $*IntPair, #IntPair.x
751+
%1b = struct_element_addr %1 : $*IntPair, #IntPair.y
752+
cond_br undef, bb1, bb2
753+
754+
bb1:
755+
store %0a to [trivial] %1a : $*Builtin.Int32
756+
store %0c to [trivial] %1b : $*Builtin.Int32
757+
br bb3
758+
759+
bb2:
760+
store %0a to [trivial] %1a : $*Builtin.Int32
761+
store %0b to [trivial] %1b : $*Builtin.Int32
762+
br bb3
763+
764+
bb3:
765+
br bb4
766+
767+
bb4:
768+
br bb5
769+
770+
bb5:
771+
%2 = load [trivial] %1 : $*IntPair
772+
cond_br undef, bb6, bb7
773+
774+
bb6:
775+
apply %func(%2) : $@convention(thin) (IntPair) -> ()
776+
br bb5
777+
778+
bb7:
779+
apply %func(%2) : $@convention(thin) (IntPair) -> ()
780+
dealloc_stack %1 : $*IntPair
781+
%9999 = tuple()
782+
return %9999 : $()
783+
}
784+
785+
sil [ossa] @multiple_available_values_diamond_followed_by_loop_trivial_store_in_loop : $@convention(thin) (Builtin.Int32, Builtin.Int32, Builtin.Int32) -> () {
786+
bb0(%0a : $Builtin.Int32, %0b : $Builtin.Int32, %0c : $Builtin.Int32):
787+
%func = function_ref @intpair_user : $@convention(thin) (IntPair) -> ()
788+
%1 = alloc_stack $IntPair
789+
%1a = struct_element_addr %1 : $*IntPair, #IntPair.x
790+
%1b = struct_element_addr %1 : $*IntPair, #IntPair.y
791+
cond_br undef, bb1, bb2
792+
793+
bb1:
794+
store %0a to [trivial] %1a : $*Builtin.Int32
795+
store %0b to [trivial] %1b : $*Builtin.Int32
796+
br bb3
797+
798+
bb2:
799+
store %0a to [trivial] %1a : $*Builtin.Int32
800+
store %0b to [trivial] %1b : $*Builtin.Int32
801+
br bb3
802+
803+
bb3:
804+
br bb4
805+
806+
bb4:
807+
br bb5
808+
809+
bb5:
810+
%2 = load [trivial] %1 : $*IntPair
811+
cond_br undef, bb6, bb7
812+
813+
bb6:
814+
apply %func(%2) : $@convention(thin) (IntPair) -> ()
815+
store %0b to [trivial] %1b : $*Builtin.Int32
816+
br bb5
817+
818+
bb7:
819+
apply %func(%2) : $@convention(thin) (IntPair) -> ()
820+
dealloc_stack %1 : $*IntPair
821+
%9999 = tuple()
822+
return %9999 : $()
823+
}

0 commit comments

Comments
 (0)