Skip to content

Commit 270824e

Browse files
eecksteineeckstein
authored
ArrayBoundCheckOpts: introduce a limit for the maximum dominator tree recursion depth (swiftlang#37339)
This is a quick fix for a stack overflow in case of very large functions. TODO: Ideally this algorithm would be implemented as an iterative worklist algorithm. rdar://77563057
1 parent 7236117 commit 270824e

File tree

2 files changed

+111
-8
lines changed

2 files changed

+111
-8
lines changed

‎lib/SILOptimizer/LoopTransforms/ArrayBoundsCheckOpts.cpp

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -870,6 +870,10 @@ static void reportBoundsChecks(SILFunction *F) {
870870
#endif
871871

872872
namespace {
873+
874+
// Should be more than enough to cover "usual" functions.
875+
static constexpr int maxRecursionDepth = 500;
876+
873877
/// Remove redundant checks in basic blocks and hoist redundant checks out of
874878
/// loops.
875879
class ABCOpt : public SILFunctionTransform {
@@ -891,13 +895,15 @@ class ABCOpt : public SILFunctionTransform {
891895
/// Walk down the dominator tree inside the loop, removing redundant checks.
892896
bool removeRedundantChecksInLoop(DominanceInfoNode *CurBB, ABCAnalysis &ABC,
893897
IndexedArraySet &DominatingSafeChecks,
894-
SILLoop *Loop);
898+
SILLoop *Loop,
899+
int recursionDepth);
895900
/// Analyze the loop for arrays that are not modified and perform dominator
896901
/// tree based redundant bounds check removal.
897902
bool hoistChecksInLoop(DominanceInfoNode *DTNode, ABCAnalysis &ABC,
898903
InductionAnalysis &IndVars, SILBasicBlock *Preheader,
899904
SILBasicBlock *Header,
900-
SILBasicBlock *SingleExitingBlk);
905+
SILBasicBlock *SingleExitingBlk,
906+
int recursionDepth);
901907

902908
public:
903909
void run() override {
@@ -1055,10 +1061,16 @@ bool ABCOpt::removeRedundantChecksInBlock(SILBasicBlock &BB) {
10551061
bool ABCOpt::removeRedundantChecksInLoop(DominanceInfoNode *CurBB,
10561062
ABCAnalysis &ABC,
10571063
IndexedArraySet &DominatingSafeChecks,
1058-
SILLoop *Loop) {
1064+
SILLoop *Loop,
1065+
int recursionDepth) {
10591066
auto *BB = CurBB->getBlock();
10601067
if (!Loop->contains(BB))
10611068
return false;
1069+
1070+
// Avoid a stack overflow for very deep dominator trees.
1071+
if (recursionDepth >= maxRecursionDepth)
1072+
return false;
1073+
10621074
bool Changed = false;
10631075

10641076
// When we come back from the dominator tree recursion we need to remove
@@ -1112,7 +1124,8 @@ bool ABCOpt::removeRedundantChecksInLoop(DominanceInfoNode *CurBB,
11121124
// Traverse the children in the dominator tree inside the loop.
11131125
for (auto Child : *CurBB)
11141126
Changed |=
1115-
removeRedundantChecksInLoop(Child, ABC, DominatingSafeChecks, Loop);
1127+
removeRedundantChecksInLoop(Child, ABC, DominatingSafeChecks, Loop,
1128+
recursionDepth + 1);
11161129

11171130
// Remove checks we have seen for the first time.
11181131
std::for_each(SafeChecksToPop.begin(), SafeChecksToPop.end(),
@@ -1155,7 +1168,8 @@ bool ABCOpt::processLoop(SILLoop *Loop) {
11551168
// check for safety outside the loop (with ABCAnalysis).
11561169
IndexedArraySet DominatingSafeChecks;
11571170
bool Changed = removeRedundantChecksInLoop(DT->getNode(Header), ABC,
1158-
DominatingSafeChecks, Loop);
1171+
DominatingSafeChecks, Loop,
1172+
/*recursionDepth*/ 0);
11591173

11601174
if (!EnableABCHoisting)
11611175
return Changed;
@@ -1261,7 +1275,7 @@ bool ABCOpt::processLoop(SILLoop *Loop) {
12611275

12621276
// Hoist bounds checks.
12631277
Changed |= hoistChecksInLoop(DT->getNode(Header), ABC, IndVars, Preheader,
1264-
Header, SingleExitingBlk);
1278+
Header, SingleExitingBlk, /*recursionDepth*/ 0);
12651279
if (Changed) {
12661280
Preheader->getParent()->verify();
12671281
}
@@ -1271,7 +1285,12 @@ bool ABCOpt::processLoop(SILLoop *Loop) {
12711285
bool ABCOpt::hoistChecksInLoop(DominanceInfoNode *DTNode, ABCAnalysis &ABC,
12721286
InductionAnalysis &IndVars,
12731287
SILBasicBlock *Preheader, SILBasicBlock *Header,
1274-
SILBasicBlock *SingleExitingBlk) {
1288+
SILBasicBlock *SingleExitingBlk,
1289+
int recursionDepth) {
1290+
// Avoid a stack overflow for very deep dominator trees.
1291+
if (recursionDepth >= maxRecursionDepth)
1292+
return false;
1293+
12751294
bool Changed = false;
12761295
auto *CurBB = DTNode->getBlock();
12771296
bool blockAlwaysExecutes =
@@ -1370,7 +1389,7 @@ bool ABCOpt::hoistChecksInLoop(DominanceInfoNode *DTNode, ABCAnalysis &ABC,
13701389
// Traverse the children in the dominator tree.
13711390
for (auto Child : *DTNode)
13721391
Changed |= hoistChecksInLoop(Child, ABC, IndVars, Preheader, Header,
1373-
SingleExitingBlk);
1392+
SingleExitingBlk, recursionDepth + 1);
13741393

13751394
return Changed;
13761395
}

‎test/SILOptimizer/abcopt_large_cfg.sil.gyb

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
// RUN: %empty-directory(%t)
2+
// RUN: %gyb %s > %t/main.sil
3+
4+
// Check that the optimization does not crash due to a stack overflow.
5+
6+
// RUN: %target-sil-opt -sil-verify-none -abcopts %t/main.sil | %FileCheck %s
7+
8+
sil_stage canonical
9+
10+
import Builtin
11+
import Swift
12+
import SwiftShims
13+
14+
struct ArrayIntBuffer {
15+
var storage : Builtin.NativeObject
16+
}
17+
18+
struct ArrayInt{
19+
var buffer : ArrayIntBuffer
20+
}
21+
22+
sil [ossa] @take_array : $@convention(thin) (@inout ArrayInt) -> () {
23+
bb0(%0 : $*ArrayInt):
24+
unreachable
25+
}
26+
27+
28+
sil public_external [ossa] [_semantics "array.check_subscript"] @checkbounds : $@convention(method) (Int32, Bool, @owned ArrayInt) -> _DependenceToken {
29+
bb0(%0: $Int32, %1: $Bool, %2: @owned $ArrayInt):
30+
unreachable
31+
}
32+
33+
// CHECK-LABEL: sil [ossa] @test_very_deep_domtree :
34+
35+
// Currently there is nothing ot check here, because the optimization bails in
36+
// this case.
37+
// In future we might check that even with a deep domtree it can hoist the check.
38+
39+
// CHECK-LABEL: } // end sil function 'test_very_deep_domtree'
40+
sil [ossa] @test_very_deep_domtree : $@convention(thin) (Int32, @inout ArrayInt) -> Int32 {
41+
bb0(%0 : $Int32, %1 : $*ArrayInt):
42+
%%2 = integer_literal $Builtin.Int1, -1
43+
%%3 = struct $Bool (%2 : $Builtin.Int1)
44+
%%4 = struct_extract %0 : $Int32, #Int32._value
45+
%%5 = integer_literal $Builtin.Int32, 0
46+
br bb1(%5 : $Builtin.Int32)
47+
48+
bb1(%10 : $Builtin.Int32):
49+
50+
% for i in range(50000):
51+
br bb${i+2}
52+
bb${i+2}:
53+
% end
54+
55+
br bb200000
56+
57+
bb200000:
58+
%%11 = struct $Int32 (%10 : $Builtin.Int32)
59+
%%12 = function_ref @checkbounds : $@convention(method) (Int32, Bool, @owned ArrayInt) -> _DependenceToken
60+
%%13 = load [copy] %1 : $*ArrayInt
61+
%%17 = apply %12(%11, %3, %13) : $@convention(method) (Int32, Bool, @owned ArrayInt) -> _DependenceToken
62+
%%18 = integer_literal $Builtin.Int32, 1
63+
%%19 = integer_literal $Builtin.Int1, -1
64+
%%20 = builtin "sadd_with_overflow_Int32"(%10 : $Builtin.Int32, %18 : $Builtin.Int32, %19 : $Builtin.Int1) : $(Builtin.Int32, Builtin.Int1)
65+
%%21 = tuple_extract %20 : $(Builtin.Int32, Builtin.Int1), 0
66+
%%22 = tuple_extract %20 : $(Builtin.Int32, Builtin.Int1), 1
67+
cond_fail %22 : $Builtin.Int1, ""
68+
%%24 = builtin "cmp_eq_Int32"(%21 : $Builtin.Int32, %4 : $Builtin.Int32) : $Builtin.Int1
69+
cond_br %24, bb200002, bb200001
70+
71+
bb200001:
72+
br bb1(%21 : $Builtin.Int32)
73+
74+
bb200002:
75+
%%27 = function_ref @take_array : $@convention(thin) (@inout ArrayInt) -> ()
76+
%%28 = apply %27(%1) : $@convention(thin) (@inout ArrayInt) -> ()
77+
%%29 = load [copy] %1 : $*ArrayInt
78+
%%30 = apply %12(%11, %3, %29) : $@convention(method) (Int32, Bool, @owned ArrayInt) -> _DependenceToken
79+
%%33 = struct $Int32 (%21 : $Builtin.Int32)
80+
return %33 : $Int32
81+
}
82+
83+
84+

0 commit comments

Comments
 (0)