Skip to content

Commit 3a725c3

Browse files
gottesmmgottesmm
committed
[temp-rvalue] Handle promoting alloc_stack that are destroyed via a load [take] in ossa.
There are a few interesting things here: 1. All of this code is conditionalized implicitly on ossa by us checking for load [take]. If we are in non-ossa, then we will have unqualified loads and this code path will be skipped. I left in the old test that made sure we did not support this in non-ossa code for this purpose. 2. We treat the load [take] like a destroy_addr. Thus the current method of providing safety via using lifetime analysis to show all "destroys" form a minimal post-dominating set for the copy_addr. 3. We do not allow for load [take] on sub-element initializations. The reason why is that SILGen initializes temporaries completely in memory and generally destroys addresses completely as well. Given that is the pattern we are looking for, we just reduce the state space by banning this pattern. 4. If we have a copy_addr [init], then we not only change the load [take] to have the original source address as an argument, but we also change the load [take] to a load [copy] so we don't lose the +1. Additionally, we have to hoist the load [copy] to the copy_addr [init] site to ensure that we do not insert a use-after-free from the retain happening too late. I am doing this to clean up some simple temp rvalue patterns in SIL before semantic arc opts runs. This ensures that we are more likely to have a load operation come from a more meaningful semantic entity (for instance a let field of a guaranteed class) and thus allow us to convert a load [copy] to a load_borrow. I am also going to add support in subsequent commits for eliminating alloc_stack that are stored into as well (another pattern I am seeing).
1 parent 7ac3338 commit 3a725c3

File tree

2 files changed

+172
-21
lines changed

2 files changed

+172
-21
lines changed

‎lib/SILOptimizer/Transforms/TempRValueElimination.cpp

Lines changed: 82 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,9 @@ class TempRValueOptPass : public SILFunctionTransform {
7070
checkNoSourceModification(CopyAddrInst *copyInst,
7171
const SmallPtrSetImpl<SILInstruction *> &useInsts);
7272

73-
bool checkTempObjectDestroy(AllocStackInst *tempObj, CopyAddrInst *copyInst);
73+
bool
74+
checkTempObjectDestroy(AllocStackInst *tempObj, CopyAddrInst *copyInst,
75+
ValueLifetimeAnalysis::Frontier &tempAddressFrontier);
7476

7577
bool tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst);
7678

