Skip to content

Commit e4e1689

Browse files
committed
[ownership] When RAUWing addresses, do not search for intptrs for addresses rooted in pointer_to_address.
The reason why is that addresses from pointer_to_address never have transitive interior pointer constraints from where ever the pointer originally came from. This is the issue that was causing a CSE test to fail, so I added a test to ossa_rauw_test that works this code path.
1 parent 5136581 commit e4e1689

File tree

5 files changed

+80
-5
lines changed

5 files changed

+80
-5
lines changed

include/swift/SIL/InstructionUtils.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,10 @@ SILValue getUnderlyingObject(SILValue V);
2727

2828
SILValue getUnderlyingObjectStopAtMarkDependence(SILValue V);
2929

30+
/// Given an address look through address to address projections and indexing
31+
/// insts.
32+
SILValue getUnderlyingObjectStoppingAtObjectToAddrProjections(SILValue v);
33+
3034
SILValue stripSinglePredecessorArgs(SILValue V);
3135

3236
/// Return the underlying SILValue after stripping off all casts from the
@@ -55,8 +59,14 @@ SILValue stripClassCasts(SILValue V);
5559

5660
/// Return the underlying SILValue after stripping off all address projection
5761
/// instructions.
62+
///
63+
/// FIXME: Today address projections are referring to the result of the
64+
/// projection and doesn't consider the operand. Should we change this?
5865
SILValue stripAddressProjections(SILValue V);
5966

67+
/// Look through any projections that transform an address -> an address.
68+
SILValue lookThroughAddressToAddressProjections(SILValue v);
69+
6070
/// Return the underlying SILValue after stripping off all aggregate projection
6171
/// instructions.
6272
///

include/swift/SIL/Projection.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -391,6 +391,10 @@ class Projection {
391391
}
392392
}
393393

394+
static bool isAddressToAddressProjection(SILValue v) {
395+
return isAddressProjection(v) && !isObjectToAddressProjection(v);
396+
}
397+
394398
/// Returns true if this instruction projects from an object type into an
395399
/// address subtype.
396400
static bool isObjectToAddressProjection(SILValue V) {

lib/SIL/Utils/InstructionUtils.cpp

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,20 @@ SILValue swift::getUnderlyingObject(SILValue v) {
6262
}
6363
}
6464

