Skip to content

[Mem2Reg] Handled unreachables for single-block allocations. #39930

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 2 commits into from
Oct 28, 2021
Merged

[Mem2Reg] Handled unreachables for single-block allocations. #39930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0fc25cc
[Mem2Reg] Skipped unreachable blocks.
nate-chandler Oct 27, 2021
797eab1
[Mem2Reg] Handled unreachables for single-block allocations.
nate-chandler Oct 25, 2021
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
46 changes: 40 additions & 6 deletions lib/SILOptimizer/Transforms/SILMem2Reg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1651,12 +1651,42 @@ void MemoryToRegisters::removeSingleBlockAllocation(AllocStackInst *asi) {

if (runningVals && runningVals->isStorageValid &&
shouldAddLexicalLifetime(asi)) {
assert(isa<UnreachableInst>(parentBlock->getTerminator()) &&
"valid storage after reaching end of single-block allocation not "
"terminated by unreachable?!");
endLexicalLifetimeBeforeInst(
asi, /*beforeInstruction=*/parentBlock->getTerminator(), ctx,
runningVals->value);
// There is still valid storage after visiting all instructions in this
// block which are the only instructions involving this alloc_stack.
// This can only happen if all paths from this block end in unreachable.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you give a SIL example here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I talked with Nate. I am ok with this actually.

//
// We need to end the lexical lifetime at the last possible location, either
// just before an unreachable instruction or just before a branch to a block
// that is not dominated by parentBlock.

// Walk forward from parentBlock until finding blocks which either
// (1) terminate in unreachable
// (2) have successors which are not dominated by parentBlock
GraphNodeWorklist<SILBasicBlock *, 2> worklist;
worklist.initialize(parentBlock);
while (auto *block = worklist.pop()) {
auto *terminator = block->getTerminator();
if (isa<UnreachableInst>(terminator)) {
endLexicalLifetimeBeforeInst(asi, /*beforeInstruction=*/terminator, ctx,
runningVals->value);
continue;
}
bool endedLifetime = false;
for (auto *successor : block->getSuccessorBlocks()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This loop confuses me. A block with multiple successors must dominate all of them. You should be able to call something like getSingleSuccessor and only do the dominance check in that case, without any loop or confusing control flow based on a boolean flag.

To sanity check this, you can just assert that parentBlock dominates each block popped from the worklist immediately after popping it.

if (!domInfo->dominates(parentBlock, successor)) {
endLexicalLifetimeBeforeInst(asi, /*beforeInstruction=*/terminator,
ctx, runningVals->value);
endedLifetime = true;
break;
}
}
if (endedLifetime) {
continue;
}
for (auto *successor : block->getSuccessorBlocks()) {
worklist.insert(successor);
}
}
}
}