@@ -189,6 +191,18 @@ bool TempRValueOptPass::collectLoads(
189191
case SILInstructionKind::LoadBorrowInst:
190192
// Loads are the end of the data flow chain. The users of the load can't
191193
// access the temporary storage.
194+
//
195+
// That being said, if we see a load [take] here then we must have had a
196+
// load [take] of a projection of our temporary stack location since we skip
197+
// all the load [take] of the top level allocation in the caller of this
198+
// function. So if we have such a load [take], we /must/ have a
199+
// reinitialization or an alloc_stack that does not fit the pattern we are
200+
// expecting from SILGen. Be conservative and return false.
201+
if (auto *li = dyn_cast<LoadInst>(user)) {
202+
if (li->getOwnershipQualifier() == LoadOwnershipQualifier::Take) {
203+
return false;
204+
}
205+
}
192206
loadInsts.insert(user);
193207
return true;
194208

@@ -250,8 +264,9 @@ bool TempRValueOptPass::checkNoSourceModification(
250264
/// it is legal to destroy an in-memory object by loading the value and
251265
/// releasing it. Rather than detecting unbalanced load releases, simply check
252266
/// that tempObj is destroyed directly on all paths.
253-
bool TempRValueOptPass::checkTempObjectDestroy(AllocStackInst *tempObj,
254-
CopyAddrInst *copyInst) {
267+
bool TempRValueOptPass::checkTempObjectDestroy(
268+
AllocStackInst *tempObj, CopyAddrInst *copyInst,
269+
ValueLifetimeAnalysis::Frontier &tempAddressFrontier) {
255270
// If the original copy was a take, then replacing all uses cannot affect
256271
// the lifetime.
257272
if (copyInst->isTakeOfSrc())
@@ -275,7 +290,6 @@ bool TempRValueOptPass::checkTempObjectDestroy(AllocStackInst *tempObj,
275290
}
276291
// Find the boundary of tempObj's address lifetime, starting at copyInst.
277292
ValueLifetimeAnalysis vla(copyInst, users);
278-
ValueLifetimeAnalysis::Frontier tempAddressFrontier;
279293
if (!vla.computeFrontier(tempAddressFrontier,
280294
ValueLifetimeAnalysis::DontModifyCFG)) {
281295
return false;
@@ -289,13 +303,19 @@ bool TempRValueOptPass::checkTempObjectDestroy(AllocStackInst *tempObj,
289303
if (pos == frontierInst->getParent()->begin())
290304
return false;
291305

292-
// Look for a known destroy point as described in the funciton level
306+
// Look for a known destroy point as described in the function level
293307
// comment. This whitelist can be expanded as more cases are handled in
294308
// tryOptimizeCopyIntoTemp during copy replacement.
295309
SILInstruction *lastUser = &*std::prev(pos);
296310
if (isa<DestroyAddrInst>(lastUser))
297311
continue;
298312

313+
if (auto *li = dyn_cast<LoadInst>(lastUser)) {
314+
if (li->getOwnershipQualifier() == LoadOwnershipQualifier::Take) {
315+
continue;
316+
}
317+
}
318+
299319
if (auto *cai = dyn_cast<CopyAddrInst>(lastUser)) {
300320
assert(cai->getSrc() == tempObj && "collectLoads checks for writes");
301321
assert(!copyInst->isTakeOfSrc() && "checked above");
@@ -334,6 +354,15 @@ bool TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) {
334354
if (isa<DestroyAddrInst>(user) || isa<DeallocStackInst>(user))
335355
continue;
336356

357+
// Same for load [take] on the top level temp object. SILGen always takes
358+
// whole values from temporaries. If we have load [take] on projections from
359+
// our base, we fail since those would be re-initializations.
360+
if (auto *li = dyn_cast<LoadInst>(user)) {
361+
if (li->getOwnershipQualifier() == LoadOwnershipQualifier::Take) {
362+
continue;
363+
}
364+
}
365+
337366
if (!collectLoads(useOper, user, tempObj, copyInst->getSrc(), loadInsts))
338367
return false;
339368
}
@@ -342,7 +371,8 @@ bool TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) {
342371
if (!checkNoSourceModification(copyInst, loadInsts))
343372
return false;
344373

345-
if (!checkTempObjectDestroy(tempObj, copyInst))
374+
ValueLifetimeAnalysis::Frontier tempAddressFrontier;
375+
if (!checkTempObjectDestroy(tempObj, copyInst, tempAddressFrontier))
346376
return false;
347377

348378
LLVM_DEBUG(llvm::dbgs() << " Success: replace temp" << *tempObj);
@@ -351,6 +381,10 @@ bool TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) {
351381
// the source address. Note: we must not delete the original copyInst because
352382
// it would crash the instruction iteration in run(). Instead the copyInst
353383
// gets identical Src and Dest operands.
384+
//
385+
// NOTE: We delete instructions at the end to allow us to use
386+
// tempAddressFrontier to insert compensating destroys for load [take].
387+
SmallVector<SILInstruction *, 4> toDelete;
354388
while (!tempObj->use_empty()) {
355389
Operand *use = *tempObj->use_begin();
356390
SILInstruction *user = use->getUser();
@@ -359,11 +393,13 @@ bool TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) {
359393
if (copyInst->isTakeOfSrc()) {
360394
use->set(copyInst->getSrc());
361395
} else {
362-
user->eraseFromParent();
396+
user->dropAllReferences();
397+
toDelete.push_back(user);
363398
}
364399
break;
365400
case SILInstructionKind::DeallocStackInst:
366-
user->eraseFromParent();
401+
user->dropAllReferences();
402+
toDelete.push_back(user);
367403
break;
368404
case SILInstructionKind::CopyAddrInst: {
369405
auto *cai = cast<CopyAddrInst>(user);
@@ -375,6 +411,40 @@ bool TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) {
375411
use->set(copyInst->getSrc());
376412
break;
377413
}
414+
case SILInstructionKind::LoadInst: {
415+
// If we do not have a load [take] or we have a load [take] and our
416+
// copy_addr takes the source, just do the normal thing of setting the
417+
// load to use the copyInst's source.
418+
auto *li = cast<LoadInst>(user);
419+
if (li->getOwnershipQualifier() != LoadOwnershipQualifier::Take ||
420+
copyInst->isTakeOfSrc()) {
421+
use->set(copyInst->getSrc());
422+
break;
423+
}
424+
425+
// Otherwise, since copy_addr is not taking src, we need to ensure that we
426+
// insert a copy of our value. We do that by creating a load [copy] at the
427+
// copy_addr inst and RAUWing the load [take] with that. We then insert
428+
// destroy_value for the load [copy] at all points where we had destroys
429+
// that are not the specific take that we were optimizing.
430+
SILBuilderWithScope builder(copyInst);
431+
SILValue newLoad = builder.emitLoadValueOperation(
432+
copyInst->getLoc(), copyInst->getSrc(), LoadOwnershipQualifier::Copy);
433+
for (auto *inst : tempAddressFrontier) {
434+
assert(inst->getIterator() != inst->getParent()->begin() &&
435+
"Should have caught this when checking destructor");
436+
auto prevInst = std::prev(inst->getIterator());
437+
if (&*prevInst == li)
438+
continue;
439+
SILBuilderWithScope builder(prevInst);
440+
builder.emitDestroyValueOperation(prevInst->getLoc(), newLoad);
441+
}
442+
li->replaceAllUsesWith(newLoad);
443+
li->dropAllReferences();
444+
toDelete.push_back(li);
445+
break;
446+
}
447+
378448
// ASSUMPTION: no operations that may be handled by this default clause can
379449
// destroy tempObj. This includes operations that load the value from memory
380450
// and release it.
@@ -383,6 +453,10 @@ bool TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) {
383453
break;
384454
}
385455
}
456+
457+
while (!toDelete.empty()) {
458+
toDelete.pop_back_val()->eraseFromParent();
459+
}
386460
tempObj->eraseFromParent();
387461
return true;
388462
}

‎test/SILOptimizer/temp_rvalue_opt_ossa.sil

Lines changed: 90 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ class Klass {}
1818

1919
sil @unknown : $@convention(thin) () -> ()
2020

21+
sil @use_gsbase_builtinnativeobject : $@convention(thin) (@guaranteed GS<Builtin.NativeObject>) -> ()
22+
2123
sil [ossa] @inguaranteed_user_without_result : $@convention(thin) (@in_guaranteed Klass) -> () {
2224
bb0(%0 : $*Klass):
2325
%9999 = tuple()
@@ -604,25 +606,17 @@ bb3:
604606
br bb2
605607
}
606608

607-
///////////////////////////////////////////////////////////////////////////////
608-
// Test checkTempObjectDestroy
609-
// <rdar://problem/56393491> Use-after free crashing an XCTest.
610-
611609
sil [ossa] @takeGuaranteedObj : $@convention(thin) (@guaranteed Builtin.NativeObject) -> ()
612610

613-
// Do not remove a copy that is released via a load (because
614-
// TempRValueOpt is an address-based optimization does not know how to
615-
// remove releases, and trying to do that would reduce to ARC
611+
// Now that we support ossa, eliminate the alloc_stack and change the load
612+
// [take] to a load [copy] in the process.
616613
//
617-
// optimization).
618614
// CHECK-LABEL: sil [ossa] @copyWithLoadRelease : $@convention(thin) (@in_guaranteed Builtin.NativeObject) -> () {
619615
// CHECK: bb0(%0 : $*Builtin.NativeObject):
620-
// CHECK: [[STK:%.*]] = alloc_stack $Builtin.NativeObject
621-
// CHECK: copy_addr %0 to [initialization] [[STK]] : $*Builtin.NativeObject
622-
// CHECK: [[VAL:%.*]] = load [take] [[STK]] : $*Builtin.NativeObject
616+
// CHECK-NOT: alloc_stack
617+
// CHECK: [[VAL:%.*]] = load [copy] %0 : $*Builtin.NativeObject
623618
// CHECK: apply %{{.*}}([[VAL]]) : $@convention(thin) (@guaranteed Builtin.NativeObject) -> ()
624619
// CHECK: destroy_value [[VAL]] : $Builtin.NativeObject
625-
// CHECK: dealloc_stack [[STK]] : $*Builtin.NativeObject
626620
// CHECK-LABEL: } // end sil function 'copyWithLoadRelease'
627621
sil [ossa] @copyWithLoadRelease : $@convention(thin) (@in_guaranteed Builtin.NativeObject) -> () {
628622
bb0(%0 : $*Builtin.NativeObject):
@@ -637,7 +631,8 @@ bb0(%0 : $*Builtin.NativeObject):
637631
return %v : $()
638632
}
639633

640-
// Remove a copy that is released via a load as long as it was a copy [take].
634+
// Remove a copy that is released via a load. Leave the load [take] alone since
635+
// our copy_addr is taking from source.
641636
//
642637
// CHECK-LABEL: sil [ossa] @takeWithLoadRelease : $@convention(thin) (@in Builtin.NativeObject) -> () {
643638
// CHECK: bb0(%0 : $*Builtin.NativeObject):
@@ -657,3 +652,85 @@ bb0(%0 : $*Builtin.NativeObject):
657652
%v = tuple ()
658653
return %v : $()
659654
}
655+
656+
// Do not remove a copy that is released via a load of a projection. This is not
657+
// the pattern from SILGen that we are targeting, so we reduce the state space by banning the pattern.
658+
//
659+
// CHECK-LABEL: sil [ossa] @takeWithLoadReleaseOfProjection : $@convention(thin) (@in GS<Builtin.NativeObject>) -> () {
660+
// CHECK: alloc_stack
661+
// CHECK: } // end sil function 'takeWithLoadReleaseOfProjection'
662+
sil [ossa] @takeWithLoadReleaseOfProjection : $@convention(thin) (@in GS<Builtin.NativeObject>) -> () {
663+
bb0(%0 : $*GS<Builtin.NativeObject>):
664+
%stk = alloc_stack $GS<Builtin.NativeObject>
665+
copy_addr [take] %0 to [initialization] %stk : $*GS<Builtin.NativeObject>
666+
%proj = struct_element_addr %stk : $*GS<Builtin.NativeObject>, #GS._base
667+
%obj = load [take] %proj : $*Builtin.NativeObject
668+
%f = function_ref @takeGuaranteedObj : $@convention(thin) (@guaranteed Builtin.NativeObject) -> ()
669+
%call = apply %f(%obj) : $@convention(thin) (@guaranteed Builtin.NativeObject) -> ()
670+
destroy_value %obj : $Builtin.NativeObject
671+
dealloc_stack %stk : $*GS<Builtin.NativeObject>
672+
%v = tuple ()
673+
return %v : $()
674+
}
675+
676+
// Make sure that when we convert the load [take] to a load [copy], we hoist the
677+
// load of src /before/ the destroy of %0.
678+
// CHECK-LABEL: sil [ossa] @hoist_load_copy_to_src_copy_addr_site : $@convention(thin) (@in GS<Builtin.NativeObject>) -> @owned GS<Builtin.NativeObject> {
679+
// CHECK: bb0([[ARG:%.*]] :
680+
// CHECK: [[VAL:%.*]] = load [copy] [[ARG]]
681+
// CHECK: destroy_addr [[ARG]]
682+
// CHECK: apply {{%.*}}([[VAL]])
683+
// CHECK: return [[VAL]]
684+
// CHECK: } // end sil function 'hoist_load_copy_to_src_copy_addr_site'
685+
sil [ossa] @hoist_load_copy_to_src_copy_addr_site : $@convention(thin) (@in GS<Builtin.NativeObject>) -> @owned GS<Builtin.NativeObject> {
686+
bb0(%0 : $*GS<Builtin.NativeObject>):
687+
%f = function_ref @use_gsbase_builtinnativeobject : $@convention(thin) (@guaranteed GS<Builtin.NativeObject>) -> ()
688+
%stk = alloc_stack $GS<Builtin.NativeObject>
689+
copy_addr %0 to [initialization] %stk : $*GS<Builtin.NativeObject>
690+
destroy_addr %0 : $*GS<Builtin.NativeObject>
691+
%obj = load [take] %stk : $*GS<Builtin.NativeObject>
692+
dealloc_stack %stk : $*GS<Builtin.NativeObject>
693+
apply %f(%obj) : $@convention(thin) (@guaranteed GS<Builtin.NativeObject>) -> ()
694+
return %obj : $GS<Builtin.NativeObject>
695+
}
696+
697+
// CHECK-LABEL: sil [ossa] @hoist_load_copy_to_src_copy_addr_site_two_takes : $@convention(thin) (@in GS<Builtin.NativeObject>) -> @owned GS<Builtin.NativeObject> {
698+
// CHECK: bb0([[ARG:%.*]] :
699+
// CHECK: [[COPY_1:%.*]] = load [copy] [[ARG]]
700+
// CHECK: [[COPY_2:%.*]] = load [copy] [[ARG]]
701+
// CHECK: destroy_addr [[ARG]]
702+
// CHECK: cond_br undef, bb1, bb2
703+
//
704+
// CHECK: bb1:
705+
// CHECK: destroy_value [[COPY_1]]
706+
// CHECK: br bb3([[COPY_2]] :
707+
//
708+
// CHECK: bb2:
709+
// CHECK: destroy_value [[COPY_2]]
710+
// CHECK: br bb3([[COPY_1]] :
711+
//
712+
// CHECK: bb3([[RESULT:%.*]] : @owned
713+
// CHECK: apply {{%.*}}([[RESULT]])
714+
// CHECK: return [[RESULT]]
715+
// CHECK: } // end sil function 'hoist_load_copy_to_src_copy_addr_site_two_takes'
716+
sil [ossa] @hoist_load_copy_to_src_copy_addr_site_two_takes : $@convention(thin) (@in GS<Builtin.NativeObject>) -> @owned GS<Builtin.NativeObject> {
717+
bb0(%0 : $*GS<Builtin.NativeObject>):
718+
%f = function_ref @use_gsbase_builtinnativeobject : $@convention(thin) (@guaranteed GS<Builtin.NativeObject>) -> ()
719+
%stk = alloc_stack $GS<Builtin.NativeObject>
720+
copy_addr %0 to [initialization] %stk : $*GS<Builtin.NativeObject>
721+
destroy_addr %0 : $*GS<Builtin.NativeObject>
722+
cond_br undef, bb1, bb2
723+
724+
bb1:
725+
%obj = load [take] %stk : $*GS<Builtin.NativeObject>
726+
br bb3(%obj : $GS<Builtin.NativeObject>)
727+
728+
bb2:
729+
%obj2 = load [take] %stk : $*GS<Builtin.NativeObject>
730+
br bb3(%obj2 : $GS<Builtin.NativeObject>)
731+
732+
bb3(%obj3 : @owned $GS<Builtin.NativeObject>):
733+
dealloc_stack %stk : $*GS<Builtin.NativeObject>
734+
apply %f(%obj3) : $@convention(thin) (@guaranteed GS<Builtin.NativeObject>) -> ()
735+
return %obj3 : $GS<Builtin.NativeObject>
736+
}

0 commit comments

Comments
 (0)