Skip to content

Commit c3b055a

Browse files
hamishknighthamishknight
committed
[Profiler] Improve if statement coverage
Fix counters for regions following `else if`s, fix the counters for `else if` conditions, and fix handling of `break` statements. Also while here, clean up the handling of branch exit regions such that we don't generate multiple overlapping regions for each branch, but a single region at the end of the entire `if` statement that accounts for all exiting control flow. rdar://104078910 rdar://104079242
1 parent f4e09c5 commit c3b055a

7 files changed

+293
-92
lines changed

‎lib/SIL/IR/SILProfiler.cpp

+80-7
Original file line numberDiff line numberDiff line change
@@ -422,6 +422,13 @@ class CounterExpr {
422422
/// Returns true if this is a zero counter.
423423
bool isZero() const { return Counter.isZero(); }
424424

425+
friend bool operator==(const CounterExpr &LHS, const CounterExpr &RHS) {
426+
return LHS.Counter == RHS.Counter;
427+
}
428+
friend bool operator!=(const CounterExpr &LHS, const CounterExpr &RHS) {
429+
return !(LHS == RHS);
430+
}
431+
425432
llvm::coverage::Counter getLLVMCounter() const { return Counter; }
426433

427434
void print(raw_ostream &OS,
@@ -476,8 +483,9 @@ class SourceMappingRegion {
476483
}
477484

478485
SourceMappingRegion(Kind RegionKind, ASTNode Node, SourceRange Range,
486+
llvm::Optional<CounterExpr> Counter,
479487
const SourceManager &SM)
480-
: RegionKind(RegionKind), Node(Node) {
488+
: RegionKind(RegionKind), Node(Node), Counter(Counter) {
481489
assert(Range.isValid());
482490
StartLoc = Range.Start;
483491
EndLoc = Lexer::getLocForEndOfToken(SM, Range.End);
@@ -492,16 +500,18 @@ class SourceMappingRegion {
492500

493501
// Note we don't store counters for nodes, as we need to be able to fix them
494502
// up later.
495-
return SourceMappingRegion(Kind::Node, Node, Range, SM);
503+
return SourceMappingRegion(Kind::Node, Node, Range, /*Counter*/ llvm::None,
504+
SM);
496505
}
497506

498507
/// Create a source region for an ASTNode that is only present for scoping of
499508
/// child regions, and doesn't need to be included in the resulting set of
500509
/// regions.
501-
static SourceMappingRegion scopingOnly(ASTNode Node,
502-
const SourceManager &SM) {
510+
static SourceMappingRegion
511+
scopingOnly(ASTNode Node, const SourceManager &SM,
512+
llvm::Optional<CounterExpr> Counter = llvm::None) {
503513
return SourceMappingRegion(Kind::ScopingOnly, Node, Node.getSourceRange(),
504-
SM);
514+
Counter, SM);
505515
}
506516

507517
/// Create a refined region for a given counter.
@@ -967,6 +977,14 @@ struct CoverageMapping : public ASTWalker {
967977
return;
968978
}
969979

980+
// Don't apply exit adjustments to if statement branches, they should
981+
// be handled at the end of the statement. This avoids creating awkward
982+
// overlapping exit regions for each branch, and ensures 'break'
983+
// statements only have their jump counted once for the entire
984+
// statement.
985+
if (isa<IfStmt>(ParentStmt))
986+
return;
987+
970988
if (auto *LS = dyn_cast<LabeledStmt>(ParentStmt))
971989
JumpsToLabel = getCounter(LS);
972990
}
@@ -1225,7 +1243,7 @@ struct CoverageMapping : public ASTWalker {
12251243
// it by break statements.
12261244
assignCounter(IS, CounterExpr::Zero());
12271245

1228-
// FIXME: This is a redundant region.
1246+
// FIXME: This is a redundant region for non else-ifs.
12291247
if (auto *Cond = getConditionNode(IS->getCond()))
12301248
assignCounter(Cond, getCurrentCounter());
12311249

@@ -1244,13 +1262,68 @@ struct CoverageMapping : public ASTWalker {
12441262
// terms of it.
12451263
auto ThenCounter = assignKnownCounter(IS->getThenStmt());
12461264
IS->getThenStmt()->walk(*this);
1265+
auto ThenDelta =
1266+
CounterExpr::Sub(ThenCounter, getExitCounter(), CounterBuilder);
12471267

1268+
llvm::Optional<CounterExpr> ElseDelta;
12481269
if (auto *Else = IS->getElseStmt()) {
12491270
auto ElseCounter = CounterExpr::Sub(ParentCounter, ThenCounter,
12501271
CounterBuilder);
1251-
assignCounter(Else, ElseCounter);
1272+
// We handle `else if` and `else` slightly differently here. For
1273+
// `else` we have a BraceStmt, and can use the existing scoping logic
1274+
// to handle calculating the exit count. For `else if`, we need to
1275+
// set up a new scope to contain the child `if` statement, effectively
1276+
// we treat:
1277+
//
1278+
// if .random() {
1279+
// } else if .random() {
1280+
// } else {
1281+
// }
1282+
//
1283+
// the same as:
1284+
//
1285+
// if .random() {
1286+
// } else {
1287+
// if .random() {
1288+
// } else {
1289+
// }
1290+
// }
1291+
//
1292+
// This ensures we assign a correct counter to the `else if`
1293+
// condition, and allows us to compute the exit count correctly. We
1294+
// don't need the fake `else` scope to be included in the resulting
1295+
// set of regions, so we mark it scoping-only.
1296+
if (isa<BraceStmt>(Else)) {
1297+
assignCounter(Else, ElseCounter);
1298+
} else {
1299+
pushRegion(SourceMappingRegion::scopingOnly(Else, SM, ElseCounter));
1300+
}
12521301
Else->walk(*this);
1302+
1303+
// Once we've walked the `else`, compute the delta exit count. For
1304+
// a normal `else` we can use the computed exit count, for an
1305+
// `else if` we can take the current region count since we don't have
1306+
// a proper scope. This is a little hacked together, but we'll be able
1307+
// to do away with all of this once we re-implement as a SILOptimizer
1308+
// pass.
1309+
auto AfterElse = isa<BraceStmt>(Else) ? getExitCounter()
1310+
: getCurrentCounter();
1311+
if (!isa<BraceStmt>(Else))
1312+
popRegions(Else);
1313+
1314+
ElseDelta = CounterExpr::Sub(ElseCounter, AfterElse, CounterBuilder);
12531315
}
1316+
// Compute the exit count following the `if`, taking jumps to the
1317+
// statement by breaks into account, and the delta of the `then` branch
1318+
// and `else` branch if we have one.
1319+
auto AfterIf = getCurrentCounter();
1320+
AfterIf = CounterExpr::Add(AfterIf, getCounter(IS), CounterBuilder);
1321+
AfterIf = CounterExpr::Sub(AfterIf, ThenDelta, CounterBuilder);
1322+
if (ElseDelta)
1323+
AfterIf = CounterExpr::Sub(AfterIf, *ElseDelta, CounterBuilder);
1324+
1325+
if (AfterIf != getCurrentCounter())
1326+
replaceCount(AfterIf, getEndLoc(IS));
12541327
}
12551328
// Already visited the children.
12561329
// FIXME: The ASTWalker should do a post-walk for SkipChildren.

‎test/Profiler/coverage_deinit.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -21,4 +21,4 @@ public class Foo {
2121
// CHECK-NEXT: [[@LINE-8]]:10 -> [[@LINE-4]]:4 : 0
2222
// CHECK-NEXT: [[@LINE-8]]:8 -> [[@LINE-8]]:17 : 0
2323
// CHECK-NEXT: [[@LINE-9]]:18 -> [[@LINE-7]]:6 : 1
24-
// CHECK-NEXT: [[@LINE-8]]:6 -> [[@LINE-7]]:4 : 0
24+
// CHECK-NEXT: }

‎test/Profiler/coverage_errors.swift

+31-35
Original file line numberDiff line numberDiff line change
@@ -658,62 +658,58 @@ func test49() throws -> Int { // CHECK-NEXT: [[@LINE]]:29 -> [[@LINE+6]]:2 :
658658
} // CHECK-NEXT: }
659659

660660
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test50SiyKF"
661-
func test50() throws -> Int { // CHECK-NEXT: [[@LINE]]:29 -> [[@LINE+8]]:2 : 0
661+
func test50() throws -> Int { // CHECK-NEXT: [[@LINE]]:29 -> [[@LINE+7]]:2 : 0
662662
let x = if try throwingBool() { // CHECK-NEXT: [[@LINE]]:14 -> [[@LINE]]:32 : 0
663663
try throwingFn() // CHECK-NEXT: [[@LINE-1]]:32 -> [[@LINE+4]]:11 : (0 - 2)
664664
} else { // CHECK-NEXT: [[@LINE-2]]:33 -> [[@LINE]]:4 : 1
665665
1 // CHECK-NEXT: [[@LINE-2]]:21 -> [[@LINE-1]]:4 : (1 - 3)
666-
} // CHECK-NEXT: [[@LINE-2]]:4 -> [[@LINE+1]]:11 : ((0 - 2) - 3)
667-
return x // CHECK-NEXT: [[@LINE-3]]:10 -> [[@LINE-1]]:4 : ((0 - 1) - 2)
668-
// CHECK-NEXT: [[@LINE-2]]:4 -> [[@LINE-1]]:11 : ((0 - 2) - 3)
666+
} // CHECK-NEXT: [[@LINE-2]]:10 -> [[@LINE]]:4 : ((0 - 1) - 2)
667+
return x // CHECK-NEXT: [[@LINE-1]]:4 -> [[@LINE]]:11 : ((0 - 2) - 3)
669668
} // CHECK-NEXT: }
670669

671670
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test51SiyKF"
672-
func test51() throws -> Int { // CHECK-NEXT: [[@LINE]]:29 -> [[@LINE+9]]:2 : 0
671+
func test51() throws -> Int { // CHECK-NEXT: [[@LINE]]:29 -> [[@LINE+8]]:2 : 0
673672
let x = if try throwingBool() { // CHECK-NEXT: [[@LINE]]:14 -> [[@LINE]]:32 : 0
674673
try throwingFn() // CHECK-NEXT: [[@LINE-1]]:32 -> [[@LINE+4]]:11 : (0 - 2)
675674
} else { // CHECK-NEXT: [[@LINE-2]]:33 -> [[@LINE]]:4 : 1
676675
try throwingFn() // CHECK-NEXT: [[@LINE-2]]:21 -> [[@LINE-1]]:4 : (1 - 3)
677-
} // CHECK-NEXT: [[@LINE-2]]:4 -> [[@LINE+1]]:11 : ((0 - 2) - 3)
678-
return x // CHECK-NEXT: [[@LINE-3]]:10 -> [[@LINE-1]]:4 : ((0 - 1) - 2)
679-
// CHECK-NEXT: [[@LINE-3]]:21 -> [[@LINE-2]]:4 : (((0 - 1) - 2) - 4)
680-
// CHECK-NEXT: [[@LINE-3]]:4 -> [[@LINE-2]]:11 : (((0 - 2) - 3) - 4)
676+
} // CHECK-NEXT: [[@LINE-2]]:10 -> [[@LINE]]:4 : ((0 - 1) - 2)
677+
return x // CHECK-NEXT: [[@LINE-2]]:21 -> [[@LINE-1]]:4 : (((0 - 1) - 2) - 4)
678+
// CHECK-NEXT: [[@LINE-2]]:4 -> [[@LINE-1]]:11 : (((0 - 2) - 3) - 4)
681679
} // CHECK-NEXT: }
682680

683681
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test52SiyKF"
684-
func test52() throws -> Int { // CHECK-NEXT: [[@LINE]]:29 -> [[@LINE+10]]:2 : 0
685-
let x = if try throwingBool(), // CHECK-NEXT: [[@LINE]]:14 -> [[@LINE]]:32 : 0
686-
try throwingBool() { // CHECK-NEXT: [[@LINE-1]]:32 -> [[@LINE+5]]:11 : (0 - 2)
687-
try throwingFn() // CHECK-NEXT: [[@LINE-1]]:32 -> [[@LINE+4]]:11 : ((0 - 2) - 3)
688-
} else { // CHECK-NEXT: [[@LINE-2]]:33 -> [[@LINE]]:4 : 1
689-
try throwingFn() // CHECK-NEXT: [[@LINE-2]]:21 -> [[@LINE-1]]:4 : (1 - 4)
690-
} // CHECK-NEXT: [[@LINE-2]]:4 -> [[@LINE+1]]:11 : (((0 - 2) - 3) - 4)
691-
return x // CHECK-NEXT: [[@LINE-3]]:10 -> [[@LINE-1]]:4 : (((0 - 1) - 2) - 3)
692-
// CHECK-NEXT: [[@LINE-3]]:21 -> [[@LINE-2]]:4 : ((((0 - 1) - 2) - 3) - 5)
693-
// CHECK-NEXT: [[@LINE-3]]:4 -> [[@LINE-2]]:11 : ((((0 - 2) - 3) - 4) - 5)
682+
func test52() throws -> Int { // CHECK-NEXT: [[@LINE]]:29 -> [[@LINE+9]]:2 : 0
683+
let x = if try throwingBool(), // CHECK-NEXT: [[@LINE]]:14 -> [[@LINE]]:32 : 0
684+
try throwingBool() { // CHECK-NEXT: [[@LINE-1]]:32 -> [[@LINE+5]]:11 : (0 - 2)
685+
try throwingFn() // CHECK-NEXT: [[@LINE-1]]:32 -> [[@LINE+4]]:11 : ((0 - 2) - 3)
686+
} else { // CHECK-NEXT: [[@LINE-2]]:33 -> [[@LINE]]:4 : 1
687+
try throwingFn() // CHECK-NEXT: [[@LINE-2]]:21 -> [[@LINE-1]]:4 : (1 - 4)
688+
} // CHECK-NEXT: [[@LINE-2]]:10 -> [[@LINE]]:4 : (((0 - 1) - 2) - 3)
689+
return x // CHECK-NEXT: [[@LINE-2]]:21 -> [[@LINE-1]]:4 : ((((0 - 1) - 2) - 3) - 5)
690+
// CHECK-NEXT: [[@LINE-2]]:4 -> [[@LINE-1]]:11 : ((((0 - 2) - 3) - 4) - 5)
694691
} // CHECK-NEXT: }
695692

696693
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test53yyKF"
697-
func test53() throws { // CHECK-NEXT: [[@LINE]]:22 -> [[@LINE+10]]:2 : 0
698-
if try throwingBool(), // CHECK-NEXT: [[@LINE]]:6 -> [[@LINE]]:24 : 0
699-
try throwingBool() { // CHECK-NEXT: [[@LINE-1]]:24 -> [[@LINE+8]]:2 : (0 - 2)
700-
try throwingFn() // CHECK-NEXT: [[@LINE-1]]:25 -> [[@LINE+7]]:2 : ((0 - 2) - 3)
701-
} else { // CHECK-NEXT: [[@LINE-2]]:26 -> [[@LINE]]:4 : 1
702-
try throwingFn() // CHECK-NEXT: [[@LINE-2]]:21 -> [[@LINE-1]]:4 : (1 - 4)
703-
} // CHECK-NEXT: [[@LINE-2]]:4 -> [[@LINE+4]]:2 : (((0 - 2) - 3) - 4)
704-
// CHECK-NEXT: [[@LINE-3]]:10 -> [[@LINE-1]]:4 : (((0 - 1) - 2) - 3)
705-
// CHECK-NEXT: [[@LINE-3]]:21 -> [[@LINE-2]]:4 : ((((0 - 1) - 2) - 3) - 5)
706-
// CHECK-NEXT: [[@LINE-3]]:4 -> [[@LINE+1]]:2 : ((((0 - 2) - 3) - 4) - 5)
694+
func test53() throws { // CHECK-NEXT: [[@LINE]]:22 -> [[@LINE+9]]:2 : 0
695+
if try throwingBool(), // CHECK-NEXT: [[@LINE]]:6 -> [[@LINE]]:24 : 0
696+
try throwingBool() { // CHECK-NEXT: [[@LINE-1]]:24 -> [[@LINE+7]]:2 : (0 - 2)
697+
try throwingFn() // CHECK-NEXT: [[@LINE-1]]:25 -> [[@LINE+6]]:2 : ((0 - 2) - 3)
698+
} else { // CHECK-NEXT: [[@LINE-2]]:26 -> [[@LINE]]:4 : 1
699+
try throwingFn() // CHECK-NEXT: [[@LINE-2]]:21 -> [[@LINE-1]]:4 : (1 - 4)
700+
} // CHECK-NEXT: [[@LINE-2]]:10 -> [[@LINE]]:4 : (((0 - 1) - 2) - 3)
701+
// CHECK-NEXT: [[@LINE-2]]:21 -> [[@LINE-1]]:4 : ((((0 - 1) - 2) - 3) - 5)
702+
// CHECK-NEXT: [[@LINE-2]]:4 -> [[@LINE+1]]:2 : ((((0 - 2) - 3) - 4) - 5)
707703
} // CHECK-NEXT: }
708704

709705
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test54SiSgyF"
710-
func test54()-> Int? { // CHECK-NEXT: [[@LINE]]:22 -> [[@LINE+7]]:2 : 0
706+
func test54()-> Int? { // CHECK-NEXT: [[@LINE]]:22 -> [[@LINE+7]]:2 : 0
711707
if let x = try? throwingFn(),
712-
let y = try? throwingFn() { // CHECK-NEXT: [[@LINE]]:33 -> [[@LINE+2]]:4 : 1
713-
x + y // FIXME: This region is redundant, and not really accurate since we have implicit returns (rdar://118653218)
714-
} else { // CHECK-NEXT: [[@LINE]]:4 -> [[@LINE+3]]:2 : 0
715-
try? throwingFn() // CHECK-NEXT: [[@LINE-1]]:10 -> [[@LINE+1]]:4 : (0 - 1)
716-
} // CHECK-NEXT: [[@LINE]]:4 -> [[@LINE+1]]:2 : 0
708+
let y = try? throwingFn() { // CHECK-NEXT: [[@LINE]]:33 -> [[@LINE+2]]:4 : 1
709+
x + y
710+
} else { // CHECK-NEXT: [[@LINE]]:10 -> [[@LINE+2]]:4 : (0 - 1)
711+
try? throwingFn()
712+
}
717713
} // CHECK-NEXT: }
718714

719715
// CHECK-LABEL: sil_coverage_map {{.*}} "$s15coverage_errors6test55yyKF"

0 commit comments

Comments
 (0)