Skip to content

Commit 39db766

Browse files
committed
Restrict CopyForwarding (and destroy hoisting) to OSSA SIL.
The premise of CopyForwarding was that memory locations have their own ownership lifetime. We knew this was unmaintainable at the time, and that was the original incentive for SIL opaque values, aided by OSSA. In the meantime, we've been relying on SILGen producing reasonable SIL patterns. Unfortunately, the CopyForwarding pass is still with us while we make major changes to SIL ownership patterns and agressively optimize ownership. That introduces risk. Ultimately, the entire CopyForwarding pass should be redesigned for OSSA-only and destroy hoisting should be a simple OSSA utility where most of the work is done by AccessPath::collectUses. But in the meantime, we should remove the source of risk by limiting the CopyForwarding pass to OSSA. Any performance regressions will be recovered as OSSA moves later in the pipeline. After that, opaque values will improve even more over the current state by handling generic SIL using the more powerful CopyPropagation pass. Fixes rdar://71584600 (miscompile in CopyForwarding's release hoisting) Here's an example of the kind of SIL the CopyForwarding does not anticipate (although it only actually miscompiles in much more obscure scenarios, which is why it's so dangerous): bb0(%0 : $AnyObject): %alloc1 = alloc_stack $AnyObject store %0 to %objaddr : $*AnyObject %ref = load %objaddr : $*AnyObject %alloc2 = alloc_stack $ObjWrapper # The in-memory reference is destroyed before retaining the loaded ref. copy_addr [take] %alloc1 to [initialization] %alloc2 : $*ObjWrapper retain_value %ref : $AnyObject
1 parent b50dfef commit 39db766

File tree

2 files changed

+57
-938
lines changed

2 files changed

+57
-938
lines changed

lib/SILOptimizer/Transforms/CopyForwarding.cpp

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1508,6 +1508,23 @@ class CopyForwardingPass : public SILFunctionTransform
15081508
if (!EnableCopyForwarding && !EnableDestroyHoisting)
15091509
return;
15101510

1511+
// This pass assumes that the ownership lifetime of a value in a memory
1512+
// locations can be determined by analyzing operations on the memory
1513+
// address. However, in non-OSSA code, this is not guaranteed. For example,
1514+
// this is valid non-OSSA SIL:
1515+
//
1516+
// bb0(%0 : $AnyObject):
1517+
// %alloc1 = alloc_stack $AnyObject
1518+
// store %0 to %objaddr : $*AnyObject
1519+
// %ref = load %objaddr : $*AnyObject
1520+
// %alloc2 = alloc_stack $ObjWrapper
1521+
// # The in-memory reference is destroyed before retaining the loaded ref.
1522+
// copy_addr [take] %alloc1 to [initialization] %alloc2 : $*ObjWrapper
1523+
// retain_value %ref : $AnyObject
1524+
// destroy_addr %alloc2 : $*ObjWrapper
1525+
if (!getFunction()->hasOwnership())
1526+
return;
1527+
15111528
LLVM_DEBUG(llvm::dbgs() << "Copy Forwarding in Func "
15121529
<< getFunction()->getName() << "\n");
15131530

0 commit comments

Comments
 (0)