Skip to content

Commit 31ad30e

Browse files
meg-guptameg-gupta
authored
Merge pull request swiftlang#79493 from meg-gupta/csesave
Avoid creating unoptimizable copies in CSE
2 parents f490e61 + 4561658 commit 31ad30e

File tree

4 files changed

+75
-10
lines changed

4 files changed

+75
-10
lines changed

‎include/swift/SILOptimizer/Utils/OwnershipOptUtils.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,8 @@ class OwnershipRAUWHelper {
279279
return ctx->extraAddressFixupInfo.base;
280280
}
281281

282+
bool mayIntroduceUnoptimizableCopies();
283+
282284
/// Perform OSSA fixup on newValue and return a fixed-up value based that can
283285
/// be used to replace all uses of oldValue.
284286
///

‎lib/SILOptimizer/Transforms/CSE.cpp

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1101,13 +1101,14 @@ bool CSE::processNode(DominanceInfoNode *Node) {
11011101
if (!isa<SingleValueInstruction>(Inst))
11021102
continue;
11031103

1104-
OwnershipRAUWHelper helper(RAUWFixupContext,
1105-
cast<SingleValueInstruction>(Inst),
1106-
cast<SingleValueInstruction>(AvailInst));
1104+
auto oldValue = cast<SingleValueInstruction>(Inst);
1105+
auto newValue = cast<SingleValueInstruction>(AvailInst);
1106+
OwnershipRAUWHelper helper(RAUWFixupContext, oldValue, newValue);
11071107
// If RAUW requires cloning the original, then there's no point. If it
11081108
// also requires introducing a copy and new borrow scope, then it's a
11091109
// very bad idea.
1110-
if (!helper.isValid() || helper.requiresCopyBorrowAndClone())
1110+
if (!helper.isValid() || helper.requiresCopyBorrowAndClone() ||
1111+
helper.mayIntroduceUnoptimizableCopies())
11111112
continue;
11121113
// Replace SingleValueInstruction using OSSA RAUW here
11131114
nextI = helper.perform();

‎lib/SILOptimizer/Utils/OwnershipOptUtils.cpp

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -439,6 +439,18 @@ static bool canFixUpOwnershipForRAUW(SILValue oldValue, SILValue newValue,
439439
}
440440
}
441441

442+
bool OwnershipRAUWHelper::mayIntroduceUnoptimizableCopies() {
443+
if (oldValue->getOwnershipKind() != OwnershipKind::Guaranteed) {
444+
return false;
445+
}
446+
447+
if (areUsesWithinValueLifetime(newValue, ctx->guaranteedUsePoints,
448+
&ctx->deBlocks)) {
449+
return false;
450+
}
451+
return true;
452+
}
453+
442454
bool swift::areUsesWithinLexicalValueLifetime(SILValue value,
443455
ArrayRef<Operand *> uses) {
444456
assert(value->isLexical());

‎test/SILOptimizer/cse_ossa_nontrivial.sil

Lines changed: 56 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -142,9 +142,11 @@ bb0(%0 : @owned $Klass, %1 : @owned $Klass):
142142
return %5 : $(Klass)
143143
}
144144

