Skip to content

Commit 05e2309

Browse files
hamishknighthamishknight
committed
[Profiler] Fix coverage mapping for switches
Previously we assumed the exit counter was the same as the entry counter. Update the logic such that we track break statements (including implicitly at the end of a case), as well as early returns and jumps to parent statements. This follows a similar logic to what we do for if statements. rdar://99141044
1 parent 40bb608 commit 05e2309

File tree

3 files changed

+156
-49
lines changed

3 files changed

+156
-49
lines changed

‎lib/SIL/IR/SILProfiler.cpp

Lines changed: 35 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -259,8 +259,6 @@ struct MapRegionCounters : public ASTWalker {
259259
mapRegion(RWS->getBody());
260260
} else if (auto *FES = dyn_cast<ForEachStmt>(S)) {
261261
mapRegion(FES->getBody());
262-
} else if (auto *SS = dyn_cast<SwitchStmt>(S)) {
263-
mapRegion(SS);
264262
} else if (auto *CS = dyn_cast<CaseStmt>(S)) {
265263
mapRegion(getProfilerStmtForCase(CS));
266264
}
@@ -647,8 +645,6 @@ struct PGOMapping : public ASTWalker {
647645
} else if (auto *FES = dyn_cast<ForEachStmt>(S)) {
648646
setKnownExecutionCount(FES->getBody());
649647
setExecutionCount(FES, parentCount);
650-
} else if (auto *SS = dyn_cast<SwitchStmt>(S)) {
651-
setKnownExecutionCount(SS);
652648
} else if (auto *CS = dyn_cast<CaseStmt>(S)) {
653649
setKnownExecutionCount(getProfilerStmtForCase(CS));
654650
}
@@ -823,17 +819,7 @@ struct CoverageMapping : public ASTWalker {
823819
if (ControlFlowAdjust)
824820
Count = &createCounter(CounterExpr::Sub(*Count, *ControlFlowAdjust));
825821

826-
if (Count->isSemanticallyZero()) {
827-
// If the counter is semantically zero, form an 'incomplete' region with
828-
// no starting location. This prevents forming unreachable regions unless
829-
// there is a following statement or expression to extend the region.
830-
RegionStack.emplace_back(ASTNode(), *Count, None, None);
831-
} else {
832-
// Otherwise, we have a non-zero counter, so form a new region starting
833-
// at the end of the previous scope. This ensures the user covers both
834-
// branches of a condition.
835-
RegionStack.emplace_back(ASTNode(), *Count, getEndLoc(Scope), None);
836-
}
822+
replaceCount(Count, getEndLoc(Scope));
837823
}
838824

839825
/// Push a region covering \c Node onto the stack.
@@ -847,10 +833,24 @@ struct CoverageMapping : public ASTWalker {
847833
});
848834
}
849835

