Skip to content

Commit cb1ed89

Browse files
committed
[NFC] Add support for a mandatory-copy-propagation pass.
It is currently disabled so this commit is NFC. MandatoryCopyPropagation canonicalizes all all OSSA lifetimes with either CopyValue or DestroyValue operations. While regular CopyPropagation only canonicalizes lifetimes with copies. This ensures that more lifetime program bugs are found in debug builds. Eventually, regular CopyPropagation will also canonicalize all lifetimes, but for now, we don't want to expose optimized code to more behavior change than necessary. Add frontend flags for developers to easily control copy propagation: -enable-copy-propagation: enables whatever form of copy propagation the current pipeline runs (mandatory-copy-propagation at -Onone, regular copy-propation at -O). -disable-copy-propagation: similarly disables any form of copy propagation in the current pipelien. To control a specific variant of the passes, use -Xllvm -disable-pass=mandatory-copy-propagation or -Xllvm -disable-pass=copy-propagation instead. The meaning of these flags will stay the same as we adjust the defaults. Soon mandatory-copy-propagation will be enabled by default. There are two reasons to do this, both related to predictable behavior across Debug and Release builds. 1. Shortening object lifetimes can cause observable changes in program behavior in the presense of weak/unowned reference and deinitializer side effects. 2. Programmers need to know reliably whether a given code pattern will copy the storage for copy-on-write types (Array, Set). Eliminating the "unexpected" copies the same way at -Onone and -O both makes debugging tractable and provides assurance that the code isn't relying on the luck of the optimizer in a particular compiler release.
1 parent 748fe46 commit cb1ed89

File tree

4 files changed

+28
-12
lines changed

4 files changed

+28
-12
lines changed