65+
SILValue
66+
swift::getUnderlyingObjectStoppingAtObjectToAddrProjections(SILValue v) {
67+
if (!v->getType().isAddress())
68+
return SILValue();
69+
70+
while (true) {
71+
auto v2 = lookThroughAddressToAddressProjections(v);
72+
v2 = stripIndexingInsts(v2);
73+
if (v2 == v)
74+
return v2;
75+
v = v2;
76+
}
77+
}
78+
6579
SILValue swift::getUnderlyingObjectStopAtMarkDependence(SILValue v) {
6680
while (true) {
6781
SILValue v2 = stripCastsWithoutMarkDependence(v);
@@ -207,6 +221,15 @@ SILValue swift::stripAddressProjections(SILValue V) {
207221
}
208222
}
209223

224+
SILValue swift::lookThroughAddressToAddressProjections(SILValue v) {
225+
while (true) {
226+
v = stripSinglePredecessorArgs(v);
227+
if (!Projection::isAddressToAddressProjection(v))
228+
return v;
229+
v = cast<SingleValueInstruction>(v)->getOperand(0);
230+
}
231+
}
232+
210233
SILValue swift::stripValueProjections(SILValue V) {
211234
while (true) {
212235
V = stripSinglePredecessorArgs(V);

lib/SILOptimizer/Utils/OwnershipOptUtils.cpp

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -901,28 +901,46 @@ OwnershipRAUWHelper::OwnershipRAUWHelper(OwnershipFixupContext &inputCtx,
901901
// But if we have an address, we need to check if new value is from an
902902
// interior pointer or not in a way that the pass understands. What we do is:
903903
//
904-
// 1. Compute the AccessPathWithBase of newValue. If we do not get back a
904+
// 1. Early exit some cases that we know can never have interior pointers.
905+
//
906+
// 2. Compute the AccessPathWithBase of newValue. If we do not get back a
905907
// valid such object, invalidate and then bail.
906908
//
907-
// 2. Then we check if the base address is the result of an interior pointer
909+
// 3. Then we check if the base address is the result of an interior pointer
908910
// instruction. If we do not find one we bail.
909911
//
910-
// 3. Then grab the base value of the interior pointer operand. We only
912+
// 4. Then grab the base value of the interior pointer operand. We only
911913
// support cases where we have a single BorrowedValue as our base. This is
912914
// a safe future proof assumption since one reborrows are on
913915
// structs/tuple/destructures, a guaranteed value will always be associated
914916
// with a single BorrowedValue, so this will never fail (and the code will
915917
// probably be DCEed).
916918
//
917-
// 4. Then we compute an AccessPathWithBase for oldValue and then find its
919+
// 5. Then we compute an AccessPathWithBase for oldValue and then find its
918920
// derived uses. If we fail, we bail.
919921
//
920-
// 5. At this point, we know that we can perform this RAUW. The only question
922+
// 6. At this point, we know that we can perform this RAUW. The only question
921923
// is if we need to when we RAUW copy the interior pointer base value. We
922924
// perform this check by making sure all of the old value's derived uses
923925
// are within our BorrowedValue's scope. If so, we clear the extra state we
924926
// were tracking (the interior pointer/oldValue's transitive uses), so we
925927
// perform just a normal RAUW (without inserting the copy) when we RAUW.
928+
//
929+
// We can always RAUW an address with a pointer_to_address since if there
930+
// were any interior pointer constraints on whatever address pointer came
931+
// from, the address_to_pointer producing that value erases that
932+
// information, so we can RAUW without worrying.
933+
//
934+
// NOTE: We also need to handle this here since a pointer_to_address is not a
935+
// valid base value for an access path since it doesn't refer to any storage.
936+
{
937+
auto baseProj =
938+
getUnderlyingObjectStoppingAtObjectToAddrProjections(newValue);
939+
if (isa<PointerToAddressInst>(baseProj)) {
940+
return;
941+
}
942+
}
943+
926944
auto accessPathWithBase = AccessPathWithBase::compute(newValue);
927945
if (!accessPathWithBase.base) {
928946
// Invalidate!

test/SILOptimizer/ossa_rauw_tests.sil

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -973,3 +973,23 @@ bb4:
973973
destroy_value %0 : $Klass
974974
return %3 : $Builtin.NativeObject
975975
}
976+
977+
// Make sure that we always RAUW if our new value address is from a
978+
// pointer_to_address since we don't track interior pointers that escape through
979+
// address_to_pointer.
980+
//
981+
// CHECK-LABEL: sil [ossa] @interior_pointer_no_base_new_value : $@convention(thin) (Builtin.RawPointer, Builtin.Word) -> @owned Builtin.NativeObject {
982+
// CHECK: pointer_to_address
983+
// CHECK-NOT: address_to_pointer
984+
// CHECK-NOT: pointer_to_address
985+
// CHECK: } // end sil function 'interior_pointer_no_base_new_value'
986+
sil [ossa] @interior_pointer_no_base_new_value : $@convention(thin) (Builtin.RawPointer, Builtin.Word) -> @owned Builtin.NativeObject {
987+
bb0(%0 : $Builtin.RawPointer, %count : $Builtin.Word):
988+
%0a = index_raw_pointer %0 : $Builtin.RawPointer, %count : $Builtin.Word
989+
%2 = pointer_to_address %0a : $Builtin.RawPointer to [strict] $*Builtin.NativeObject
990+
%3 = index_addr %2 : $*Builtin.NativeObject, %count : $Builtin.Word
991+
%4 = address_to_pointer %3 : $*Builtin.NativeObject to $Builtin.RawPointer
992+
%5 = pointer_to_address %4 : $Builtin.RawPointer to [strict] $*Builtin.NativeObject
993+
%6 = load [copy] %5 : $*Builtin.NativeObject
994+
return %6 : $Builtin.NativeObject
995+
}

0 commit comments

Comments
 (0)