850-
/// Replace the current region's count by pushing an incomplete region.
851-
void replaceCount(CounterExpr &&Expr, Optional<SourceLoc> Start = None) {
852-
CounterExpr &Counter = createCounter(std::move(Expr));
853-
RegionStack.emplace_back(ASTNode(), Counter, Start, None);
836+
/// Replace the current region at \p Start with a new counter. If \p Start is
837+
/// \c None, or the counter is semantically zero, an 'incomplete' region is
838+
/// formed, which is not recorded unless followed by additional AST nodes.
839+
void replaceCount(CounterExpr *Counter, Optional<SourceLoc> Start) {
840+
// If the counter is semantically zero, form an 'incomplete' region with
841+
// no starting location. This prevents forming unreachable regions unless
842+
// there is a following statement or expression to extend the region.
843+
if (Start && Counter->isSemanticallyZero())
844+
Start = None;
845+
846+
RegionStack.emplace_back(ASTNode(), *Counter, Start, None);
847+
}
848+
849+
/// Replace the current region at \p Start with a new counter. If \p Start is
850+
/// \c None, or the counter is semantically zero, an 'incomplete' region is
851+
/// formed, which is not recorded unless followed by additional AST nodes.
852+
void replaceCount(CounterExpr &&Expr, Optional<SourceLoc> Start) {
853+
replaceCount(&createCounter(std::move(Expr)), Start);
854854
}
855855

856856
/// Get the location for the end of the last token in \c Node.
@@ -912,7 +912,7 @@ struct CoverageMapping : public ASTWalker {
912912
SourceMappingRegion &Region = getRegion();
913913
if (!Region.hasEndLoc())
914914
Region.setEndLoc(getEndLoc(S));
915-
replaceCount(CounterExpr::Zero());
915+
replaceCount(CounterExpr::Zero(), /*Start*/ None);
916916
}
917917

918918
Expr *getConditionNode(StmtCondition SC) {
@@ -1031,7 +1031,14 @@ struct CoverageMapping : public ASTWalker {
10311031
assignCounter(FES->getBody());
10321032

10331033
} else if (auto *SS = dyn_cast<SwitchStmt>(S)) {
1034-
assignCounter(SS);
1034+
// The counter for the switch statement itself tracks the number of jumps
1035+
// to it by break statements, including the implicit breaks at the end of
1036+
// cases.
1037+
assignCounter(SS, CounterExpr::Zero());
1038+
1039+
assignCounter(SS->getSubjectExpr(),
1040+
CounterExpr::Ref(getCurrentCounter()));
1041+
10351042
// Assign counters for cases so they're available for fallthrough.
10361043
for (CaseStmt *Case : SS->getCases())
10371044
assignCounter(Case);
@@ -1097,7 +1104,8 @@ struct CoverageMapping : public ASTWalker {
10971104
Stmt *BreakTarget = BS->getTarget();
10981105
if (auto *RWS = dyn_cast<RepeatWhileStmt>(BreakTarget)) {
10991106
subtractFromCounter(RWS->getCond(), getCurrentCounter());
1100-
} else if (!isa<SwitchStmt>(BreakTarget)) {
1107+
} else {
1108+
// Update the exit counter for the target.
11011109
addToCounter(BS->getTarget(), getCurrentCounter());
11021110
}
11031111

@@ -1115,9 +1123,12 @@ struct CoverageMapping : public ASTWalker {
11151123
replaceCount(CounterExpr::Ref(getCounter(S)), getEndLoc(S));
11161124

11171125
} else if (auto caseStmt = dyn_cast<CaseStmt>(S)) {
1118-
if (caseStmt->getParentKind() == CaseParentKind::Switch)
1126+
if (caseStmt->getParentKind() == CaseParentKind::Switch) {
1127+
// The end of a case block is an implicit break, update the exit
1128+
// counter to reflect this.
1129+
addToCounter(caseStmt->getParentStmt(), getCurrentCounter());
11191130
popRegions(S);
1120-
1131+
}
11211132
} else if (auto *DCS = dyn_cast<DoCatchStmt>(S)) {
11221133
assert(DoCatchStack.back() == DCS && "Malformed do-catch stack");
11231134
DoCatchStack.pop_back();

‎lib/SILGen/SILGenPattern.cpp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2814,7 +2814,6 @@ void SILGenFunction::emitSwitchStmt(SwitchStmt *S) {
28142814
emission.emitAddressOnlyAllocations();
28152815

28162816
SILBasicBlock *contBB = createBasicBlock();
2817-
emitProfilerIncrement(S);
28182817
JumpDest contDest(contBB, Cleanups.getCleanupsDepth(), CleanupLocation(S));
28192818

28202819
LexicalScope switchScope(*this, CleanupLocation(S));

‎test/Profiler/coverage_switch.swift

Lines changed: 121 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,61 @@
11
// RUN: %target-swift-frontend -Xllvm -sil-full-demangle -profile-generate -profile-coverage-mapping -emit-sorted-sil -emit-sil -module-name coverage_switch %s | %FileCheck %s
22
// RUN: %target-swift-frontend -profile-generate -profile-coverage-mapping -emit-ir %s
33

4+
// CHECK-LABEL: sil hidden @$s15coverage_switch2f1yys5Int32VF : $@convention(thin) (Int32) -> ()
5+
// CHECK: string_literal
6+
// CHECK-NEXT: integer_literal $Builtin.Int64, 0
7+
// CHECK-NEXT: integer_literal $Builtin.Int32, 4
8+
// CHECK-NEXT: integer_literal $Builtin.Int32, 0
9+
// CHECK-NEXT: int_instrprof_increment
10+
11+
// CHECK: integer_literal $Builtin.Int32, 1
12+
// CHECK: cmp_eq_Int32
13+
// CHECK: cond_br {{%[0-9]+}}, [[CASE1:bb[0-9]+]], [[NOTCASE1:bb[0-9]+]]
14+
15+
// CHECK: [[NOTCASE1]]
16+
// CHECK: integer_literal $Builtin.Int32, 2
17+
// CHECK: cmp_eq_Int32
18+
// CHECK: cond_br {{%[0-9]+}}, [[CASE2:bb[0-9]+]], [[NOTCASE2:bb[0-9]+]]
19+
20+
// CHECK: [[NOTCASE2]]
21+
// CHECK-NEXT: string_literal
22+
// CHECK-NEXT: integer_literal $Builtin.Int64, 0
23+
// CHECK-NEXT: integer_literal $Builtin.Int32, 4
24+
// CHECK-NEXT: integer_literal $Builtin.Int32, 3
25+
// CHECK-NEXT: int_instrprof_increment
26+
// CHECK-NEXT: br [[DEFAULT:bb[0-9]+]]
27+
28+
// CHECK: [[CASE2]]
29+
// CHECK-NEXT: string_literal
30+
// CHECK-NEXT: integer_literal $Builtin.Int64, 0
31+
// CHECK-NEXT: integer_literal $Builtin.Int32, 4
32+
// CHECK-NEXT: integer_literal $Builtin.Int32, 2
33+
// CHECK-NEXT: int_instrprof_increment
34+
// CHECK-NEXT: br [[DEFAULT]]
35+
36+
// CHECK: [[DEFAULT]]
37+
// CHECK: function_ref @$s15coverage_switch2f1yys5Int32VF : $@convention(thin) (Int32) -> ()
38+
39+
// CHECK: [[CASE1]]
40+
// CHECK-NEXT: string_literal
41+
// CHECK-NEXT: integer_literal $Builtin.Int64, 0
42+
// CHECK-NEXT: integer_literal $Builtin.Int32, 4
43+
// CHECK-NEXT: integer_literal $Builtin.Int32, 1
44+
// CHECK-NEXT: int_instrprof_increment
45+
446
// CHECK-LABEL: sil_coverage_map {{.*}}// coverage_switch.f1
5-
func f1(_ x : Int32) {
6-
switch (x) {
7-
case 1: // CHECK: [[@LINE]]:3 -> [[@LINE+1]]:10 : 2
47+
func f1(_ x : Int32) { // CHECK-NEXT: [[@LINE]]:22 -> [[@LINE+11]]:2 : 0
48+
switch (x) { // CHECK-NEXT: [[@LINE]]:10 -> [[@LINE]]:13 : 0
49+
case 1: // CHECK-NEXT: [[@LINE]]:3 -> [[@LINE+1]]:10 : 1
850
break
9-
case 2: // CHECK: [[@LINE]]:3 -> [[@LINE+1]]:16 : 3
51+
case 2: // CHECK-NEXT: [[@LINE]]:3 -> [[@LINE+1]]:16 : 2
1052
fallthrough
11-
default: // CHECK: [[@LINE]]:3 -> [[@LINE+1]]:14 : (3 + 4)
53+
default: // CHECK-NEXT: [[@LINE]]:3 -> [[@LINE+1]]:14 : (2 + 3)
1254
f1(x - 1)
13-
} // CHECK: [[@LINE]]:4 -> [[@LINE+3]]:2 : 1
55+
} // CHECK-NEXT: [[@LINE]]:4 -> [[@LINE+3]]:2 : ((1 + 2) + 3)
1456

1557
var y = x
16-
}
58+
} // CHECK-NEXT: }
1759

1860
enum Algebraic {
1961
case Type1(Int32, Int32)
@@ -25,41 +67,96 @@ enum Algebraic {
2567
func nop() {}
2668

2769
// CHECK-LABEL: sil_coverage_map {{.*}}// coverage_switch.f2
28-
func f2(_ x : Algebraic) -> Int32 {
29-
switch(x) {
30-
case let .Type1(y, z): // CHECK: [[@LINE]]:3 -> [[@LINE+1]]:10 : 2
70+
func f2(_ x : Algebraic) -> Int32 { // CHECK-NEXT: [[@LINE]]:35 -> [[@LINE+15]]:2 : 0
71+
switch(x) { // CHECK-NEXT: [[@LINE]]:9 -> [[@LINE]]:12 : 0
72+
case let .Type1(y, z): // CHECK-NEXT: [[@LINE]]:3 -> [[@LINE+1]]:10 : 1
3173
nop()
32-
case .Type2(let b): // CHECK: [[@LINE]]:3 -> [[@LINE+2]]:16 : 3
74+
case .Type2(let b): // CHECK-NEXT: [[@LINE]]:3 -> [[@LINE+2]]:16 : 2
3375
nop()
3476
fallthrough
35-
case .Type3: // CHECK: [[@LINE]]:3 -> [[@LINE+3]]:6 : (3 + 4)
36-
if (false) { // CHECK: [[@LINE]]:16 -> [[@LINE+2]]:6 : 5
37-
fallthrough
38-
}
39-
case .Type4: // CHECK: [[@LINE]]:3 -> [[@LINE+1]]:10 : (5 + 6)
77+
case .Type3: // CHECK-NEXT: [[@LINE]]:3 -> [[@LINE+3]]:6 : (2 + 3)
78+
if (false) { // CHECK-NEXT: [[@LINE]]:8 -> [[@LINE]]:15 : (2 + 3)
79+
fallthrough // CHECK-NEXT: [[@LINE-1]]:16 -> [[@LINE+1]]:6 : 4
80+
} // CHECK-NEXT: [[@LINE]]:6 -> [[@LINE]]:6 : ((2 + 3) - 4)
81+
case .Type4: // CHECK-NEXT: [[@LINE]]:3 -> [[@LINE+1]]:10 : (4 + 5)
4082
break
41-
} // CHECK: [[@LINE]]:4 -> [[@LINE+1]]:11 : 1
83+
} // CHECK-NEXT: [[@LINE]]:4 -> [[@LINE+1]]:11 : (((1 + 2) + 3) + 5)
4284
return 0
43-
}
85+
} // CHECK-NEXT: }
4486

4587
public enum Simple {
4688
case First, Second
4789
}
4890

4991
// CHECK-LABEL: sil_coverage_map {{.*}}// coverage_switch.f3
50-
func f3(_ x : Simple) -> Int32 {
51-
switch (x) {
52-
case .First: // CHECK: [[@LINE]]:3 -> [[@LINE+1]]:13 : 2
92+
func f3(_ x : Simple) -> Int32 { // CHECK-NEXT: [[@LINE]]:32 -> [[@LINE+9]]:2 : 0
93+
switch (x) { // CHECK-NEXT: [[@LINE]]:10 -> [[@LINE]]:13 : 0
94+
case .First: // CHECK-NEXT: [[@LINE]]:3 -> [[@LINE+1]]:13 : 1
5395
return 1
54-
case .Second: // CHECK: [[@LINE]]:3 -> [[@LINE+1]]:10 : 3
96+
case .Second: // CHECK-NEXT: [[@LINE]]:3 -> [[@LINE+1]]:10 : 2
5597
break
56-
} // CHECK: [[@LINE]]:4 -> [[@LINE+2]]:11 : 1
98+
} // CHECK: [[@LINE]]:4 -> [[@LINE+2]]:11 : 2
5799

58100
return 0
59-
}
101+
} // CHECK-NEXT: }
60102

61103
f1(3)
62104
f2(Algebraic.Type1(1, 1))
63105
f2(Algebraic.Type2(false))
64106
f2(Algebraic.Type3)
65107
f3(Simple.Second)
108+
109+
// CHECK-LABEL: sil_coverage_map {{.*}}// coverage_switch.f4
110+
func f4(_ x: Int) throws -> Int { // CHECK-NEXT: [[@LINE]]:33 -> [[@LINE+21]]:2 : 0
111+
y: do { // CHECK-NEXT: [[@LINE]]:9 -> [[@LINE+18]]:4 : 0
112+
switch x { // CHECK-NEXT: [[@LINE]]:12 -> [[@LINE]]:13 : 0
113+
case 1, 2, 3: // CHECK-NEXT: [[@LINE]]:5 -> [[@LINE+4]]:18 : 1
114+
if .random() { // CHECK-NEXT: [[@LINE]]:10 -> [[@LINE]]:19 : 1
115+
return 5 // CHECK-NEXT: [[@LINE-1]]:20 -> [[@LINE+1]]:8 : 2
116+
} // CHECK-NEXT: [[@LINE]]:8 -> [[@LINE+1]]:18 : (1 - 2)
117+
fallthrough
118+
case 4: // CHECK-NEXT: [[@LINE]]:5 -> [[@LINE+4]]:8 : ((1 + 3) - 2)
119+
if .random() { // CHECK-NEXT: [[@LINE]]:10 -> [[@LINE]]:19 : ((1 + 3) - 2)
120+
struct E : Error {} // CHECK-NEXT: [[@LINE-1]]:20 -> [[@LINE+2]]:8 : 4
121+
throw E()
122+
} // CHECK-NEXT: [[@LINE]]:8 -> [[@LINE]]:8 : (((1 + 3) - 2) - 4)
123+
default: // CHECK-NEXT: [[@LINE]]:5 -> [[@LINE+3]]:8 : 5
124+
if .random() { // CHECK-NEXT: [[@LINE]]:10 -> [[@LINE]]:19 : 5
125+
break y // CHECK-NEXT: [[@LINE-1]]:20 -> [[@LINE+1]]:8 : 6
126+
} // CHECK-NEXT: [[@LINE]]:8 -> [[@LINE]]:8 : (5 - 6)
127+
}
128+
f1(0) // CHECK-NEXT: [[@LINE-1]]:6 -> [[@LINE+1]]:4 : (((((1 + 3) + 5) - 2) - 4) - 6)
129+
}
130+
return 1 // CHECK-NEXT: [[@LINE-1]]:4 -> [[@LINE]]:11 : ((((1 + 3) + 5) - 2) - 4)
131+
} // CHECK-NEXT: }
132+
133+
// CHECK-LABEL: sil_coverage_map {{.*}}// coverage_switch.f5
134+
func f5(_ x: Simple) -> Int { // CHECK-NEXT: [[@LINE]]:29 -> [[@LINE+7]]:2 : 0
135+
switch x { // CHECK-NEXT: [[@LINE]]:10 -> [[@LINE]]:11 : 0
136+
case .First: // CHECK-NEXT: [[@LINE]]:3 -> [[@LINE+1]]:13 : 1
137+
return 0
138+
case .Second: // CHECK-NEXT: [[@LINE]]:3 -> [[@LINE+1]]:13 : 2
139+
return 1
140+
}
141+
} // CHECK-NEXT: }
142+
143+
// CHECK-LABEL: sil_coverage_map {{.*}}// coverage_switch.f6
144+
func f6(_ x: Simple) -> Int { // CHECK-NEXT: [[@LINE]]:29 -> [[@LINE+5]]:2 : 0
145+
switch x { // CHECK-NEXT: [[@LINE]]:10 -> [[@LINE]]:11 : 0
146+
case .First: return 0 // CHECK-NEXT: [[@LINE]]:3 -> [[@LINE]]:25 : 1
147+
case .Second: return 1 // CHECK-NEXT: [[@LINE]]:3 -> [[@LINE]]:25 : 2
148+
}
149+
} // CHECK-NEXT: }
150+
151+
// CHECK-LABEL: sil_coverage_map {{.*}}// coverage_switch.f7
152+
func f7() -> Int { // CHECK-NEXT: [[@LINE]]:18 -> [[@LINE+10]]:2 : 0
153+
switch .random() ? Simple.First : .Second {
154+
// CHECK-NEXT: [[@LINE-1]]:10 -> [[@LINE-1]]:44 : 0
155+
// CHECK-NEXT: [[@LINE-2]]:22 -> [[@LINE-2]]:34 : 1
156+
// CHECK-NEXT: [[@LINE-3]]:37 -> [[@LINE-3]]:44 : (0 - 1)
157+
case .First: // CHECK-NEXT: [[@LINE]]:3 -> [[@LINE+1]]:13 : 2
158+
return 0
159+
case .Second: // CHECK-NEXT: [[@LINE]]:3 -> [[@LINE+1]]:13 : 3
160+
return 1
161+
}
162+
} // CHECK-NEXT: }

0 commit comments

Comments
 (0)