145+
// mayIntroduceUnoptimizableCopies returns true because swift::areUsesWithinValueLifetime returns false
146+
// since findOwnershipReferenceAggregate only looks through single operand forwarding instructions
145147
// CHECK-LABEL: sil [ossa] @tuple_test2 :
146148
// CHECK: tuple ({{%[0-9]+}} : $Klass, {{%[0-9]+}} : $Klass)
147-
// CHECK-NOT: tuple ({{%[0-9]+}} : $Klass, {{%[0-9]+}} : $Klass)
149+
// TODO-CHECK-NOT: tuple ({{%[0-9]+}} : $Klass, {{%[0-9]+}} : $Klass)
148150
// CHECK-LABEL: } // end sil function 'tuple_test2'
149151
sil [ossa] @tuple_test2 : $@convention(thin) (@guaranteed Klass, @guaranteed Klass) -> @owned (Klass) {
150152
bb0(%0 : @guaranteed $Klass, %1 : @guaranteed $Klass):
@@ -439,7 +441,7 @@ bb0(%0 : @guaranteed $(Klass, Klass)):
439441

440442
// CHECK-LABEL: sil [ossa] @struct_test2 :
441443
// CHECK: struct $NonTrivialStruct
442-
// CHECK-NOT: struct $NonTrivialStruct
444+
// TODO-CHECK-NOT: struct $NonTrivialStruct
443445
// CHECK-LABEL: } // end sil function 'struct_test2'
444446
sil [ossa] @struct_test2 : $@convention(thin) (@guaranteed Klass) -> @owned Klass {
445447
bb0(%0 : @guaranteed $Klass):
@@ -454,7 +456,7 @@ bb0(%0 : @guaranteed $Klass):
454456

455457
// CHECK-LABEL: sil [ossa] @struct_test3 :
456458
// CHECK: struct $NonTrivialStruct
457-
// CHECK-NOT: struct $NonTrivialStruct
459+
// TODO-CHECK-NOT: struct $NonTrivialStruct
458460
// CHECK-LABEL: } // end sil function 'struct_test3'
459461
sil [ossa] @struct_test3 : $@convention(thin) (@guaranteed Klass) -> () {
460462
bb0(%0 : @guaranteed $Klass):
@@ -504,7 +506,7 @@ bb0(%0 : @guaranteed $Klass):
504506

505507
// CHECK-LABEL: sil [ossa] @struct_extract_test1 :
506508
// CHECK: struct_extract
507-
// CHECK-NOT: struct_extract
509+
// TODO-CHECK-NOT: struct_extract
508510
// CHECK-LABEL: } // end sil function 'struct_extract_test1'
509511
sil [ossa] @struct_extract_test1 : $@convention(thin) (@owned NonTrivialStruct) -> @owned Klass {
510512
bb0(%0 : @owned $NonTrivialStruct):
@@ -644,9 +646,9 @@ bb3(%borrow : @guaranteed $Klass):
644646
// CHECK-LABEL: sil [ossa] @test_rauwfailsandthensucceeds :
645647
// CHECK: bb0
646648
// CHECK: struct_extract
647-
// CHECK: struct_extract
649+
// TODO-CHECK: struct_extract
648650
// CHECK: struct $_SliceBuffer
649-
// CHECK-NOT: struct $_SliceBuffer
651+
// TODO-CHECK-NOT: struct $_SliceBuffer
650652
// CHECK-LABEL: } // end sil function 'test_rauwfailsandthensucceeds'
651653
sil [ossa] @test_rauwfailsandthensucceeds : $@convention(method) <Element> (UnsafeMutablePointer<Element>, @guaranteed _ContiguousArrayBuffer<Element>, Int, UInt) -> @owned _SliceBuffer<Element> {
652654
bb0(%0 : $UnsafeMutablePointer<Element>, %1 : @guaranteed $_ContiguousArrayBuffer<Element>, %2 : $Int, %3 : $UInt):
@@ -1076,3 +1078,51 @@ bb0(%0 : $*Wrapper):
10761078
%18 = tuple ()
10771079
return %18
10781080
}
1081+
1082+
sil @get_nontrivialstruct : $@convention(thin) () -> @owned NonTrivialStruct
1083+
1084+
// Both CopyPropagation and CopyToBorrow cannot eliminate the copy generated by
1085+
// OSSA RAUW if we look through ownership instructions while CSE'ing
1086+
// CHECK-LABEL: sil [ossa] @struct_copy_test1 :
1087+
// CHECK: struct_extract
1088+
// CHECK: struct_extract
1089+
// CHECK-LABEL: } // end sil function 'struct_copy_test1'
1090+
sil [ossa] @struct_copy_test1 : $@convention(thin) (@owned NonTrivialStruct) -> () {
1091+
bb0(%0 : @owned $NonTrivialStruct):
1092+
%f = function_ref @use_klass : $@convention(thin) (@guaranteed Klass) -> ()
1093+
%b1 = begin_borrow %0
1094+
%ex1 = struct_extract %b1, #NonTrivialStruct.val
1095+
apply %f(%ex1) : $@convention(thin) (@guaranteed Klass) -> ()
1096+
end_borrow %b1
1097+
%b2 = begin_borrow %0
1098+
%ex2 = struct_extract %b2, #NonTrivialStruct.val
1099+
apply %f(%ex2) : $@convention(thin) (@guaranteed Klass) -> ()
1100+
end_borrow %b2
1101+
destroy_value %0 : $NonTrivialStruct
1102+
%res = tuple ()
1103+
return %res : $()
1104+
}
1105+
1106+
// Both CopyPropagation and CopyToBorrow cannot eliminate the copy generated by
1107+
// OSSA RAUW if we look through ownership instructions while CSE'ing
1108+
// CHECK-LABEL: sil [ossa] @struct_copy_test2 :
1109+
// CHECK: struct_extract
1110+
// CHECK: struct_extract
1111+
// CHECK-LABEL: } // end sil function 'struct_copy_test2'
1112+
sil [ossa] @struct_copy_test2 : $@convention(thin) () -> () {
1113+
bb0:
1114+
%f1 = function_ref @get_nontrivialstruct : $@convention(thin) () -> @owned NonTrivialStruct
1115+
%0 = apply %f1() : $@convention(thin) () -> @owned NonTrivialStruct
1116+
%f2 = function_ref @use_klass : $@convention(thin) (@guaranteed Klass) -> ()
1117+
%b1 = begin_borrow %0
1118+
%ex1 = struct_extract %b1, #NonTrivialStruct.val
1119+
apply %f2(%ex1) : $@convention(thin) (@guaranteed Klass) -> ()
1120+
end_borrow %b1
1121+
%b2 = begin_borrow %0
1122+
%ex2 = struct_extract %b2, #NonTrivialStruct.val
1123+
apply %f2(%ex2) : $@convention(thin) (@guaranteed Klass) -> ()
1124+
end_borrow %b2
1125+
destroy_value %0 : $NonTrivialStruct
1126+
%res = tuple ()
1127+
return %res : $()
1128+
}

0 commit comments

Comments
 (0)