[TempRValueOpt] Fold added destroy_addrs into loads/copy_addrs. #65052
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -535,10 +535,11 @@ void TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) { | |
| SILValue copySrc = copyInst->getSrc(); | ||
| assert(tempObj != copySrc && "can't initialize temporary with itself"); | ||
|
|
||
| // If the source of the copyInst is taken, it must be deinitialized (via | ||
| // destroy_addr, load [take], copy_addr [take]). This must be done at the | ||
| // right spot: after the last use tempObj, but before any (potential) | ||
| // re-initialization of the source. | ||
| bool needFinalDeinit = copyInst->isTakeOfSrc(); | ||
|
|
||
| // Scan all uses of the temporary storage (tempObj) to verify they all refer | ||
| // to the value initialized by this copy. It is sufficient to check that the | ||
|
|
@@ -557,7 +558,7 @@ void TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) { | |
|
|
||
| // Also, destroys are allowed to be in a different block. | ||
| if (isa<DestroyAddrInst>(user)) { | ||
| if (!isOSSA && needFinalDeinit) { | ||
| // In non-OSSA mode, for the purpose of inserting the destroy of | ||
| // copySrc, we have to be conservative and assume that the lifetime of | ||
| // tempObj goes beyond it's last use - until the final destroy_addr. | ||
|
|
@@ -588,7 +589,7 @@ void TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) { | |
| // Example: | ||
| // copy_addr [take] %copySrc to [init] %tempObj // copyInst | ||
| // copy_addr [take] %tempObj to [init] %copySrc // lastLoadInst | ||
| if (needFinalDeinit && lastLoadInst != copyInst && | ||
| !isa<DestroyAddrInst>(lastLoadInst) && | ||
| aa->mayWriteToMemory(lastLoadInst, copySrc)) | ||
| return; | ||
|
|
@@ -601,6 +602,34 @@ void TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) { | |
|
|
||
| LLVM_DEBUG(llvm::dbgs() << " Success: replace temp" << *tempObj); | ||
|
|
||
| // If copyInst's source must be deinitialized, whether that must be done via | ||
| // a newly created destroy_addr. | ||
| // | ||
| // If lastLoadInst is a load or a copy_addr, then the deinitialization can be | ||
| // done in that instruction. | ||
| // | ||
| // This is necessary for correctness: otherwise, copies of move-only values | ||
| // would be introduced. | ||
| bool needToInsertDestroy = [&]() { | ||
| if (!needFinalDeinit) | ||
| return false; | ||
| if (lastLoadInst == copyInst) | ||
| return true; | ||
| if (auto *cai = dyn_cast<CopyAddrInst>(lastLoadInst)) { | ||
| auto retval = cai->getSrc() != tempObj || !cai->isTakeOfSrc(); | ||
| assert(!tempObj->getType().isMoveOnly() || | ||
| !retval && "introducing copy of move-only value!?"); | ||
| return retval; | ||
| } | ||
| if (auto *li = dyn_cast<LoadInst>(lastLoadInst)) { | ||
| auto retval = li->getOperand() != tempObj || | ||
| li->getOwnershipQualifier() != LoadOwnershipQualifier::Take; | ||
| assert(!tempObj->getType().isMoveOnly() || | ||
| !retval && "introducing copy of move-only value!?"); | ||
| return retval; | ||
| } | ||
| return true; | ||
| }(); | ||
|
There was a problem hiding this comment. Maybe you can add an assert here to check if it's a move only type then There was a problem hiding this comment. Added asserts along those lines in 1021519 . (Along with a test illustrating why |
||
| if (needToInsertDestroy) { | ||
| // Compensate the [take] of the original copyInst. | ||
| SILBuilderWithScope::insertAfter(lastLoadInst, [&] (SILBuilder &builder) { | ||
|
|
@@ -630,16 +659,19 @@ void TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) { | |
| auto *cai = cast<CopyAddrInst>(user); | ||
| if (cai != copyInst) { | ||
| assert(cai->getSrc() == tempObj); | ||
| if (cai->isTakeOfSrc() && (!needFinalDeinit || lastLoadInst != cai)) { | ||
| cai->setIsTakeOfSrc(IsNotTake); | ||
| } | ||
| } | ||
| use->set(copySrc); | ||
| break; | ||
| } | ||
| case SILInstructionKind::LoadInst: { | ||
| auto *li = cast<LoadInst>(user); | ||
| if (li->getOwnershipQualifier() == LoadOwnershipQualifier::Take && | ||
| (!needFinalDeinit || li != lastLoadInst)) { | ||
| li->setOwnershipQualifier(LoadOwnershipQualifier::Copy); | ||
| } | ||
| use->set(copySrc); | ||
| break; | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: all these conditions are getting hard to understand. IMO, for clarity it's better to write
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#65099