Skip to content

Commit c4fe3c2

Browse files
gottesmmgottesmm
authored
Merge pull request #30088 from gottesmm/pr-3e6f27334eea2b48b5d8768445143250d4fa13d4
[temp-rvalue] Handle promoting alloc_stack that are destroyed via a load [take] in ossa.
2 parents 92f33f8 + 3a725c3 commit c4fe3c2

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)