include/swift/SILOptimizer/Utils/CanonicalOSSALifetime.h

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,7 @@ class CanonicalizeOSSALifetime {
187187
private:
188188
/// If true, then debug_value instructions outside of non-debug
189189
/// liveness may be pruned during canonicalization.
190-
bool pruneDebug;
190+
bool pruneDebugMode;
191191

192192
NonLocalAccessBlockAnalysis *accessBlockAnalysis;
193193
// Lazily initialize accessBlocks only when
@@ -237,12 +237,13 @@ class CanonicalizeOSSALifetime {
237237
CanonicalOSSAConsumeInfo consumes;
238238

239239
public:
240-
CanonicalizeOSSALifetime(bool pruneDebug,
240+
CanonicalizeOSSALifetime(bool pruneDebugMode,
241241
NonLocalAccessBlockAnalysis *accessBlockAnalysis,
242242
DominanceAnalysis *dominanceAnalysis,
243243
DeadEndBlocks *deBlocks)
244-
: pruneDebug(pruneDebug), accessBlockAnalysis(accessBlockAnalysis),
245-
dominanceAnalysis(dominanceAnalysis), deBlocks(deBlocks) {}
244+
: pruneDebugMode(pruneDebugMode),
245+
accessBlockAnalysis(accessBlockAnalysis),
246+
dominanceAnalysis(dominanceAnalysis), deBlocks(deBlocks) {}
246247

247248
SILValue getCurrentDef() const { return currentDef; }
248249

lib/SILOptimizer/PassManager/PassPipeline.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -835,6 +835,10 @@ SILPassPipelinePlan::getOnonePassPipeline(const SILOptions &Options) {
835835
P.startPipeline("non-Diagnostic Enabling Mandatory Optimizations");
836836
P.addForEachLoopUnroll();
837837
P.addMandatoryCombine();
838+
// MandatoryCopyPropagation should only be run at -Onone, not -O.
839+
P.addMandatoryCopyPropagation();
840+
// TODO: GuaranteedARCOpts should be subsumed by CopyPropagation. There should
841+
// be no need to run another analysis of copies at -Onone.
838842
P.addGuaranteedARCOpts();
839843

840844
// First serialize the SIL if we are asked to.

lib/SILOptimizer/Transforms/CopyPropagation.cpp

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,12 @@ namespace {
4242
class CopyPropagation : public SILFunctionTransform {
4343
/// True if debug_value instructions should be pruned.
4444
bool pruneDebug;
45+
/// True of all values should be canonicalized.
46+
bool canonicalizeAll;
4547

4648
public:
47-
CopyPropagation(bool pruneDebug): pruneDebug(pruneDebug) {}
49+
CopyPropagation(bool pruneDebug, bool canonicalizeAll)
50+
: pruneDebug(pruneDebug), canonicalizeAll(canonicalizeAll) {}
4851

4952
/// The entry point to this function transformation.
5053
void run() override;
@@ -83,9 +86,15 @@ void CopyPropagation::run() {
8386
llvm::SmallSetVector<SILValue, 16> copiedDefs;
8487
for (auto &bb : *f) {
8588
for (auto &i : bb) {
86-
if (auto *copy = dyn_cast<CopyValueInst>(&i))
89+
if (auto *copy = dyn_cast<CopyValueInst>(&i)) {
8790
copiedDefs.insert(
8891
CanonicalizeOSSALifetime::getCanonicalCopiedDef(copy));
92+
} else if (canonicalizeAll) {
93+
if (auto *destroy = dyn_cast<DestroyValueInst>(&i)) {
94+
copiedDefs.insert(CanonicalizeOSSALifetime::getCanonicalCopiedDef(
95+
destroy->getOperand()));
96+
}
97+
}
8998
}
9099
}
91100
// Perform copy propgation for each copied value.

lib/SILOptimizer/Utils/CanonicalOSSALifetime.cpp

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -369,7 +369,7 @@ bool CanonicalizeOSSALifetime::computeCanonicalLiveness() {
369369
continue;
370370
}
371371
// Handle debug_value instructions separately.
372-
if (pruneDebug) {
372+
if (pruneDebugMode) {
373373
if (auto *dvi = dyn_cast<DebugValueInst>(user)) {
374374
// Only instructions potentially outside current pruned liveness are
375375
// interesting.
@@ -647,7 +647,7 @@ void CanonicalizeOSSALifetime::findOrInsertDestroyInBlock(SILBasicBlock *bb) {
647647
while (true) {
648648
auto *inst = &*instIter;
649649

650-
if (pruneDebug) {
650+
if (pruneDebugMode) {
651651
if (auto *dvi = dyn_cast<DebugValueInst>(inst)) {
652652
if (debugValues.erase(dvi))
653653
consumes.recordDebugAfterConsume(dvi);
@@ -675,7 +675,8 @@ void CanonicalizeOSSALifetime::findOrInsertDestroyInBlock(SILBasicBlock *bb) {
675675
return;
676676
}
677677
// This is not a potential last user. Keep scanning.
678-
// Allow lifetimes to be artificially extended up to the next call.
678+
// Allow lifetimes to be artificially extended up to the next call. The goal
679+
// is to prevent repeated destroy rewriting without inhibiting optimization.
679680
if (ApplySite::isa(inst)) {
680681
existingDestroy = nullptr;
681682
} else if (!existingDestroy) {
@@ -842,9 +843,10 @@ void CanonicalizeOSSALifetime::rewriteCopies() {
842843
if (reusedCopyOp) {
843844
reusedCopyOp->set(srcCopy);
844845
} else {
845-
instsToDelete.insert(srcCopy);
846-
LLVM_DEBUG(llvm::dbgs() << " Removing " << *srcCopy);
847-
++NumCopiesEliminated;
846+
if (instsToDelete.insert(srcCopy)) {
847+
LLVM_DEBUG(llvm::dbgs() << " Removing " << *srcCopy);
848+
++NumCopiesEliminated;
849+
}
848850
}
849851
}
850852
}

0 commit comments

Comments
 (0)