Skip to content

Commit 64eeed8

Browse files
gottesmmgottesmm
committed
[pmo] Add an assert that when using SILSSAUpdater in ossa, we only have singular values if the underlying type is either trivial or we have a take.
The reason why this is true is that we will be inserting new copy_values for each available value implying that we can never have such a singular value. I also added two test cases that show that we have a singular value with the trivial type and that works.
1 parent 55b6ff1 commit 64eeed8

File tree

2 files changed

+167
-6
lines changed

2 files changed

+167
-6
lines changed

‎lib/SILOptimizer/Mandatory/PredictableMemOpt.cpp

Lines changed: 32 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -637,8 +637,19 @@ AvailableValueAggregator::aggregateFullyAvailableValue(SILType loadTy,
637637

638638
// If we only are tracking a singular value, we do not need to construct
639639
// SSA. Just return that value.
640-
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");
641651
return val;
652+
}
642653

643654
// Finally, grab the value from the SSA updater.
644655
SILValue result = updater.GetValueInMiddleOfBlock(B.getInsertionBB());
@@ -741,7 +752,10 @@ SILValue AvailableValueAggregator::handlePrimitiveValue(SILType loadTy,
741752
}
742753

743754
// If we have an available value, then we want to extract the subelement from
744-
// 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.
745759
SILSSAUpdater updater;
746760
updater.Initialize(loadTy);
747761

@@ -764,13 +778,25 @@ SILValue AvailableValueAggregator::handlePrimitiveValue(SILType loadTy,
764778
updater.AddAvailableValue(i->getParent(), eltVal);
765779
}
766780

767-
// If we only are tracking a singular value, we do not need to construct
768-
// SSA. Just return that value.
769-
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?!");
770795
return val;
796+
}
771797

772798
// Finally, grab the value from the SSA updater.
773-
SILValue eltVal = updater.GetValueInMiddleOfBlock(B.getInsertionBB());
799+
SILValue eltVal = updater.GetValueInMiddleOfBlock(insertBlock);
774800
assert(!B.hasOwnership() ||
775801
eltVal.getOwnershipKind().isCompatibleWith(ValueOwnershipKind::Owned));
776802
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)