Expand Down Expand Up @@ -1733,6 +1763,10 @@ bool MemoryToRegisters::run() {
f.verifyCriticalEdges();

for (auto &block : f) {
// Don't waste time optimizing unreachable blocks.
if (!domInfo->isReachableFromEntry(&block)) {
continue;
}
for (SILInstruction *inst : deleter.updatingReverseRange(&block)) {
auto *asi = dyn_cast<AllocStackInst>(inst);
if (!asi)
Expand Down
181 changes: 181 additions & 0 deletions test/SILOptimizer/mem2reg_lifetime.sil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,8 @@ enum E {
case one(C)
}

sil [ossa] @getC : $@convention(thin) () -> @owned C

sil [ossa] @callee_guaranteed : $@convention(thin) (@guaranteed C) -> ()
sil [ossa] @callee_owned : $@convention(thin) (@owned C) -> ()
sil [ossa] @callee_error_owned : $@convention(thin) (@owned C) -> @error Error
Expand Down Expand Up @@ -1545,6 +1547,185 @@ entry(%instance : @owned $C):
unreachable
}


// CHECK-LABEL: sil [ossa] @unreachable_5 : $@convention(thin) (@owned C) -> () {
// CHECK: {{bb[0-9]+}}([[INSTANCE:%[^,]+]] : @owned $C):
// CHECK: [[LIFETIME:%[^,]+]] = begin_borrow [lexical] [[INSTANCE]]
// CHECK: [[LIFETIME_OWNED:%[^,]+]] = copy_value [[LIFETIME]]
// CHECK: br [[EXIT:bb[0-9]+]]
// CHECK: [[EXIT]]:
// CHECK: end_borrow [[LIFETIME]]
// CHECK: destroy_value [[INSTANCE]]
// CHECK: unreachable
// CHECK-LABEL: } // end sil function 'unreachable_5'
sil [ossa] @unreachable_5 : $@convention(thin) (@owned C) -> () {
entry(%instance_1 : @owned $C):
%addr = alloc_stack [lexical] $C
store %instance_1 to [init] %addr :$*C
br exit
exit:
unreachable
}

// CHECK-LABEL: sil [ossa] @unreachable_6 : $@convention(thin) (@owned C) -> () {
Copy link
Contributor

Choose a reason for hiding this comment

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

None of these unit test are commented. Outside the context of this PR it wouldn't be obvious what's being tested. A single line like: "bailout on alloc_stack in unreachable block" helps. When updating dozens of SIL tests cases, it's not practical to look at the git log for all of them.

I know no one else comments tests, but I still think it's important.

// CHECK: alloc_stack [lexical]
// CHECK-LABEL: } // end sil function 'unreachable_6'
sil [ossa] @unreachable_6 : $@convention(thin) (@owned C) -> () {
entry(%instance_1 : @owned $C):
br exit
unr:
%addr = alloc_stack [lexical] $C
store %instance_1 to [init] %addr :$*C
br die
die:
unreachable
exit:
%res = tuple ()
return %res : $()
}


// CHECK-LABEL: sil [ossa] @unreachable_7 : $@convention(thin) (@owned C) -> () {
// CHECK: alloc_stack [lexical]
// CHECK-LABEL: } // end sil function 'unreachable_7'
sil [ossa] @unreachable_7 : $@convention(thin) (@owned C) -> () {
entry(%instance_1 : @owned $C):
cond_br undef, exit, die_later
unr:
%addr = alloc_stack [lexical] $C
store %instance_1 to [init] %addr :$*C
br die_later_2
die_later_2:
br die
die_later:
br die
die:
unreachable
exit:
%res = tuple ()
return %res : $()
}


// CHECK-LABEL: sil [ossa] @unreachable_8 : $@convention(thin) (@owned C) -> () {
// CHECK: alloc_stack [lexical]
// CHECK-LABEL: } // end sil function 'unreachable_8'
sil [ossa] @unreachable_8 : $@convention(thin) (@owned C) -> () {
entry(%instance_1 : @owned $C):
cond_br undef, exit, die_later
unr:
%addr = alloc_stack [lexical] $C
store %instance_1 to [init] %addr : $*C
cond_br undef, die_later_2_a, die_later_2_b
die_later_2_a:
br die_later_2
die_later_2_b:
br die_later_2
die_later_2:
br die
die_later:
br die
die:
unreachable
exit:
%res = tuple ()
return %res : $()
}


// CHECK-LABEL: sil [ossa] @unreachable_loop_1 : $@convention(thin) () -> () {
// CHECK: {{bb[0-9]+}}:
// CHECK: [[GET_C:%[^,]+]] = function_ref @getC : $@convention(thin) () -> @owned C
// CHECK: [[INSTANCE:%[^,]+]] = apply [[GET_C]]() : $@convention(thin) () -> @owned C
// CHECK: [[LIFETIME:%[^,]+]] = begin_borrow [lexical] [[INSTANCE]]
// CHECK: [[LIFETIME_OWNED:%[^,]+]] = copy_value [[LIFETIME]]
// CHECK: br [[LOOP_HEADER:bb[0-9]+]]
// CHECK: [[LOOP_HEADER]]:
// CHECK: br [[LOOP_BODY:bb[0-9]+]]
// CHECK: [[LOOP_BODY]]:
// CHECK: br [[LOOP_EXIT:bb[0-9]+]]
// CHECK: [[LOOP_EXIT]]:
// CHECK: cond_br undef, [[LOOP_BACKEDGE:bb[0-9]+]], [[DIE:bb[0-9]+]]
// CHECK: [[LOOP_BACKEDGE]]:
// CHECK: br [[LOOP_HEADER]]
// CHECK: [[DIE]]:
// CHECK: end_borrow [[LIFETIME]]
// CHECK: destroy_value [[INSTANCE]]
// CHECK: unreachable
// CHECK-LABEL: } // end sil function 'unreachable_loop_1'
sil [ossa] @unreachable_loop_1 : $@convention(thin) () -> () {
entry:
%delegate_var = alloc_stack [lexical] $C
%getC = function_ref @getC : $@convention(thin) () -> @owned C
%delegate = apply %getC() : $@convention(thin) () -> @owned C
store %delegate to [init] %delegate_var : $*C
br loop_header
loop_header:
br loop_body
loop_body:
br loop_exit
loop_exit:
cond_br undef, loop_backedge, die
loop_backedge:
br loop_header
die:
unreachable
}


// CHECK-LABEL: sil [ossa] @unreachable_loop_2 : $@convention(thin) () -> () {
// CHECK: alloc_stack [lexical]
// CHECK-LABEL: } // end sil function 'unreachable_loop_2'
sil [ossa] @unreachable_loop_2 : $@convention(thin) () -> () {
entry:
br loop_header
unr:
%delegate_var = alloc_stack [lexical] $C
%getC = function_ref @getC : $@convention(thin) () -> @owned C
%delegate = apply %getC() : $@convention(thin) () -> @owned C
store %delegate to [init] %delegate_var : $*C
br loop_header
loop_header:
br loop_body
loop_body:
br loop_exit
loop_exit:
cond_br undef, loop_backedge, die
loop_backedge:
br loop_header
die:
unreachable
}


// CHECK-LABEL: sil [ossa] @unreachable_loop_3 : $@convention(thin) () -> () {
// CHECK: alloc_stack [lexical]
// CHECK-LABEL: } // end sil function 'unreachable_loop_3'
sil [ossa] @unreachable_loop_3 : $@convention(thin) () -> () {
entry:
br die_entry
die_entry:
br die
unr:
%delegate_var = alloc_stack [lexical] $C
%getC = function_ref @getC : $@convention(thin) () -> @owned C
%delegate = apply %getC() : $@convention(thin) () -> @owned C
store %delegate to [init] %delegate_var : $*C
br loop_header
loop_header:
br loop_body
loop_body:
br loop_exit
loop_exit:
cond_br undef, loop_backedge, die_loop
loop_backedge:
br loop_header
die_loop:
br die
die:
unreachable
}

// }} unreachable_N

// diamond_N {{ the usages of the alloc_stack occur over a diamond of blocks
Expand Down