Skip to content

Commit cf2c052

Browse files
eecksteineeckstein
authored
Merge pull request #37315 from eeckstein/fix-abc-opt
ArrayBoundCheckOpts: introduce a limit for the maximum dominator tree recursion depth
2 parents e91b305 + 2a17d9a commit cf2c052

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
@@ -864,6 +864,10 @@ static void reportBoundsChecks(SILFunction *F) {
864864
#endif
865865

866866
namespace {
867+
868+
// Should be more than enough to cover "usual" functions.
869+
static constexpr int maxRecursionDepth = 500;
870+
867871
/// Remove redundant checks in basic blocks and hoist redundant checks out of
868872
/// loops.
869873
class ABCOpt : public SILFunctionTransform {
@@ -885,13 +889,15 @@ class ABCOpt : public SILFunctionTransform {
885889
/// Walk down the dominator tree inside the loop, removing redundant checks.
886890
bool removeRedundantChecksInLoop(DominanceInfoNode *CurBB, ABCAnalysis &ABC,
887891
IndexedArraySet &DominatingSafeChecks,
888-
SILLoop *Loop);
892+
SILLoop *Loop,
893+
int recursionDepth);
889894
/// Analyze the loop for arrays that are not modified and perform dominator
890895
/// tree based redundant bounds check removal.
891896
bool hoistChecksInLoop(DominanceInfoNode *DTNode, ABCAnalysis &ABC,
892897
InductionAnalysis &IndVars, SILBasicBlock *Preheader,
893898
SILBasicBlock *Header,
894-
SILBasicBlock *SingleExitingBlk);
899+
SILBasicBlock *SingleExitingBlk,
900+
int recursionDepth);
895901

896902
public:
897903
void run() override {
@@ -1049,10 +1055,16 @@ bool ABCOpt::removeRedundantChecksInBlock(SILBasicBlock &BB) {
10491055
bool ABCOpt::removeRedundantChecksInLoop(DominanceInfoNode *CurBB,
10501056
ABCAnalysis &ABC,
10511057
IndexedArraySet &DominatingSafeChecks,
1052-
SILLoop *Loop) {
1058+
SILLoop *Loop,
1059+
int recursionDepth) {
10531060
auto *BB = CurBB->getBlock();
10541061
if (!Loop->contains(BB))
10551062
return false;
1063+
1064+
// Avoid a stack overflow for very deep dominator trees.
1065+
if (recursionDepth >= maxRecursionDepth)
1066+
return false;
1067+
10561068
bool Changed = false;
10571069

10581070
// When we come back from the dominator tree recursion we need to remove
@@ -1106,7 +1118,8 @@ bool ABCOpt::removeRedundantChecksInLoop(DominanceInfoNode *CurBB,
11061118
// Traverse the children in the dominator tree inside the loop.
11071119
for (auto Child : *CurBB)
11081120
Changed |=
1109-
removeRedundantChecksInLoop(Child, ABC, DominatingSafeChecks, Loop);
1121+
removeRedundantChecksInLoop(Child, ABC, DominatingSafeChecks, Loop,
1122+
recursionDepth + 1);
11101123

11111124
// Remove checks we have seen for the first time.
11121125
std::for_each(SafeChecksToPop.begin(), SafeChecksToPop.end(),
@@ -1149,7 +1162,8 @@ bool ABCOpt::processLoop(SILLoop *Loop) {
11491162
// check for safety outside the loop (with ABCAnalysis).
11501163
IndexedArraySet DominatingSafeChecks;
11511164
bool Changed = removeRedundantChecksInLoop(DT->getNode(Header), ABC,
1152-
DominatingSafeChecks, Loop);
1165+
DominatingSafeChecks, Loop,
1166+
/*recursionDepth*/ 0);
11531167

11541168
if (!EnableABCHoisting)
11551169
return Changed;
@@ -1255,7 +1269,7 @@ bool ABCOpt::processLoop(SILLoop *Loop) {
12551269

12561270
// Hoist bounds checks.
12571271
Changed |= hoistChecksInLoop(DT->getNode(Header), ABC, IndVars, Preheader,
1258-
Header, SingleExitingBlk);
1272+
Header, SingleExitingBlk, /*recursionDepth*/ 0);
12591273
if (Changed) {
12601274
Preheader->getParent()->verify();
12611275
}
@@ -1265,7 +1279,12 @@ bool ABCOpt::processLoop(SILLoop *Loop) {
12651279
bool ABCOpt::hoistChecksInLoop(DominanceInfoNode *DTNode, ABCAnalysis &ABC,
12661280
InductionAnalysis &IndVars,
12671281
SILBasicBlock *Preheader, SILBasicBlock *Header,
1268-
SILBasicBlock *SingleExitingBlk) {
1282+
SILBasicBlock *SingleExitingBlk,
1283+
int recursionDepth) {
1284+
// Avoid a stack overflow for very deep dominator trees.
1285+
if (recursionDepth >= maxRecursionDepth)
1286+
return false;
1287+
12691288
bool Changed = false;
12701289
auto *CurBB = DTNode->getBlock();
12711290
bool blockAlwaysExecutes =
@@ -1364,7 +1383,7 @@ bool ABCOpt::hoistChecksInLoop(DominanceInfoNode *DTNode, ABCAnalysis &ABC,
13641383
// Traverse the children in the dominator tree.
13651384
for (auto Child : *DTNode)
13661385
Changed |= hoistChecksInLoop(Child, ABC, IndVars, Preheader, Header,
1367-
SingleExitingBlk);
1386+
SingleExitingBlk, recursionDepth + 1);
13681387

13691388
return Changed;
13701389
}

‎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)