Skip to content

[move-only] Make sure that we convert copy_value/load [copy] move only to their after error variant if we error early. #62631

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 16, 2022
Merged

[move-only] Make sure that we convert copy_value/load [copy] move only to their after error variant if we error early. #62631

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 59 additions & 39 deletions lib/SILOptimizer/Mandatory/MoveOnlyAddressChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,17 @@ struct MoveOnlyChecker {
/// [noimplicitcopy]. If we find one that does not fit a pattern that we
/// understand, emit an error diagnostic telling the programmer that the move
/// checker did not know how to recognize this code pattern.
void searchForCandidateMarkMustChecks();
///
/// Returns true if we emitted a diagnostic. Returns false otherwise.
bool searchForCandidateMarkMustChecks();

/// After we have emitted a diagnostic, we need to clean up the instruction
/// stream by converting /all/ copies of move only typed things to use
/// explicit_copy_value so that we maintain the SIL invariant that in
/// canonical SIL move only types are not copied by normal copies.
///
/// Returns true if we actually changed any instructions.
void cleanupAfterEmittingDiagnostic();

/// Emits an error diagnostic for \p markedValue.
void performObjectCheck(MarkMustCheckInst *markedValue);
Expand Down Expand Up @@ -1334,7 +1344,44 @@ bool GlobalDataflow::compute() {
// MARK: Main Pass Implementation
//===----------------------------------------------------------------------===//

void MoveOnlyChecker::searchForCandidateMarkMustChecks() {
void MoveOnlyChecker::cleanupAfterEmittingDiagnostic() {
for (auto &block : *fn) {
for (auto ii = block.begin(), ie = block.end(); ii != ie;) {
auto *inst = &*ii;
++ii;

// Convert load [copy] -> load_borrow + explicit_copy_value.
if (auto *li = dyn_cast<LoadInst>(inst)) {
if (li->getOwnershipQualifier() == LoadOwnershipQualifier::Copy) {
SILBuilderWithScope builder(li);
auto *lbi = builder.createLoadBorrow(li->getLoc(), li->getOperand());
auto *cvi = builder.createExplicitCopyValue(li->getLoc(), lbi);
builder.createEndBorrow(li->getLoc(), lbi);
li->replaceAllUsesWith(cvi);
li->eraseFromParent();
changed = true;
}
}

// Convert copy_addr !take of src to its explicit value form so we don't
// error.
if (auto *copyAddr = dyn_cast<CopyAddrInst>(inst)) {
if (!copyAddr->isTakeOfSrc()) {
SILBuilderWithScope builder(copyAddr);
builder.createExplicitCopyAddr(
copyAddr->getLoc(), copyAddr->getSrc(), copyAddr->getDest(),
IsTake_t(copyAddr->isTakeOfSrc()),
IsInitialization_t(copyAddr->isInitializationOfDest()));
copyAddr->eraseFromParent();
changed = true;
}
}
}
}
}

bool MoveOnlyChecker::searchForCandidateMarkMustChecks() {
bool emittedDiagnostic = false;
for (auto &block : *fn) {
for (auto ii = block.begin(), ie = block.end(); ii != ie;) {
auto *mmci = dyn_cast<MarkMustCheckInst>(&*ii);
Expand All @@ -1351,6 +1398,7 @@ void MoveOnlyChecker::searchForCandidateMarkMustChecks() {
llvm::dbgs()
<< "Early emitting diagnostic for unsupported alloc box!\n");
diagnosticEmitter.emitCheckerDoesntUnderstandDiagnostic(mmci);
emittedDiagnostic = true;
continue;
}

Expand All @@ -1360,6 +1408,7 @@ void MoveOnlyChecker::searchForCandidateMarkMustChecks() {
llvm::dbgs()
<< "Early emitting diagnostic for unsupported alloc box!\n");
diagnosticEmitter.emitCheckerDoesntUnderstandDiagnostic(mmci);
emittedDiagnostic = true;
continue;
}
}
Expand All @@ -1368,6 +1417,7 @@ void MoveOnlyChecker::searchForCandidateMarkMustChecks() {
moveIntroducersToProcess.insert(mmci);
}
}
return emittedDiagnostic;
}

bool MoveOnlyChecker::performSingleCheck(MarkMustCheckInst *markedAddress) {
Expand Down Expand Up @@ -1566,15 +1616,18 @@ bool MoveOnlyChecker::performSingleCheck(MarkMustCheckInst *markedAddress) {
bool MoveOnlyChecker::check() {
// First search for candidates to process and emit diagnostics on any
// mark_must_check [noimplicitcopy] we didn't recognize.
searchForCandidateMarkMustChecks();
bool emittedDiagnostic = searchForCandidateMarkMustChecks();

// If we didn't find any introducers to check, just return changed.
//
// NOTE: changed /can/ be true here if we had any mark_must_check
// [noimplicitcopy] that we didn't understand and emitting a diagnostic upon
// and then deleting.
if (moveIntroducersToProcess.empty())
if (moveIntroducersToProcess.empty()) {
if (emittedDiagnostic)
cleanupAfterEmittingDiagnostic();
return changed;
}

for (auto *markedValue : moveIntroducersToProcess) {
LLVM_DEBUG(llvm::dbgs() << "Visiting: " << *markedValue);
Expand All @@ -1587,7 +1640,7 @@ bool MoveOnlyChecker::check() {
diagnosticEmitter.emitCheckerDoesntUnderstandDiagnostic(markedValue);
}
}
bool emittedDiagnostic = diagnosticEmitter.emittedAnyDiagnostics();
emittedDiagnostic = diagnosticEmitter.emittedAnyDiagnostics();

// Ok, now that we have performed our checks, we need to eliminate all mark
// must check inst since it is invalid for these to be in canonical SIL and
Expand All @@ -1608,40 +1661,7 @@ bool MoveOnlyChecker::check() {
// It is also ok that we use a little more compile time and go over the
// function again, since we are going to fail the compilation and not codegen.
if (emittedDiagnostic) {
for (auto &block : *fn) {
for (auto ii = block.begin(), ie = block.end(); ii != ie;) {
auto *inst = &*ii;
++ii;

// Convert load [copy] -> load_borrow + explicit_copy_value.
if (auto *li = dyn_cast<LoadInst>(inst)) {
if (li->getOwnershipQualifier() == LoadOwnershipQualifier::Copy) {
SILBuilderWithScope builder(li);
auto *lbi =
builder.createLoadBorrow(li->getLoc(), li->getOperand());
auto *cvi = builder.createExplicitCopyValue(li->getLoc(), lbi);
builder.createEndBorrow(li->getLoc(), lbi);
li->replaceAllUsesWith(cvi);
li->eraseFromParent();
changed = true;
}
}

// Convert copy_addr !take of src to its explicit value form so we don't
// error.
if (auto *copyAddr = dyn_cast<CopyAddrInst>(inst)) {
if (!copyAddr->isTakeOfSrc()) {
SILBuilderWithScope builder(copyAddr);
builder.createExplicitCopyAddr(
copyAddr->getLoc(), copyAddr->getSrc(), copyAddr->getDest(),
IsTake_t(copyAddr->isTakeOfSrc()),
IsInitialization_t(copyAddr->isInitializationOfDest()));
copyAddr->eraseFromParent();
changed = true;
}
}
}
}
cleanupAfterEmittingDiagnostic();
}

return changed;
Expand Down
71 changes: 46 additions & 25 deletions lib/SILOptimizer/Mandatory/MoveOnlyObjectChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,39 @@ struct MoveOnlyChecker {

bool check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
DominanceInfo *domTree);

/// After we have emitted a diagnostic, we need to clean up the instruction
/// stream by converting /all/ copies of move only typed things to use
/// explicit_copy_value so that we maintain the SIL invariant that in
/// canonical SIL move only types are not copied by normal copies.
///
/// Returns true if we actually changed any instructions.
bool cleanupAfterEmittingDiagnostic();
};

} // namespace

bool MoveOnlyChecker::cleanupAfterEmittingDiagnostic() {
bool changed = false;
for (auto &block : *fn) {
for (auto ii = block.begin(), ie = block.end(); ii != ie;) {
auto *cvi = dyn_cast<CopyValueInst>(&*ii);
++ii;

if (!cvi || !cvi->getOperand()->getType().isMoveOnlyWrapped())
continue;

SILBuilderWithScope b(cvi);
auto *expCopy =
b.createExplicitCopyValue(cvi->getLoc(), cvi->getOperand());
cvi->replaceAllUsesWith(expCopy);
cvi->eraseFromParent();
changed = true;
}
}
return changed;
}

bool MoveOnlyChecker::searchForCandidateMarkMustChecks(
DiagnosticEmitter &emitter) {
bool changed = false;
Expand Down Expand Up @@ -296,15 +325,22 @@ bool MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,

// First search for candidates to process and emit diagnostics on any
// mark_must_check [noimplicitcopy] we didn't recognize.
changed |= searchForCandidateMarkMustChecks(diagnosticEmitter);
bool emittedDiagnostic = searchForCandidateMarkMustChecks(diagnosticEmitter);

// If we didn't find any introducers to check, just return changed.
// If we didn't find any introducers to check, just return if we emitted an
// error (which is the only way we emitted a change to the instruction
// stream).
//
// NOTE: changed /can/ be true here if we had any mark_must_check
// [noimplicitcopy] that we didn't understand and emitting a diagnostic upon
// and then deleting.
if (moveIntroducersToProcess.empty())
return changed;
if (moveIntroducersToProcess.empty()) {
if (emittedDiagnostic) {
cleanupAfterEmittingDiagnostic();
return true;
}
return false;
}

auto moveIntroducers = llvm::makeArrayRef(moveIntroducersToProcess.begin(),
moveIntroducersToProcess.end());
Expand Down Expand Up @@ -345,7 +381,7 @@ bool MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
diagnosticEmitter.emitObjectOwnedDiagnostic(markedValue);
}

bool emittedDiagnostic = diagnosticEmitter.emittedAnyDiagnostics();
emittedDiagnostic = diagnosticEmitter.emittedAnyDiagnostics();

// Ok, we have success. All of our marker instructions were proven as safe or
// we emitted a diagnostic. Now we need to clean up the IR by eliminating our
Expand Down Expand Up @@ -390,30 +426,15 @@ bool MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
}

// Once we have finished processing, if we emitted any diagnostics, then we
// may have copy_value of @moveOnly typed values. This is not valid in
// Canonical SIL, so we need to ensure that those copy_value become
// explicit_copy_value. This is ok to do since we are already going to fail
// the compilation and just are trying to maintain SIL invariants.
// may have copy_value of move only and @moveOnly wrapped type values. This is
// not valid in Canonical SIL, so we need to ensure that those copy_value
// become explicit_copy_value. This is ok to do since we are already going to
// fail the compilation and just are trying to maintain SIL invariants.
//
// It is also ok that we use a little more compile time and go over the
// function again, since we are going to fail the compilation and not codegen.
if (emittedDiagnostic) {
for (auto &block : *fn) {
for (auto ii = block.begin(), ie = block.end(); ii != ie;) {
auto *cvi = dyn_cast<CopyValueInst>(&*ii);
++ii;

if (!cvi || !cvi->getOperand()->getType().isMoveOnlyWrapped())
continue;

SILBuilderWithScope b(cvi);
auto *expCopy =
b.createExplicitCopyValue(cvi->getLoc(), cvi->getOperand());
cvi->replaceAllUsesWith(expCopy);
cvi->eraseFromParent();
changed = true;
}
}
changed |= cleanupAfterEmittingDiagnostic();
}

return changed;
Expand Down