Skip to content

Commit d40c915

Browse files
committed
Fix ownership rauw to not leave behind stale ownership fixup context
Ownership rauw uses a shared ownership fixup context to maintain state. When ownership rauw fails, due to some invalid condition, we leave behind stale data in this shared ownership fixup context. This stale context can indvertantly affect the next rauw on addresses. In addition to setting the ownership fixup context to nullptr, we should also clear it so that it's internal data structures are cleared.
1 parent 923c69d commit d40c915

File tree

3 files changed

+52
-10
lines changed

3 files changed

+52
-10
lines changed

include/swift/SILOptimizer/Utils/OwnershipOptUtils.h

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,11 @@ class OwnershipRAUWHelper {
144144
private:
145145
SILBasicBlock::iterator replaceAddressUses(SingleValueInstruction *oldValue,
146146
SILValue newValue);
147+
148+
void invalidate() {
149+
ctx->clear();
150+
ctx = nullptr;
151+
}
147152
};
148153

149154
/// A utility composed ontop of OwnershipFixupContext that knows how to replace
@@ -175,6 +180,12 @@ class OwnershipReplaceSingleUseHelper {
175180

176181
/// Perform the actual RAUW.
177182
SILBasicBlock::iterator perform();
183+
184+
private:
185+
void invalidate() {
186+
ctx->clear();
187+
ctx = nullptr;
188+
}
178189
};
179190

180191
/// An abstraction over LoadInst/LoadBorrowInst so one can handle both types of

lib/SILOptimizer/Utils/OwnershipOptUtils.cpp

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -859,7 +859,7 @@ OwnershipRAUWHelper::OwnershipRAUWHelper(OwnershipFixupContext &inputCtx,
859859
// Otherwise, lets check if we can perform this RAUW operation. If we can't,
860860
// set ctx to nullptr to invalidate the helper and return.
861861
if (!canFixUpOwnershipForRAUW(oldValue, newValue, inputCtx)) {
862-
ctx = nullptr;
862+
invalidate();
863863
return;
864864
}
865865

@@ -908,16 +908,14 @@ OwnershipRAUWHelper::OwnershipRAUWHelper(OwnershipFixupContext &inputCtx,
908908
return;
909909

910910
if (!borrowedAddress.interiorPointerOp) {
911-
// Invalidate!
912-
ctx = nullptr;
911+
invalidate();
913912
return;
914913
}
915914

916915
ctx->extraAddressFixupInfo.intPtrOp = borrowedAddress.interiorPointerOp;
917916
auto borrowedValue = borrowedAddress.interiorPointerOp.getSingleBaseValue();
918917
if (!borrowedValue) {
919-
// Invalidate!
920-
ctx = nullptr;
918+
invalidate();
921919
return;
922920
}
923921

@@ -929,16 +927,15 @@ OwnershipRAUWHelper::OwnershipRAUWHelper(OwnershipFixupContext &inputCtx,
929927
// This cloner check must match the later cloner invocation in
930928
// replaceAddressUses()
931929
if (!canCloneUseDefChain(newValue, checkBase)) {
932-
ctx = nullptr;
930+
invalidate();
933931
return;
934932
}
935933

936934
// For now, just gather up uses
937935
auto &oldValueUses = ctx->extraAddressFixupInfo.allAddressUsesFromOldValue;
938936
if (InteriorPointerOperand::findTransitiveUsesForAddress(oldValue,
939937
oldValueUses)) {
940-
// If we found an error, invalidate and return!
941-
ctx = nullptr;
938+
invalidate();
942939
return;
943940
}
944941

@@ -1183,14 +1180,14 @@ OwnershipReplaceSingleUseHelper::OwnershipReplaceSingleUseHelper(
11831180

11841181
// If we have an address, bail. We don't support this.
11851182
if (newValue->getType().isAddress()) {
1186-
ctx = nullptr;
1183+
invalidate();
11871184
return;
11881185
}
11891186

11901187
// Otherwise, lets check if we can perform this RAUW operation. If we can't,
11911188
// set ctx to nullptr to invalidate the helper and return.
11921189
if (!hasValidRAUWOwnership(use->get(), newValue)) {
1193-
ctx = nullptr;
1190+
invalidate();
11941191
return;
11951192
}
11961193

test/SILOptimizer/cse_ossa_nontrivial.sil

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -859,3 +859,37 @@ bb0(%0 : @guaranteed $TestKlass):
859859
return %139 : $()
860860
}
861861

862+
struct X {
863+
var i: Int64
864+
}
865+
866+
sil_global hidden @globalX : $X
867+
868+
sil [ossa] @use_address : $@convention(thin) (@in_guaranteed X) -> ()
869+
870+
// CHECK-LABEL: sil [ossa] @cse_ossa_validrauwfollowinvalidrauw :
871+
// CHECK: global_addr
872+
// CHECK-NOT: global_addr
873+
// CHECK-LABEL: } // end sil function 'cse_ossa_validrauwfollowinvalidrauw'
874+
sil [ossa] @cse_ossa_validrauwfollowinvalidrauw : $@convention(thin) (@guaranteed TestKlass) -> () {
875+
bb0(%0 : @guaranteed $TestKlass):
876+
%1 = function_ref @use_address : $@convention(thin) (@in_guaranteed X) -> ()
877+
%2 = ref_element_addr %0 : $TestKlass, #TestKlass.testStruct
878+
%3 = begin_access [modify] [dynamic] %2 : $*NonTrivialStruct
879+
%4 = struct_element_addr %3 : $*NonTrivialStruct, #NonTrivialStruct.val
880+
%5 = load_borrow %4 : $*Klass
881+
%6 = function_ref @use_klass : $@convention(thin) (@guaranteed Klass) -> ()
882+
%7 = apply %6(%5) : $@convention(thin) (@guaranteed Klass) -> ()
883+
end_borrow %5 : $Klass
884+
%9 = global_addr @globalX : $*X
885+
%10 = apply %1(%9) : $@convention(thin) (@in_guaranteed X) -> ()
886+
%11 = struct_element_addr %3 : $*NonTrivialStruct, #NonTrivialStruct.val
887+
%12 = load_borrow %11 : $*Klass
888+
%13 = apply %6(%12) : $@convention(thin) (@guaranteed Klass) -> ()
889+
end_borrow %12 : $Klass
890+
%15 = global_addr @globalX : $*X
891+
%16 = apply %1(%15) : $@convention(thin) (@in_guaranteed X) -> ()
892+
end_access %3 : $*NonTrivialStruct
893+
%18 = tuple ()
894+
return %18 : $()
895+
}

0 commit comments

Comments
 (0)