Skip to content

[5.1] SILOptimier: Fix a miscompile in COWArrayOpt. #23431

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
Mar 20, 2019
Merged

[5.1] SILOptimier: Fix a miscompile in COWArrayOpt. #23431

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
29 changes: 23 additions & 6 deletions lib/SILOptimizer/LoopTransforms/COWArrayOpt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,8 +410,7 @@ class COWArrayOpt {
// analyzing.
SILValue CurrentArrayAddr;
public:
COWArrayOpt(RCIdentityFunctionInfo *RCIA, SILLoop *L,
DominanceAnalysis *DA)
COWArrayOpt(RCIdentityFunctionInfo *RCIA, SILLoop *L, DominanceAnalysis *DA)
: RCIA(RCIA), Function(L->getHeader()->getParent()), Loop(L),
Preheader(L->getLoopPreheader()), DomTree(DA->get(Function)),
ColdBlocks(DA), CachedSafeLoop(false, false) {}
Expand All @@ -427,7 +426,8 @@ class COWArrayOpt {
bool checkSafeArrayValueUses(UserList &ArrayValueUsers);
bool checkSafeArrayElementUse(SILInstruction *UseInst, SILValue ArrayVal);
bool checkSafeElementValueUses(UserOperList &ElementValueUsers);
bool hoistMakeMutable(ArraySemanticsCall MakeMutable);
bool hoistMakeMutable(ArraySemanticsCall MakeMutable, bool dominatesExits);
bool dominatesExitingBlocks(SILBasicBlock *BB);
void hoistAddressProjections(Operand &ArrayOp);
bool hasLoopOnlyDestructorSafeArrayOperations();
SILValue getArrayAddressBase(SILValue V);
Expand Down Expand Up @@ -1037,7 +1037,8 @@ void COWArrayOpt::hoistAddressProjections(Operand &ArrayOp) {

/// Check if this call to "make_mutable" is hoistable, and move it, or delete it
/// if it's already hoisted.
bool COWArrayOpt::hoistMakeMutable(ArraySemanticsCall MakeMutable) {
bool COWArrayOpt::hoistMakeMutable(ArraySemanticsCall MakeMutable,
bool dominatesExits) {
LLVM_DEBUG(llvm::dbgs() << " Checking mutable array: " <<CurrentArrayAddr);

// We can hoist address projections (even if they are only conditionally
Expand All @@ -1056,7 +1057,12 @@ bool COWArrayOpt::hoistMakeMutable(ArraySemanticsCall MakeMutable) {

// Check whether we can hoist make_mutable based on the operations that are
// in the loop.
if (hasLoopOnlyDestructorSafeArrayOperations()) {
// Note that in this case we don't verify that the array buffer is not aliased
// and therefore we must be conservative if the make_mutable is executed
// conditionally (i.e. doesn't dominate all exit blocks).
// The test SILOptimizer/cowarray_opt.sil: dont_hoist_if_executed_conditionally
// shows the problem.
if (hasLoopOnlyDestructorSafeArrayOperations() && dominatesExits) {
// Done. We can hoist the make_mutable.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see an obvious connection between the comment and the test case. Here's my guess at how it works... For potentially aliased arrays, I think it's this pass' job to determine that a potential alias can't be accessed on any path from the loop header that does not pass through the original make_mutable. I guess we already do that within the loop but never checked outside the loop?

That isn't clear from the comment. Shouldn't it say something like:

The hoisted make_mutable releases the original array storage. If an
alias of that storage is accessed on any path reachable from the
header that doesn't already pass through make_mutable, then hoisting
is illegal. hasLoopOnlyDestructorSafeArrayOperations checks that the
array storage is not accessed within the loop. However, the array may
also be accessed after exiting the loop. Rather than analyzing code
outside the loop, simply check that the original make_mutable
dominates all exits.

I'm not sure if the above is actually true but at least it makes sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's probably a better way to describe it.

// We still need the array uses later to check if we can add loads to
// HoistableLoads.
Expand Down Expand Up @@ -1106,6 +1112,16 @@ bool COWArrayOpt::hoistMakeMutable(ArraySemanticsCall MakeMutable) {
return true;
}

bool COWArrayOpt::dominatesExitingBlocks(SILBasicBlock *BB) {
llvm::SmallVector<SILBasicBlock *, 8> ExitingBlocks;
Loop->getExitingBlocks(ExitingBlocks);
for (SILBasicBlock *Exiting : ExitingBlocks) {
if (!DomTree->dominates(BB, Exiting))
return false;
}
return true;
}

bool COWArrayOpt::run() {
LLVM_DEBUG(llvm::dbgs() << " Array Opts in Loop " << *Loop);

Expand All @@ -1123,6 +1139,7 @@ bool COWArrayOpt::run() {
for (auto *BB : Loop->getBlocks()) {
if (ColdBlocks.isCold(BB))
continue;
bool dominatesExits = dominatesExitingBlocks(BB);
for (auto II = BB->begin(), IE = BB->end(); II != IE;) {
// Inst may be moved by hoistMakeMutable.
SILInstruction *Inst = &*II;
Expand All @@ -1134,7 +1151,7 @@ bool COWArrayOpt::run() {
CurrentArrayAddr = MakeMutableCall.getSelf();
auto HoistedCallEntry = ArrayMakeMutableMap.find(CurrentArrayAddr);
if (HoistedCallEntry == ArrayMakeMutableMap.end()) {
if (!hoistMakeMutable(MakeMutableCall)) {
if (!hoistMakeMutable(MakeMutableCall, dominatesExits)) {
ArrayMakeMutableMap[CurrentArrayAddr] = nullptr;
continue;
}
Expand Down
33 changes: 33 additions & 0 deletions test/SILOptimizer/cowarray_opt.sil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,39 @@ bb2:
return %7 : $()
}

// CHECK-LABEL: sil @dont_hoist_if_executed_conditionally
// CHECK: bb0({{.*}}):
// CHECK-NOT: apply
// CHECK: bb1({{.*}}):
// CHECK: bb2:
// CHECK: apply
// CHECK: bb3:
sil @dont_hoist_if_executed_conditionally : $@convention(thin) (@inout MyArray<MyStruct>, @inout Builtin.Int1) -> MyArray<MyStruct> {
bb0(%0 : $*MyArray<MyStruct>, %1 : $*Builtin.Int1):
debug_value_addr %0 : $*MyArray<MyStruct>
%2 = load %0 : $*MyArray<MyStruct>
br bb1(%2 : $MyArray<MyStruct>)

bb1(%p1 : $MyArray<MyStruct>):
cond_br undef, bb2, bb3

bb2:
// If this block is never taken, then hoisting to bb0 would change the value of %p3.
%5 = function_ref @array_make_mutable : $@convention(method) (@inout MyArray<MyStruct>) -> ()
%6 = apply %5(%0) : $@convention(method) (@inout MyArray<MyStruct>) -> ()
%7 = load %0 : $*MyArray<MyStruct>
br bb4(%7 : $MyArray<MyStruct>)

bb3:
br bb4(%p1 : $MyArray<MyStruct>)

bb4(%p2 : $MyArray<MyStruct>):
cond_br undef, bb1(%p2 : $MyArray<MyStruct>), bb5(%p2 : $MyArray<MyStruct>)

bb5(%p3 : $MyArray<MyStruct>):
return %p3 : $MyArray<MyStruct>
}

// CHECK-LABEL: sil @cow_should_ignore_mark_dependence_addrproj_use : $@convention(thin) (@inout MyArray<MyStruct>, @inout Builtin.Int1) -> () {
// CHECK: bb0(
// CHECK-NOT: br bb
Expand Down
35 changes: 35 additions & 0 deletions test/SILOptimizer/cowarray_opt_crash.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// RUN: %empty-directory(%t)
// RUN: %target-build-swift -O %s -o %t/a.out
// RUN: %target-run %t/a.out | %FileCheck %s

// REQUIRES: executable_test

// Check that the compiled code does not crash because of an over-release.
// End-to-end test for rdar://problem/48906146.

struct LCRNG: RandomNumberGenerator {
private var state: UInt64

init(seed: Int) {
state = UInt64(truncatingIfNeeded: seed)
for _ in 0..<10 { _ = next() }
}

mutating func next() -> UInt64 {
state = 2862933555777941757 &* state &+ 3037000493
return state
}
}


func test(_ body: () -> Void) {
body()
}

test {
var rng = LCRNG(seed: 42)
let v = Array(0..<3).shuffled(using: &rng)
// CHECK: 3
print(v.count)
}