https://github.com/chapuni created
https://github.com/llvm/llvm-project/pull/125403
None
>From a6d4be0e4b05b411c7160385485cfed0f173965c Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi <geek4ci...@gmail.com>
Date: Sun, 2 Feb 2025 21:55:43 +0900
Subject: [PATCH 1/9] [MC/DC] Update CoverageMapping tests
To resolve the error, rename mcdc-error-nests.cpp -> mcdc-nested-expr.cpp at
first.
- `func_condop`
A corner case that contains close decisions.
- `func_expect`
Uses `__builtin_expect`. (#124565)
- `func_lnot`
Contains logical not(s) `!` among MC/DC binary operators. (#124563)
mcdc-single-cond.cpp is for #95336.
---
.../test/CoverageMapping/mcdc-error-nests.cpp | 10 --
.../test/CoverageMapping/mcdc-nested-expr.cpp | 34 +++++++
.../test/CoverageMapping/mcdc-single-cond.cpp | 97 +++++++++++++++++++
3 files changed, 131 insertions(+), 10 deletions(-)
delete mode 100644 clang/test/CoverageMapping/mcdc-error-nests.cpp
create mode 100644 clang/test/CoverageMapping/mcdc-nested-expr.cpp
create mode 100644 clang/test/CoverageMapping/mcdc-single-cond.cpp
diff --git a/clang/test/CoverageMapping/mcdc-error-nests.cpp
b/clang/test/CoverageMapping/mcdc-error-nests.cpp
deleted file mode 100644
index 3add2b9ccd3fb3..00000000000000
--- a/clang/test/CoverageMapping/mcdc-error-nests.cpp
+++ /dev/null
@@ -1,10 +0,0 @@
-// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++11 -fcoverage-mcdc
-fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping
-emit-llvm-only %s 2>&1| FileCheck %s
-
-// "Split-nest" -- boolean expressions within boolean expressions.
-extern bool bar(bool);
-bool func_split_nest(bool a, bool b, bool c, bool d, bool e, bool f, bool g) {
- bool res = a && b && c && bar(d && e) && f && g;
- return bar(res);
-}
-
-// CHECK: warning: unsupported MC/DC boolean expression; contains an operation
with a nested boolean expression.
diff --git a/clang/test/CoverageMapping/mcdc-nested-expr.cpp
b/clang/test/CoverageMapping/mcdc-nested-expr.cpp
new file mode 100644
index 00000000000000..bb82873e3b600d
--- /dev/null
+++ b/clang/test/CoverageMapping/mcdc-nested-expr.cpp
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++11 -fcoverage-mcdc
-fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping
-emit-llvm-only %s 2> %t.stderr.txt | FileCheck %s
+// RUN: FileCheck %s --check-prefix=WARN < %t.stderr.txt
+
+// "Split-nest" -- boolean expressions within boolean expressions.
+extern bool bar(bool);
+// CHECK: func_split_nest{{.*}}:
+bool func_split_nest(bool a, bool b, bool c, bool d, bool e, bool f, bool g) {
+ // WARN: :[[@LINE+1]]:14: warning: unsupported MC/DC boolean expression;
contains an operation with a nested boolean expression.
+ bool res = a && b && c && bar(d && e) && f && g;
+ return bar(res);
+}
+
+// The inner expr begins with the same Loc as the outer expr
+// CHECK: func_condop{{.*}}:
+bool func_condop(bool a, bool b, bool c) {
+ // WARN: :[[@LINE+1]]:10: warning: unsupported MC/DC boolean expression;
contains an operation with a nested boolean expression.
+ return (a && b ? true : false) && c;
+}
+
+// __builtin_expect
+// Treated as parentheses.
+// CHECK: func_expect{{.*}}:
+bool func_expect(bool a, bool b, bool c) {
+ // WARN: :[[@LINE+1]]:10: warning: unsupported MC/DC boolean expression;
contains an operation with a nested boolean expression.
+ return a || __builtin_expect(b && c, true);
+}
+
+// LNot among BinOp(s)
+// Doesn't split exprs.
+// CHECK: func_lnot{{.*}}:
+bool func_lnot(bool a, bool b, bool c, bool d) {
+ // WARN: :[[@LINE+1]]:10: warning: unsupported MC/DC boolean expression;
contains an operation with a nested boolean expression.
+ return !(a || b) && !(c && d);
+}
diff --git a/clang/test/CoverageMapping/mcdc-single-cond.cpp
b/clang/test/CoverageMapping/mcdc-single-cond.cpp
new file mode 100644
index 00000000000000..b1eeea879e5217
--- /dev/null
+++ b/clang/test/CoverageMapping/mcdc-single-cond.cpp
@@ -0,0 +1,97 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++11 -fcoverage-mcdc
-fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping
-disable-llvm-passes -emit-llvm -o %t2.ll %s | FileCheck %s
--check-prefixes=MM,MM2
+// RUN: FileCheck %s --check-prefixes=LL,LL2 < %t2.ll
+
+// LL: define{{.+}}func_cond{{.+}}(
+// MM: func_cond{{.*}}:
+int func_cond(bool a, bool b) {
+ // %mcdc.addr* are emitted by static order.
+ // LL: %[[MA:mcdc.addr.*]] = alloca i32, align 4
+ // LL: call void @llvm.instrprof.mcdc.parameters(ptr @[[PROFN:.+]], i64
[[#H:]], i32 [[#BS:]])
+ int count = 0;
+ if (a)
+ // NB=2 Single cond
+ // MM2-NOT: Decision
+ ++count;
+ if (a ? true : false)
+ // NB=2,2 Wider decision comes first.
+ // MA2 has C:2
+ // MA3 has C:1
+ ++count;
+ if (a && b ? true : false)
+ // NB=2,3 Wider decision comes first.
+ // MM2: Decision,File 0, [[@LINE-2]]:7 -> [[#L:@LINE-2]]:13 = M:[[#I:3]],
C:2
+ // MM: Branch,File 0, [[#L]]:7 -> [[#L]]:8 = #6, (#0 - #6) [1,2,0]
+ // MM: Branch,File 0, [[#L]]:12 -> [[#L]]:13 = #7, (#6 - #7) [2,0,0]
+ // LL: store i32 0, ptr %[[MA]], align 4
+ // LL: = load i32, ptr %[[MA]], align 4
+ // LL: store i32 %{{.+}}, ptr %[[MA]], align 4
+ // LL: = load i32, ptr %[[MA]], align 4
+ // LL: store i32 %{{.+}}, ptr %[[MA]], align 4
+ // LL2: call void @llvm.instrprof.mcdc.tvbitmap.update(ptr @[[PROFN]],
i64 [[#H]], i32 [[#B:0]], ptr %[[MA]])
+ ++count;
+ while (a || true) {
+ // NB=3 BinOp only
+ // MM: Decision,File 0, [[@LINE-2]]:10 -> [[#L:@LINE-2]]:19 =
M:[[#I:I+3]], C:2
+ // MM: Branch,File 0, [[#L]]:10 -> [[#L]]:11 = (#0 - #9), #9 [1,0,2]
+ // MM: Branch,File 0, [[#L]]:15 -> [[#L]]:19 = (#9 - #10), 0 [2,0,0]
+ // LL: store i32 0, ptr %[[MA]], align 4
+ // LL: = load i32, ptr %[[MA]], align 4
+ // LL: store i32 %{{.+}}, ptr %[[MA]], align 4
+ // LL: = load i32, ptr %[[MA]], align 4
+ // LL: store i32 %{{.+}}, ptr %[[MA]], align 4
+ // LL2: call void @llvm.instrprof.mcdc.tvbitmap.update(ptr @[[PROFN]],
i64 [[#H]], i32 [[#B:B+3]], ptr %[[MA]])
+ ++count;
+ break;
+ }
+ while (a || true ? false : true) {
+ // Wider decision comes first.
+ // MM2: Decision,File 0, [[@LINE-2]]:10 -> [[#L:@LINE-2]]:19 =
M:[[#I:I+3]], C:2
+ // MM: Branch,File 0, [[#L]]:10 -> [[#L]]:11 = ((#0 + #11) - #13), #13
[1,0,2]
+ // MM: Branch,File 0, [[#L]]:15 -> [[#L]]:19 = (#13 - #14), 0 [2,0,0]
+ // LL: store i32 0, ptr %[[MA]], align 4
+ // LL: = load i32, ptr %[[MA]], align 4
+ // LL: store i32 %{{.+}}, ptr %[[MA]], align 4
+ // LL: = load i32, ptr %[[MA]], align 4
+ // LL: store i32 %{{.+}}, ptr %[[MA]], align 4
+ // LL: call void @llvm.instrprof.mcdc.tvbitmap.update(ptr @[[PROFN]],
i64 [[#H]], i32 [[#B:B+3]], ptr %[[MA]])
+ ++count;
+ }
+ do {
+ ++count;
+ } while (a && false);
+ // BinOp only
+ // MM: Decision,File 0, [[@LINE-2]]:12 -> [[#L:@LINE-2]]:22 =
M:[[#I:I+3]], C:2
+ // MM: Branch,File 0, [[#L]]:12 -> [[#L]]:13 = #16, ((#0 + #15) - #16)
[1,2,0]
+ // MM: Branch,File 0, [[#L]]:17 -> [[#L]]:22 = 0, (#16 - #17) [2,0,0]
+ // LL: store i32 0, ptr %[[MA]], align 4
+ // LL: = load i32, ptr %[[MA]], align 4
+ // LL: store i32 %{{.+}}, ptr %[[MA]], align 4
+ // LL: = load i32, ptr %[[MA]], align 4
+ // LL: store i32 %{{.+}}, ptr %[[MA]], align 4
+ // LL2: call void @llvm.instrprof.mcdc.tvbitmap.update(ptr @[[PROFN]], i64
[[#H]], i32 [[#B:B+3]], ptr %[[MA]])
+ do {
+ ++count;
+ } while (a && false ? true : false);
+ // Wider decision comes first.
+ // MM2: Decision,File 0, [[@LINE-2]]:12 -> [[#L:@LINE-2]]:22 = M:15, C:2
+ // MM: Branch,File 0, [[#L]]:12 -> [[#L]]:13 = #20, ((#0 + #18) - #20)
[1,2,0]
+ // MM: Branch,File 0, [[#L]]:17 -> [[#L]]:22 = 0, (#20 - #21) [2,0,0]
+ // LL: store i32 0, ptr %[[MA]], align 4
+ // LL: = load i32, ptr %[[MA]], align 4
+ // LL: store i32 %{{.+}}, ptr %[[MA]], align 4
+ // LL: = load i32, ptr %[[MA]], align 4
+ // LL: store i32 %{{.+}}, ptr %[[MA]], align 4
+ // LL: call void @llvm.instrprof.mcdc.tvbitmap.update(ptr @[[PROFN]], i64
[[#H]], i32 [[#B:B+3]], ptr %[[MA]])
+ // FIXME: Confirm (B+3==BS)
+ for (int i = 0; i < (a ? 2 : 1); ++i) {
+ // Simple nested decision (different column)
+ // MM2-NOT: Decision
+ // LL2-NOT: call void @llvm.instrprof.mcdc.tvbitmap.update
+ ++count;
+ }
+ for (int i = 0; i >= 4 ? false : true; ++i) {
+ // Wider decision comes first.
+ ++count;
+ }
+ return count;
+}
>From 06d0d51dce35916ebabcbb219c2d868df375e601 Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi <geek4ci...@gmail.com>
Date: Sun, 2 Feb 2025 21:58:41 +0900
Subject: [PATCH 2/9] [MC/DC] Make covmap tolerant of nested Decisions
CoverageMappingWriter reorders `Region`s by `endLoc DESC` to
prioritize wider `Decision` with the same `startLoc`.
In `llvm-cov`, tweak seeking Decisions by reversal order to find
smaller Decision first.
---
clang/test/CoverageMapping/ctor.cpp | 4 ++--
clang/test/CoverageMapping/includehell.cpp | 18 +++++++++---------
clang/test/CoverageMapping/macros.c | 6 +++---
.../ProfileData/Coverage/CoverageMapping.cpp | 4 ++--
.../Coverage/CoverageMappingWriter.cpp | 9 ++++++++-
5 files changed, 24 insertions(+), 17 deletions(-)
diff --git a/clang/test/CoverageMapping/ctor.cpp
b/clang/test/CoverageMapping/ctor.cpp
index 1cf12cd738c2cc..191f73f5693e19 100644
--- a/clang/test/CoverageMapping/ctor.cpp
+++ b/clang/test/CoverageMapping/ctor.cpp
@@ -11,8 +11,8 @@ class B: public A {
int c;
B(int x, int y)
: A(x), // CHECK: File 0, [[@LINE]]:7 -> [[@LINE]]:11
= #0
- // CHECK-NEXT: File 0, [[@LINE+1]]:7 ->
[[@LINE+1]]:13 = #0
- b(x == 0? 1: 2), // CHECK-NEXT: File 0, [[@LINE]]:7 -> [[@LINE]]:19
= #0
+ // CHECK-NEXT: File 0, [[@LINE+1]]:7 ->
[[@LINE+1]]:19 = #0
+ b(x == 0? 1: 2), // CHECK-NEXT: File 0, [[@LINE]]:7 -> [[@LINE]]:13
= #0
// CHECK-NEXT: Branch,File 0, [[@LINE-1]]:7 ->
[[@LINE-1]]:13 = #1, (#0 - #1)
// CHECK-NEXT: Gap,File 0, [[@LINE-2]]:14 ->
[[@LINE-2]]:15 = #1
// CHECK-NEXT: File 0, [[@LINE-3]]:15 ->
[[@LINE-3]]:16 = #1
diff --git a/clang/test/CoverageMapping/includehell.cpp
b/clang/test/CoverageMapping/includehell.cpp
index 09e03e77799d56..3c67672fabc0fc 100644
--- a/clang/test/CoverageMapping/includehell.cpp
+++ b/clang/test/CoverageMapping/includehell.cpp
@@ -28,14 +28,14 @@ int main() {
// CHECK-MAIN-NEXT: Expansion,File [[MAIN]], 6:12 -> 6:35 = #0
// CHECK-MAIN-NEXT: File [[MAIN]], 6:35 -> 10:33 = #1
// CHECK-MAIN-NEXT: Expansion,File [[MAIN]], 8:14 -> 8:29 = #1
-// CHECK-MAIN-NEXT: Expansion,File [[MAIN]], 10:12 -> 10:33 = #1
+// CHECK-MAIN-NEXT: Expansion,File [[MAIN]], 10:12 -> 10:33 = #0
// CHECK-MAIN-NEXT: Expansion,File [[MAIN]], 12:12 -> 12:35 = #0
// CHECK-MAIN-NEXT: File [[MAIN]], 12:35 -> 14:33 = #5
// CHECK-MAIN-NEXT: Expansion,File [[MAIN]], 13:14 -> 13:29 = #5
-// CHECK-MAIN-NEXT: Expansion,File [[MAIN]], 14:12 -> 14:33 = #5
+// CHECK-MAIN-NEXT: Expansion,File [[MAIN]], 14:12 -> 14:33 = #0
// CHECK-MAIN-NEXT: Expansion,File [[MAIN]], 16:12 -> 16:35 = #0
// CHECK-MAIN-NEXT: File [[MAIN]], 16:35 -> 17:33 = #9
-// CHECK-MAIN-NEXT: Expansion,File [[MAIN]], 17:12 -> 17:33 = #9
+// CHECK-MAIN-NEXT: Expansion,File [[MAIN]], 17:12 -> 17:33 = #0
// CHECK-START: File [[START1:[0-9]]], 1:1 -> 5:1 = #0
// CHECK-START: File [[START1]], 4:17 -> 4:22 = (#0 + #1)
@@ -65,15 +65,15 @@ int main() {
// CHECK-CODE: File [[CODE2]], 9:11 -> 11:2 = #7
// CHECK-CODE: File [[CODE2]], 11:8 -> 13:2 = (#5 - #7)
-// CHECK-END: File [[END1:[0-9]]], 1:1 -> 3:2 = #1
-// CHECK-END: File [[END1]], 1:1 -> 6:1 = #0
+// CHECK-END: File [[END1:[0-9]]], 1:1 -> 6:1 = #0
+// CHECK-END: File [[END1]], 1:1 -> 3:2 = #1
// CHECK-END: File [[END1]], 5:5 -> 5:9 = #0
// CHECK-END: File [[END1]], 5:11 -> 5:16 = #4
-// CHECK-END: File [[END2:[0-9]]], 1:1 -> 3:2 = #5
-// CHECK-END: File [[END2]], 1:1 -> 6:1 = #0
+// CHECK-END: File [[END2:[0-9]]], 1:1 -> 6:1 = #0
+// CHECK-END: File [[END2]], 1:1 -> 3:2 = #5
// CHECK-END: File [[END2]], 5:5 -> 5:9 = #0
// CHECK-END: File [[END2]], 5:11 -> 5:16 = #8
-// CHECK-END: File [[END3:[0-9]]], 1:1 -> 3:2 = #9
-// CHECK-END: File [[END3]], 1:1 -> 6:1 = #0
+// CHECK-END: File [[END3:[0-9]]], 1:1 -> 6:1 = #0
+// CHECK-END: File [[END3]], 1:1 -> 3:2 = #9
// CHECK-END: File [[END3]], 5:5 -> 5:9 = #0
// CHECK-END: File [[END3]], 5:11 -> 5:16 = #10
diff --git a/clang/test/CoverageMapping/macros.c
b/clang/test/CoverageMapping/macros.c
index fcf21170ef135c..00139f33229d57 100644
--- a/clang/test/CoverageMapping/macros.c
+++ b/clang/test/CoverageMapping/macros.c
@@ -80,12 +80,12 @@ void func7(void) { // CHECK-NEXT: File 0, [[@LINE]]:18 ->
[[@LINE+6]]:2 = #0
int kk,ll; // CHECK-NEXT: File 0, [[@LINE+1]]:7 -> [[@LINE+1]]:8 = #0
if (k) // CHECK-NEXT: Branch,File 0, [[@LINE]]:7 -> [[@LINE]]:8 =
#1
m(k); // CHECK-NEXT: Gap,File 0, [[@LINE-1]]:9 -> [[@LINE]]:5 = #1
- else // CHECK-NEXT: Expansion,File 0, [[@LINE-1]]:5 ->
[[@LINE-1]]:6 = #1
+ else // CHECK-NEXT: Expansion,File 0, [[@LINE-1]]:5 ->
[[@LINE-1]]:6 = #0
l = m(l); // CHECK-NEXT: Gap,File 0, [[@LINE-2]]:7 -> [[@LINE]]:5 =
(#0 - #1)
} // CHECK-NEXT: File 0, [[@LINE-1]]:5 -> [[@LINE-1]]:10 =
(#0 - #1)
// CHECK-NEXT: Expansion,File 0, [[@LINE-2]]:9 ->
[[@LINE-2]]:10 = (#0 - #1)
- // CHECK-NEXT: File 1, [[@LINE-9]]:14 -> [[@LINE-9]]:17 = #1
- // CHECK-NEXT: File 1, [[@LINE-10]]:14 -> [[@LINE-10]]:18 =
#0
+ // CHECK-NEXT: File 1, [[@LINE-9]]:14 -> [[@LINE-9]]:18 = #0
+ // CHECK-NEXT: File 1, [[@LINE-10]]:14 -> [[@LINE-10]]:17 =
#1
// CHECK-NEXT: File 2, [[@LINE-11]]:14 -> [[@LINE-11]]:17 =
(#0 - #1)
// CHECK-NEXT: File 2, [[@LINE-12]]:14 -> [[@LINE-12]]:15 =
(#0 - #1)
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
index c39585681911a8..3205863331c913 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -800,7 +800,7 @@ class MCDCDecisionRecorder {
std::optional<DecisionAndBranches>
processBranch(const CounterMappingRegion &Branch) {
// Seek each Decision and apply Region to it.
- for (auto DecisionIter = Decisions.begin(), DecisionEnd = Decisions.end();
+ for (auto DecisionIter = Decisions.rbegin(), DecisionEnd =
Decisions.rend();
DecisionIter != DecisionEnd; ++DecisionIter)
switch (DecisionIter->addBranch(Branch)) {
case DecisionRecord::NotProcessed:
@@ -811,7 +811,7 @@ class MCDCDecisionRecorder {
DecisionAndBranches Result =
std::make_pair(DecisionIter->DecisionRegion,
std::move(DecisionIter->MCDCBranches));
- Decisions.erase(DecisionIter); // No longer used.
+ Decisions.erase(std::next(DecisionIter).base()); // No longer used.
return Result;
}
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
b/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
index 12b1687af69db3..e0a1aae2a8cc19 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
@@ -174,7 +174,14 @@ void CoverageMappingWriter::write(raw_ostream &OS) {
: 2 * Kind);
};
- return getKindKey(LHS.Kind) < getKindKey(RHS.Kind);
+ auto LHSKindKey = getKindKey(LHS.Kind);
+ auto RHSKindKey = getKindKey(RHS.Kind);
+ if (LHSKindKey != RHSKindKey)
+ return LHSKindKey < RHSKindKey;
+
+ // Compares endLoc in descending order,
+ // to prioritize wider Regions with the same startLoc.
+ return LHS.endLoc() > RHS.endLoc();
});
// Write out the fileid -> filename mapping.
>From d414f29ed8732c77fdcd05cc3b066e9ee0d9de07 Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi <geek4ci...@gmail.com>
Date: Sun, 2 Feb 2025 21:59:33 +0900
Subject: [PATCH 3/9] [MC/DC] Refactor MCDC::State::Decision. NFC.
Introduce `ID` and `InvalidID`. Then `DecisionByStmt` can have three
states.
* Not assigned if the Stmt(Expr) doesn't exist.
* When `DecisionByStmt[Expr]` exists:
* Invalid and should be ignored if `ID == Invalid`.
* Valid if `ID != Invalid`. Other member will be filled in the
Mapper.
---
clang/lib/CodeGen/CodeGenPGO.cpp | 2 +-
clang/lib/CodeGen/CoverageMappingGen.cpp | 6 ++----
clang/lib/CodeGen/MCDCState.h | 16 +++++++++++++++-
3 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/clang/lib/CodeGen/CodeGenPGO.cpp b/clang/lib/CodeGen/CodeGenPGO.cpp
index 792373839107f0..0331ff83e633f7 100644
--- a/clang/lib/CodeGen/CodeGenPGO.cpp
+++ b/clang/lib/CodeGen/CodeGenPGO.cpp
@@ -310,7 +310,7 @@ struct MapRegionCounters : public
RecursiveASTVisitor<MapRegionCounters> {
}
// Otherwise, allocate the Decision.
- MCDCState.DecisionByStmt[BinOp].BitmapIdx = 0;
+ MCDCState.DecisionByStmt[BinOp].ID = MCDCState.DecisionByStmt.size();
}
return true;
}
diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp
b/clang/lib/CodeGen/CoverageMappingGen.cpp
index f09157771d2b5c..4dbc0c70e34d60 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -2197,10 +2197,8 @@ struct CounterCoverageMappingBuilder
// Update the state for CodeGenPGO
assert(MCDCState.DecisionByStmt.contains(E));
- MCDCState.DecisionByStmt[E] = {
- MCDCState.BitmapBits, // Top
- std::move(Builder.Indices),
- };
+ MCDCState.DecisionByStmt[E].update(MCDCState.BitmapBits, // Top
+ std::move(Builder.Indices));
auto DecisionParams = mcdc::DecisionParameters{
MCDCState.BitmapBits += NumTVs, // Tail
diff --git a/clang/lib/CodeGen/MCDCState.h b/clang/lib/CodeGen/MCDCState.h
index e0dd28ff90ed12..0b6f5f28235f43 100644
--- a/clang/lib/CodeGen/MCDCState.h
+++ b/clang/lib/CodeGen/MCDCState.h
@@ -16,6 +16,8 @@
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ProfileData/Coverage/MCDCTypes.h"
+#include <cassert>
+#include <limits>
namespace clang {
class Stmt;
@@ -30,8 +32,20 @@ struct State {
unsigned BitmapBits = 0;
struct Decision {
+ using IndicesTy = llvm::SmallVector<std::array<int, 2>>;
+ static constexpr auto InvalidID = std::numeric_limits<unsigned>::max();
+
unsigned BitmapIdx;
- llvm::SmallVector<std::array<int, 2>> Indices;
+ IndicesTy Indices;
+ unsigned ID = InvalidID;
+
+ bool isValid() const { return ID != InvalidID; }
+
+ void update(unsigned I, IndicesTy &&X) {
+ assert(ID != InvalidID);
+ BitmapIdx = I;
+ Indices = std::move(X);
+ }
};
llvm::DenseMap<const Stmt *, Decision> DecisionByStmt;
>From 2b37ea400809fd57f2b71b997d5dca622113a422 Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi <geek4ci...@gmail.com>
Date: Sun, 2 Feb 2025 22:00:22 +0900
Subject: [PATCH 4/9] [MC/DC] Refactor MCDCCoverageBuilder. NFC.
- Get rid of the old `DecisionStack` and dissolve it into push/pop
`CurCondIDs` in `VisitBin`, since `VisitBin` is recursive.
- Introduce the new `DecisionStack` with `DecisionState` to handle the
current `Decision` in nested `Decision`s.
- The stack has the sentinel that has `DecisionExpr = nullptr`.
- Split out `checkDecisionRootOrPush` from `pushAndAssignIDs` for
non-BinOp. It assigns `CondID` to `E` (instead of assignment LHS
in `pushAndAssignIDs`).
- The stack is manupilated at the top Decision operator in `VisitBin`.
- The stack grows at the entrance of the Decision with the initial
state.
- In the same level in `VisitBin`, the stack is popped and the
`Decision` record is emitted.
- Introduce `DecisionEndToSince` to sweep `MCDCBranch`es partially in
`cancelDecision`.
---
clang/lib/CodeGen/CoverageMappingGen.cpp | 309 ++++++++++++++---------
1 file changed, 187 insertions(+), 122 deletions(-)
diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp
b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 4dbc0c70e34d60..3a281fd39b4bcb 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -750,41 +750,48 @@ struct MCDCCoverageBuilder {
private:
CodeGenModule &CGM;
-
- llvm::SmallVector<mcdc::ConditionIDs> DecisionStack;
MCDC::State &MCDCState;
- const Stmt *DecisionStmt = nullptr;
- mcdc::ConditionID NextID = 0;
- bool NotMapped = false;
- /// Represent a sentinel value as a pair of final decisions for the bottom
- // of DecisionStack.
- static constexpr mcdc::ConditionIDs DecisionStackSentinel{-1, -1};
+ struct DecisionState {
+ /// The root Decision
+ const Expr *DecisionExpr = nullptr;
- /// Is this a logical-AND operation?
- bool isLAnd(const BinaryOperator *E) const {
- return E->getOpcode() == BO_LAnd;
- }
+ /// Pair of Destination conditions [false, true]
+ /// -1, the final decision at the initial state.
+ /// Modify before/after the traversal of BinOp LHS.
+ mcdc::ConditionIDs CurCondIDs = {-1, -1};
+
+ /// The ID to be assigned, and total number of conditions.
+ mcdc::ConditionID NextID = 0;
+
+ /// false if the Decision is recognized but should be ignored.
+ bool Active = false;
+
+ DecisionState() = default;
+ DecisionState(const Expr *DecisionExpr, bool Valid)
+ : DecisionExpr(DecisionExpr), Active(Valid) {}
+ };
+
+ /// The bottom [0] is the sentinel.
+ /// - DecisionExpr = nullptr, doesn't match to any Expr(s).
+ /// - Active = false
+ llvm::SmallVector<DecisionState, 2> DecisionStack;
+
+ /// <Index of Decision, Index of Since>, on SourceRegions.
+ /// Used for restoring MCDCBranch=>Branch.
+ llvm::DenseMap<unsigned, unsigned> DecisionEndToSince;
public:
MCDCCoverageBuilder(CodeGenModule &CGM, MCDC::State &MCDCState)
- : CGM(CGM), DecisionStack(1, DecisionStackSentinel),
- MCDCState(MCDCState) {}
-
- /// Return whether the build of the control flow map is at the top-level
- /// (root) of a logical operator nest in a boolean expression prior to the
- /// assignment of condition IDs.
- bool isIdle() const { return (NextID == 0 && !NotMapped); }
+ : CGM(CGM), MCDCState(MCDCState), DecisionStack(1) {}
- /// Return whether any IDs have been assigned in the build of the control
- /// flow map, indicating that the map is being generated for this boolean
- /// expression.
- bool isBuilding() const { return (NextID > 0); }
+ bool isActive() const { return DecisionStack.back().Active; }
/// Set the given condition's ID.
void setCondID(const Expr *Cond, mcdc::ConditionID ID) {
- MCDCState.BranchByStmt[CodeGenFunction::stripCond(Cond)] = {ID,
- DecisionStmt};
+ assert(isActive());
+ MCDCState.BranchByStmt[CodeGenFunction::stripCond(Cond)] = {
+ ID, DecisionStack.back().DecisionExpr};
}
/// Return the ID of a given condition.
@@ -797,83 +804,99 @@ struct MCDCCoverageBuilder {
}
/// Return the LHS Decision ([0,0] if not set).
- const mcdc::ConditionIDs &back() const { return DecisionStack.back(); }
+ auto &getCurCondIDs() { return DecisionStack.back().CurCondIDs; }
- /// Push the binary operator statement to track the nest level and assign IDs
- /// to the operator's LHS and RHS. The RHS may be a larger subtree that is
- /// broken up on successive levels.
- void pushAndAssignIDs(const BinaryOperator *E) {
- if (!CGM.getCodeGenOpts().MCDCCoverage)
+ void swapConds() {
+ if (!isActive())
return;
- // If binary expression is disqualified, don't do mapping.
- if (!isBuilding() &&
- !MCDCState.DecisionByStmt.contains(CodeGenFunction::stripCond(E)))
- NotMapped = true;
+ std::swap(getCurCondIDs()[false], getCurCondIDs()[true]);
+ }
- // Don't go any further if we don't need to map condition IDs.
- if (NotMapped)
+ void checkDecisionRootOrPush(const Expr *E) {
+ // Don't push the new entry unless MC/DC Coverage.
+ if (!CGM.getCodeGenOpts().MCDCCoverage) {
+ assert(!isActive() && "The setinel should tell 'not Active'");
return;
-
- if (NextID == 0) {
- DecisionStmt = E;
- assert(MCDCState.DecisionByStmt.contains(E));
}
- const mcdc::ConditionIDs &ParentDecision = DecisionStack.back();
+ auto *SC = CodeGenFunction::stripCond(E);
+ if (getCondID(SC) >= 0)
+ return;
- // If the operator itself has an assigned ID, this means it represents a
- // larger subtree. In this case, assign that ID to its LHS node. Its RHS
- // will receive a new ID below. Otherwise, assign ID+1 to LHS.
- if (MCDCState.BranchByStmt.contains(CodeGenFunction::stripCond(E)))
- setCondID(E->getLHS(), getCondID(E));
- else
- setCondID(E->getLHS(), NextID++);
+ // Push the new entry at the Decision root.
+ if (auto DI = MCDCState.DecisionByStmt.find(SC);
+ DI != MCDCState.DecisionByStmt.end()) {
+ auto &StackTop = DecisionStack.emplace_back(SC, DI->second.isValid());
- // Assign a ID+1 for the RHS.
- mcdc::ConditionID RHSid = NextID++;
- setCondID(E->getRHS(), RHSid);
+ // The root expr (possibly BinOp) may have 1st ID.
+ // It will be propagated to the most Left hand.
+ if (isActive() && getCondID(SC) < 0)
+ setCondID(SC, StackTop.NextID++);
+ return;
+ }
- // Push the LHS decision IDs onto the DecisionStack.
- if (isLAnd(E))
- DecisionStack.push_back({ParentDecision[false], RHSid});
- else
- DecisionStack.push_back({RHSid, ParentDecision[true]});
+ assert((!isActive() || DecisionStack.back().NextID > 0) &&
+ "Should be Active and after assignments");
}
- /// Pop and return the LHS Decision ([0,0] if not set).
- mcdc::ConditionIDs pop() {
- if (!CGM.getCodeGenOpts().MCDCCoverage || NotMapped)
- return DecisionStackSentinel;
+ /// Push the binary operator statement to track the nest level and assign IDs
+ /// to the operator's LHS and RHS. The RHS may be a larger subtree that is
+ /// broken up on successive levels.
+ std::pair<mcdc::ConditionID, mcdc::ConditionID>
+ pushAndAssignIDs(const BinaryOperator *E) {
+ if (!CGM.getCodeGenOpts().MCDCCoverage)
+ return {-1, -1};
+
+ checkDecisionRootOrPush(E);
+ if (!isActive())
+ return {-1, -1};
+
+ auto &StackTop = DecisionStack.back();
+
+ // LHS inherits the ID from the parent.
+ mcdc::ConditionID LHSid = getCondID(E);
+ assert(LHSid >= 0);
+ setCondID(E->getLHS(), LHSid);
+
+ // Assign a ID+1 for the RHS.
+ mcdc::ConditionID RHSid = StackTop.NextID++;
+ setCondID(E->getRHS(), RHSid);
- assert(DecisionStack.size() > 1);
- return DecisionStack.pop_back_val();
+ return {LHSid, RHSid};
}
- /// Return the total number of conditions and reset the state. The number of
+ /// Return the total number of conditions and rewind the state. The number of
/// conditions is zero if the expression isn't mapped.
- unsigned getTotalConditionsAndReset(const BinaryOperator *E) {
- if (!CGM.getCodeGenOpts().MCDCCoverage)
- return 0;
-
- assert(!isIdle());
- assert(DecisionStack.size() == 1);
+ unsigned getTotalConditionsAndPop(const Expr *E) {
+ auto &StackTop = DecisionStack.back();
- // Reset state if not doing mapping.
- if (NotMapped) {
- NotMapped = false;
- assert(NextID == 0);
+ // Root?
+ if (StackTop.DecisionExpr != E)
return 0;
- }
-
- // Set number of conditions and reset.
- unsigned TotalConds = NextID;
- // Reset ID back to beginning.
- NextID = 0;
+ assert(StackTop.CurCondIDs[false] == -1 &&
+ StackTop.CurCondIDs[true] == -1 &&
+ "The root shouldn't depend on others.");
+ // Set number of conditions and pop.
+ unsigned TotalConds = (StackTop.Active ? StackTop.NextID : 0);
+ DecisionStack.pop_back();
+ assert(!DecisionStack.empty() && "Sentiel?");
return TotalConds;
}
+
+ void addDecisionRegionRange(unsigned Since, unsigned End) {
+ DecisionEndToSince[End] = Since;
+ }
+
+ /// Returns "Since" index corresponding to the arg Idx.
+ unsigned skipSourceRegionIndexForDecisions(unsigned Idx) {
+ auto I = DecisionEndToSince.find(Idx);
+ assert(I != DecisionEndToSince.end());
+ assert(I->second <= Idx);
+ return I->second;
+ }
};
/// A StmtVisitor that creates coverage mapping regions which map
@@ -2169,19 +2192,39 @@ struct CounterCoverageMappingBuilder
createBranchRegion(E->getCond(), TrueCount, FalseCount);
}
- void createOrCancelDecision(const BinaryOperator *E, unsigned Since) {
- unsigned NumConds = MCDCBuilder.getTotalConditionsAndReset(E);
+ inline unsigned findMCDCBranchesInSourceRegion(
+ unsigned Since, std::function<void(SourceMappingRegion &SR)> CB) {
+ unsigned I = SourceRegions.size() - 1;
+ unsigned Count = 0;
+ while (I >= Since) {
+ auto &SR = SourceRegions[I];
+ if (SR.isMCDCDecision()) {
+ // Skip a sub Decision and don't modify records in it.
+ I = MCDCBuilder.skipSourceRegionIndexForDecisions(I);
+ } else if (SR.isMCDCBranch()) {
+ ++Count;
+ CB(SR);
+ }
+
+ if (I-- <= Since)
+ break;
+ }
+
+ return Count;
+ }
+
+ void createOrCancelDecision(const Expr *E, unsigned Since) {
+ auto *SC = CodeGenFunction::stripCond(E);
+ auto NumConds = MCDCBuilder.getTotalConditionsAndPop(SC);
if (NumConds == 0)
return;
// Extract [ID, Conds] to construct the graph.
llvm::SmallVector<mcdc::ConditionIDs> CondIDs(NumConds);
- for (const auto &SR : ArrayRef(SourceRegions).slice(Since)) {
- if (SR.isMCDCBranch()) {
- auto [ID, Conds] = SR.getMCDCBranchParams();
- CondIDs[ID] = Conds;
- }
- }
+ findMCDCBranchesInSourceRegion(Since, [&](const SourceMappingRegion &SR) {
+ auto [ID, Conds] = SR.getMCDCBranchParams();
+ CondIDs[ID] = Conds;
+ });
// Construct the graph and calculate `Indices`.
mcdc::TVIdxBuilder Builder(CondIDs);
@@ -2191,14 +2234,14 @@ struct CounterCoverageMappingBuilder
if (NumTVs > MaxTVs) {
// NumTVs exceeds MaxTVs -- warn and cancel the Decision.
- cancelDecision(E, Since, NumTVs, MaxTVs);
+ cancelDecision(SC, Since, NumTVs, MaxTVs, NumConds);
return;
}
// Update the state for CodeGenPGO
- assert(MCDCState.DecisionByStmt.contains(E));
- MCDCState.DecisionByStmt[E].update(MCDCState.BitmapBits, // Top
- std::move(Builder.Indices));
+ assert(MCDCState.DecisionByStmt.contains(SC));
+ MCDCState.DecisionByStmt[SC].update(MCDCState.BitmapBits, // Top
+ std::move(Builder.Indices));
auto DecisionParams = mcdc::DecisionParameters{
MCDCState.BitmapBits += NumTVs, // Tail
@@ -2207,28 +2250,39 @@ struct CounterCoverageMappingBuilder
// Create MCDC Decision Region.
createDecisionRegion(E, DecisionParams);
+
+ // Memo
+ assert(SourceRegions.back().isMCDCDecision());
+ MCDCBuilder.addDecisionRegionRange(Since, SourceRegions.size() - 1);
}
// Warn and cancel the Decision.
- void cancelDecision(const BinaryOperator *E, unsigned Since, int NumTVs,
- int MaxTVs) {
+ void cancelDecision(const Expr *Decision, unsigned Since, int NumTVs,
+ int MaxTVs, unsigned NumConds) {
auto &Diag = CVM.getCodeGenModule().getDiags();
unsigned DiagID =
Diag.getCustomDiagID(DiagnosticsEngine::Warning,
"unsupported MC/DC boolean expression; "
"number of test vectors (%0) exceeds max (%1). "
"Expression will not be covered");
- Diag.Report(E->getBeginLoc(), DiagID) << NumTVs << MaxTVs;
+ Diag.Report(Decision->getBeginLoc(), DiagID) << NumTVs << MaxTVs;
// Restore MCDCBranch to Branch.
- for (auto &SR : MutableArrayRef(SourceRegions).slice(Since)) {
- assert(!SR.isMCDCDecision() && "Decision shouldn't be seen here");
- if (SR.isMCDCBranch())
- SR.resetMCDCParams();
- }
+ unsigned FoundCount = findMCDCBranchesInSourceRegion(
+ Since, [](SourceMappingRegion &SR) { SR.resetMCDCParams(); });
+ assert(FoundCount == NumConds &&
+ "Didn't find all MCDCBranches to be restored");
+ (void)FoundCount;
// Tell CodeGenPGO not to instrument.
- MCDCState.DecisionByStmt.erase(E);
+ for (auto I = MCDCState.BranchByStmt.begin(),
+ E = MCDCState.BranchByStmt.end();
+ I != E;) {
+ auto II = I++;
+ if (II->second.DecisionStmt == Decision)
+ MCDCState.BranchByStmt.erase(II);
+ }
+ MCDCState.DecisionByStmt.erase(Decision);
}
/// Check if E belongs to system headers.
@@ -2245,19 +2299,29 @@ struct CounterCoverageMappingBuilder
return;
}
- bool IsRootNode = MCDCBuilder.isIdle();
-
unsigned SourceRegionsSince = SourceRegions.size();
// Keep track of Binary Operator and assign MCDC condition IDs.
- MCDCBuilder.pushAndAssignIDs(E);
+ auto [_, RHSid] = MCDCBuilder.pushAndAssignIDs(E);
+
+ // DecisionRHS inherits CurCondIDs.
+ auto &CurCondIDs = MCDCBuilder.getCurCondIDs();
+ auto DecisionRHS = CurCondIDs;
+
+ CurCondIDs[true] = RHSid;
+ auto DecisionLHS = CurCondIDs;
extendRegion(E->getLHS());
propagateCounts(getRegion().getCounter(), E->getLHS());
handleFileExit(getEnd(E->getLHS()));
- // Track LHS True/False Decision.
- const auto DecisionLHS = MCDCBuilder.pop();
+ // Restore CurCondIDs.
+ {
+ auto &CurCondIDs =
+ MCDCBuilder.getCurCondIDs(); // Stack may be reallocated.
+ CurCondIDs[true] = DecisionRHS[true];
+ assert(CurCondIDs == DecisionRHS);
+ }
// Counter tracks the right hand side of a logical and operator.
extendRegion(E->getRHS());
@@ -2266,9 +2330,6 @@ struct CounterCoverageMappingBuilder
if (llvm::EnableSingleByteCoverage)
return;
- // Track RHS True/False Decision.
- const auto DecisionRHS = MCDCBuilder.back();
-
// Extract the Parent Region Counter.
Counter ParentCnt = getRegion().getCounter();
@@ -2285,9 +2346,8 @@ struct CounterCoverageMappingBuilder
// Create Branch Region around RHS condition.
createBranchRegion(E->getRHS(), RHSTrueCnt, RHSExitCnt, DecisionRHS);
- // Create MCDC Decision Region if at top-level (root).
- if (IsRootNode)
- createOrCancelDecision(E, SourceRegionsSince);
+ // Create MCDC Decision Region when E is at the top level.
+ createOrCancelDecision(E, SourceRegionsSince);
}
// Determine whether the right side of OR operation need to be visited.
@@ -2306,19 +2366,28 @@ struct CounterCoverageMappingBuilder
return;
}
- bool IsRootNode = MCDCBuilder.isIdle();
-
unsigned SourceRegionsSince = SourceRegions.size();
// Keep track of Binary Operator and assign MCDC condition IDs.
- MCDCBuilder.pushAndAssignIDs(E);
+ auto [_, RHSid] = MCDCBuilder.pushAndAssignIDs(E);
+
+ // Push the LHS decision IDs onto the DecisionStack.
+ auto &CurCondIDs = MCDCBuilder.getCurCondIDs();
+ auto DecisionRHS = CurCondIDs;
+ CurCondIDs[false] = RHSid;
+ auto DecisionLHS = CurCondIDs;
extendRegion(E->getLHS());
Counter OutCount = propagateCounts(getRegion().getCounter(), E->getLHS());
handleFileExit(getEnd(E->getLHS()));
// Track LHS True/False Decision.
- const auto DecisionLHS = MCDCBuilder.pop();
+ {
+ auto &CurCondIDs =
+ MCDCBuilder.getCurCondIDs(); // Stack may be reallocated.
+ CurCondIDs[false] = DecisionRHS[false];
+ assert(CurCondIDs == DecisionRHS);
+ }
// Counter tracks the right hand side of a logical or operator.
extendRegion(E->getRHS());
@@ -2327,9 +2396,6 @@ struct CounterCoverageMappingBuilder
if (llvm::EnableSingleByteCoverage)
return;
- // Track RHS True/False Decision.
- const auto DecisionRHS = MCDCBuilder.back();
-
// Extract the Parent Region Counter.
Counter ParentCnt = getRegion().getCounter();
@@ -2350,9 +2416,8 @@ struct CounterCoverageMappingBuilder
// Create Branch Region around RHS condition.
createBranchRegion(E->getRHS(), RHSExitCnt, RHSFalseCnt, DecisionRHS);
- // Create MCDC Decision Region if at top-level (root).
- if (IsRootNode)
- createOrCancelDecision(E, SourceRegionsSince);
+ // Create MCDC Decision Region when E is at the top level.
+ createOrCancelDecision(E, SourceRegionsSince);
}
void VisitLambdaExpr(const LambdaExpr *LE) {
>From 8eff226c98bdcfcd1366120699a42e0c4c73375c Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi <geek4ci...@gmail.com>
Date: Sun, 2 Feb 2025 22:00:48 +0900
Subject: [PATCH 5/9] [MC/DC] Prune MCDCLogOpStack and use
CGF.isMCDCDecisionExpr
`MCDCLogOpStack` is used only for detection of the Decision root. It
can be detected with `MCDC::State::DecisionByStmt`.
---
clang/lib/CodeGen/CGExprScalar.cpp | 40 ++++++++++-----------------
clang/lib/CodeGen/CodeGenFunction.cpp | 16 +++--------
clang/lib/CodeGen/CodeGenFunction.h | 8 ++++--
clang/lib/CodeGen/CodeGenPGO.h | 14 ++++++++++
4 files changed, 37 insertions(+), 41 deletions(-)
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp
b/clang/lib/CodeGen/CGExprScalar.cpp
index df850421c72c6c..37b32977b8bd3e 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -4990,11 +4990,9 @@ Value *ScalarExprEmitter::VisitBinLAnd(const
BinaryOperator *E) {
CGF.incrementProfileCounter(E);
// If the top of the logical operator nest, reset the MCDC temp to 0.
- if (CGF.MCDCLogOpStack.empty())
+ if (CGF.isMCDCDecisionExpr(E))
CGF.maybeResetMCDCCondBitmap(E);
- CGF.MCDCLogOpStack.push_back(E);
-
Value *RHSCond = CGF.EvaluateExprAsBool(E->getRHS());
// If we're generating for profiling or coverage, generate a branch to a
@@ -5014,9 +5012,8 @@ Value *ScalarExprEmitter::VisitBinLAnd(const
BinaryOperator *E) {
} else
CGF.markStmtMaybeUsed(E->getRHS());
- CGF.MCDCLogOpStack.pop_back();
// If the top of the logical operator nest, update the MCDC bitmap.
- if (CGF.MCDCLogOpStack.empty())
+ if (CGF.isMCDCDecisionExpr(E))
CGF.maybeUpdateMCDCTestVectorBitmap(E);
// ZExt result to int or bool.
@@ -5031,11 +5028,9 @@ Value *ScalarExprEmitter::VisitBinLAnd(const
BinaryOperator *E) {
}
// If the top of the logical operator nest, reset the MCDC temp to 0.
- if (CGF.MCDCLogOpStack.empty())
+ if (CGF.isMCDCDecisionExpr(E))
CGF.maybeResetMCDCCondBitmap(E);
- CGF.MCDCLogOpStack.push_back(E);
-
llvm::BasicBlock *ContBlock = CGF.createBasicBlock("land.end");
llvm::BasicBlock *RHSBlock = CGF.createBasicBlock("land.rhs");
@@ -5086,9 +5081,8 @@ Value *ScalarExprEmitter::VisitBinLAnd(const
BinaryOperator *E) {
// Insert an entry into the phi node for the edge with the value of RHSCond.
PN->addIncoming(RHSCond, RHSBlock);
- CGF.MCDCLogOpStack.pop_back();
// If the top of the logical operator nest, update the MCDC bitmap.
- if (CGF.MCDCLogOpStack.empty())
+ if (CGF.isMCDCDecisionExpr(E))
CGF.maybeUpdateMCDCTestVectorBitmap(E);
// Artificial location to preserve the scope information
@@ -5133,11 +5127,9 @@ Value *ScalarExprEmitter::VisitBinLOr(const
BinaryOperator *E) {
CGF.incrementProfileCounter(E);
// If the top of the logical operator nest, reset the MCDC temp to 0.
- if (CGF.MCDCLogOpStack.empty())
+ if (CGF.isMCDCDecisionExpr(E))
CGF.maybeResetMCDCCondBitmap(E);
- CGF.MCDCLogOpStack.push_back(E);
-
Value *RHSCond = CGF.EvaluateExprAsBool(E->getRHS());
// If we're generating for profiling or coverage, generate a branch to a
@@ -5157,9 +5149,8 @@ Value *ScalarExprEmitter::VisitBinLOr(const
BinaryOperator *E) {
} else
CGF.markStmtMaybeUsed(E->getRHS());
- CGF.MCDCLogOpStack.pop_back();
// If the top of the logical operator nest, update the MCDC bitmap.
- if (CGF.MCDCLogOpStack.empty())
+ if (CGF.isMCDCDecisionExpr(E))
CGF.maybeUpdateMCDCTestVectorBitmap(E);
// ZExt result to int or bool.
@@ -5174,11 +5165,9 @@ Value *ScalarExprEmitter::VisitBinLOr(const
BinaryOperator *E) {
}
// If the top of the logical operator nest, reset the MCDC temp to 0.
- if (CGF.MCDCLogOpStack.empty())
+ if (CGF.isMCDCDecisionExpr(E))
CGF.maybeResetMCDCCondBitmap(E);
- CGF.MCDCLogOpStack.push_back(E);
-
llvm::BasicBlock *ContBlock = CGF.createBasicBlock("lor.end");
llvm::BasicBlock *RHSBlock = CGF.createBasicBlock("lor.rhs");
@@ -5229,9 +5218,8 @@ Value *ScalarExprEmitter::VisitBinLOr(const
BinaryOperator *E) {
CGF.EmitBlock(ContBlock);
PN->addIncoming(RHSCond, RHSBlock);
- CGF.MCDCLogOpStack.pop_back();
// If the top of the logical operator nest, update the MCDC bitmap.
- if (CGF.MCDCLogOpStack.empty())
+ if (CGF.isMCDCDecisionExpr(E))
CGF.maybeUpdateMCDCTestVectorBitmap(E);
// ZExt result to int.
@@ -5390,8 +5378,8 @@ VisitAbstractConditionalOperator(const
AbstractConditionalOperator *E) {
}
// If the top of the logical operator nest, reset the MCDC temp to 0.
- if (CGF.MCDCLogOpStack.empty())
- CGF.maybeResetMCDCCondBitmap(condExpr);
+ if (auto E = CGF.stripCond(condExpr); CGF.isMCDCDecisionExpr(E))
+ CGF.maybeResetMCDCCondBitmap(E);
llvm::BasicBlock *LHSBlock = CGF.createBasicBlock("cond.true");
llvm::BasicBlock *RHSBlock = CGF.createBasicBlock("cond.false");
@@ -5406,8 +5394,8 @@ VisitAbstractConditionalOperator(const
AbstractConditionalOperator *E) {
// If the top of the logical operator nest, update the MCDC bitmap for the
// ConditionalOperator prior to visiting its LHS and RHS blocks, since they
// may also contain a boolean expression.
- if (CGF.MCDCLogOpStack.empty())
- CGF.maybeUpdateMCDCTestVectorBitmap(condExpr);
+ if (auto E = CGF.stripCond(condExpr); CGF.isMCDCDecisionExpr(E))
+ CGF.maybeUpdateMCDCTestVectorBitmap(E);
if (llvm::EnableSingleByteCoverage)
CGF.incrementProfileCounter(lhsExpr);
@@ -5426,8 +5414,8 @@ VisitAbstractConditionalOperator(const
AbstractConditionalOperator *E) {
// If the top of the logical operator nest, update the MCDC bitmap for the
// ConditionalOperator prior to visiting its LHS and RHS blocks, since they
// may also contain a boolean expression.
- if (CGF.MCDCLogOpStack.empty())
- CGF.maybeUpdateMCDCTestVectorBitmap(condExpr);
+ if (auto E = CGF.stripCond(condExpr); CGF.isMCDCDecisionExpr(E))
+ CGF.maybeUpdateMCDCTestVectorBitmap(E);
if (llvm::EnableSingleByteCoverage)
CGF.incrementProfileCounter(rhsExpr);
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp
b/clang/lib/CodeGen/CodeGenFunction.cpp
index bbef277a524480..3a6b97a8a226fa 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1851,8 +1851,6 @@ void CodeGenFunction::EmitBranchOnBoolExpr(
if (const BinaryOperator *CondBOp = dyn_cast<BinaryOperator>(Cond)) {
// Handle X && Y in a condition.
if (CondBOp->getOpcode() == BO_LAnd) {
- MCDCLogOpStack.push_back(CondBOp);
-
// If we have "1 && X", simplify the code. "0 && X" would have constant
// folded if the case was simple enough.
bool ConstantBool = false;
@@ -1862,7 +1860,6 @@ void CodeGenFunction::EmitBranchOnBoolExpr(
incrementProfileCounter(CondBOp);
EmitBranchToCounterBlock(CondBOp->getRHS(), BO_LAnd, TrueBlock,
FalseBlock, TrueCount, LH);
- MCDCLogOpStack.pop_back();
return;
}
@@ -1873,7 +1870,6 @@ void CodeGenFunction::EmitBranchOnBoolExpr(
// br(X && 1) -> br(X).
EmitBranchToCounterBlock(CondBOp->getLHS(), BO_LAnd, TrueBlock,
FalseBlock, TrueCount, LH, CondBOp);
- MCDCLogOpStack.pop_back();
return;
}
@@ -1903,13 +1899,10 @@ void CodeGenFunction::EmitBranchOnBoolExpr(
EmitBranchToCounterBlock(CondBOp->getRHS(), BO_LAnd, TrueBlock,
FalseBlock, TrueCount, LH);
eval.end(*this);
- MCDCLogOpStack.pop_back();
return;
}
if (CondBOp->getOpcode() == BO_LOr) {
- MCDCLogOpStack.push_back(CondBOp);
-
// If we have "0 || X", simplify the code. "1 || X" would have constant
// folded if the case was simple enough.
bool ConstantBool = false;
@@ -1919,7 +1912,6 @@ void CodeGenFunction::EmitBranchOnBoolExpr(
incrementProfileCounter(CondBOp);
EmitBranchToCounterBlock(CondBOp->getRHS(), BO_LOr, TrueBlock,
FalseBlock, TrueCount, LH);
- MCDCLogOpStack.pop_back();
return;
}
@@ -1930,7 +1922,6 @@ void CodeGenFunction::EmitBranchOnBoolExpr(
// br(X || 0) -> br(X).
EmitBranchToCounterBlock(CondBOp->getLHS(), BO_LOr, TrueBlock,
FalseBlock, TrueCount, LH, CondBOp);
- MCDCLogOpStack.pop_back();
return;
}
// Emit the LHS as a conditional. If the LHS conditional is true, we
@@ -1963,7 +1954,6 @@ void CodeGenFunction::EmitBranchOnBoolExpr(
RHSCount, LH);
eval.end(*this);
- MCDCLogOpStack.pop_back();
return;
}
}
@@ -2048,7 +2038,7 @@ void CodeGenFunction::EmitBranchOnBoolExpr(
// If not at the top of the logical operator nest, update MCDC temp with the
// boolean result of the evaluated condition.
- if (!MCDCLogOpStack.empty()) {
+ {
const Expr *MCDCBaseExpr = Cond;
// When a nested ConditionalOperator (ternary) is encountered in a boolean
// expression, MC/DC tracks the result of the ternary, and this is tied to
@@ -2058,7 +2048,9 @@ void CodeGenFunction::EmitBranchOnBoolExpr(
if (ConditionalOp)
MCDCBaseExpr = ConditionalOp;
- maybeUpdateMCDCCondBitmap(MCDCBaseExpr, CondV);
+ if (isMCDCBranchExpr(stripCond(MCDCBaseExpr)) &&
+ !isMCDCDecisionExpr(stripCond(Cond)))
+ maybeUpdateMCDCCondBitmap(MCDCBaseExpr, CondV);
}
llvm::MDNode *Weights = nullptr;
diff --git a/clang/lib/CodeGen/CodeGenFunction.h
b/clang/lib/CodeGen/CodeGenFunction.h
index e978cad4336238..cac2c658addae5 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -312,9 +312,6 @@ class CodeGenFunction : public CodeGenTypeCache {
/// nest would extend.
SmallVector<llvm::CanonicalLoopInfo *, 4> OMPLoopNestStack;
- /// Stack to track the Logical Operator recursion nest for MC/DC.
- SmallVector<const BinaryOperator *, 16> MCDCLogOpStack;
-
/// Stack to track the controlled convergence tokens.
SmallVector<llvm::ConvergenceControlInst *, 4> ConvergenceTokenStack;
@@ -1679,6 +1676,11 @@ class CodeGenFunction : public CodeGenTypeCache {
return (BOp && BOp->isLogicalOp());
}
+ bool isMCDCDecisionExpr(const Expr *E) const {
+ return PGO.isMCDCDecisionExpr(E);
+ }
+ bool isMCDCBranchExpr(const Expr *E) const { return PGO.isMCDCBranchExpr(E);
}
+
/// Zero-init the MCDC temp value.
void maybeResetMCDCCondBitmap(const Expr *E) {
if (isMCDCCoverageEnabled() && isBinaryLogicalOp(E)) {
diff --git a/clang/lib/CodeGen/CodeGenPGO.h b/clang/lib/CodeGen/CodeGenPGO.h
index 1944b640951d5c..0cbea643a65406 100644
--- a/clang/lib/CodeGen/CodeGenPGO.h
+++ b/clang/lib/CodeGen/CodeGenPGO.h
@@ -111,6 +111,20 @@ class CodeGenPGO {
public:
std::pair<bool, bool> getIsCounterPair(const Stmt *S) const;
+
+ bool isMCDCDecisionExpr(const Expr *E) const {
+ if (!RegionMCDCState)
+ return false;
+ auto I = RegionMCDCState->DecisionByStmt.find(E);
+ if (I == RegionMCDCState->DecisionByStmt.end())
+ return false;
+ return I->second.isValid();
+ }
+
+ bool isMCDCBranchExpr(const Expr *E) const {
+ return (RegionMCDCState && RegionMCDCState->BranchByStmt.contains(E));
+ }
+
void emitCounterSetOrIncrement(CGBuilderTy &Builder, const Stmt *S,
llvm::Value *StepV);
void emitMCDCTestVectorBitmapUpdate(CGBuilderTy &Builder, const Expr *S,
>From c74e5af3fb458230931d7cbba5d32e5999c38bf4 Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi <geek4ci...@gmail.com>
Date: Sun, 2 Feb 2025 22:01:40 +0900
Subject: [PATCH 6/9] [MC/DC] Create dedicated MCDCCondBitmapAddr for each
Decision
MCDCCondBitmapAddr is moved from `CodeGenFunction` into `MCDCState`
and created for each Decision.
In `maybeCreateMCDCCondBitmap`, Allocate bitmaps for all valid
Decisions and emit them order by ID, to prevent nondeterminism.
---
clang/lib/CodeGen/CodeGenFunction.h | 17 +--
clang/lib/CodeGen/CodeGenPGO.cpp | 41 +++++--
clang/lib/CodeGen/CodeGenPGO.h | 8 +-
clang/lib/CodeGen/MCDCState.h | 2 +
.../test/CoverageMapping/mcdc-single-cond.cpp | 101 ++++++++++++++++++
clang/test/Profile/c-mcdc-logicalop-ternary.c | 18 ++--
6 files changed, 160 insertions(+), 27 deletions(-)
create mode 100644 clang/test/CoverageMapping/mcdc-single-cond.cpp
diff --git a/clang/lib/CodeGen/CodeGenFunction.h
b/clang/lib/CodeGen/CodeGenFunction.h
index e978cad4336238..20d81ba3a3bde1 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -1628,9 +1628,6 @@ class CodeGenFunction : public CodeGenTypeCache {
CodeGenPGO PGO;
- /// Bitmap used by MC/DC to track condition outcomes of a boolean expression.
- Address MCDCCondBitmapAddr = Address::invalid();
-
/// Calculate branch weights appropriate for PGO data
llvm::MDNode *createProfileWeights(uint64_t TrueCount,
uint64_t FalseCount) const;
@@ -1669,8 +1666,12 @@ class CodeGenFunction : public CodeGenTypeCache {
void maybeCreateMCDCCondBitmap() {
if (isMCDCCoverageEnabled()) {
PGO.emitMCDCParameters(Builder);
- MCDCCondBitmapAddr =
- CreateIRTemp(getContext().UnsignedIntTy, "mcdc.addr");
+
+ // Set up MCDCCondBitmapAddr for each Decision.
+ // Note: This doesn't initialize Addrs in invalidated Decisions.
+ for (auto *MCDCCondBitmapAddr : PGO.getMCDCCondBitmapAddrArray(Builder))
+ *MCDCCondBitmapAddr =
+ CreateIRTemp(getContext().UnsignedIntTy, "mcdc.addr");
}
}
@@ -1682,7 +1683,7 @@ class CodeGenFunction : public CodeGenTypeCache {
/// Zero-init the MCDC temp value.
void maybeResetMCDCCondBitmap(const Expr *E) {
if (isMCDCCoverageEnabled() && isBinaryLogicalOp(E)) {
- PGO.emitMCDCCondBitmapReset(Builder, E, MCDCCondBitmapAddr);
+ PGO.emitMCDCCondBitmapReset(Builder, E);
PGO.setCurrentStmt(E);
}
}
@@ -1691,7 +1692,7 @@ class CodeGenFunction : public CodeGenTypeCache {
/// If \p StepV is null, the default increment is 1.
void maybeUpdateMCDCTestVectorBitmap(const Expr *E) {
if (isMCDCCoverageEnabled() && isBinaryLogicalOp(E)) {
- PGO.emitMCDCTestVectorBitmapUpdate(Builder, E, MCDCCondBitmapAddr,
*this);
+ PGO.emitMCDCTestVectorBitmapUpdate(Builder, E, *this);
PGO.setCurrentStmt(E);
}
}
@@ -1699,7 +1700,7 @@ class CodeGenFunction : public CodeGenTypeCache {
/// Update the MCDC temp value with the condition's evaluated result.
void maybeUpdateMCDCCondBitmap(const Expr *E, llvm::Value *Val) {
if (isMCDCCoverageEnabled()) {
- PGO.emitMCDCCondBitmapUpdate(Builder, E, MCDCCondBitmapAddr, Val, *this);
+ PGO.emitMCDCCondBitmapUpdate(Builder, E, Val, *this);
PGO.setCurrentStmt(E);
}
}
diff --git a/clang/lib/CodeGen/CodeGenPGO.cpp b/clang/lib/CodeGen/CodeGenPGO.cpp
index 0331ff83e633f7..7d16f673ada419 100644
--- a/clang/lib/CodeGen/CodeGenPGO.cpp
+++ b/clang/lib/CodeGen/CodeGenPGO.cpp
@@ -1245,9 +1245,29 @@ void CodeGenPGO::emitMCDCParameters(CGBuilderTy
&Builder) {
CGM.getIntrinsic(llvm::Intrinsic::instrprof_mcdc_parameters), Args);
}
+/// Fill mcdc.addr order by ID.
+std::vector<Address *>
+CodeGenPGO::getMCDCCondBitmapAddrArray(CGBuilderTy &Builder) {
+ std::vector<Address *> Result;
+
+ if (!canEmitMCDCCoverage(Builder) || !RegionMCDCState)
+ return Result;
+
+ SmallVector<std::pair<unsigned, Address *>> SortedPair;
+ for (auto &[_, V] : RegionMCDCState->DecisionByStmt)
+ if (V.isValid())
+ SortedPair.emplace_back(V.ID, &V.MCDCCondBitmapAddr);
+
+ llvm::sort(SortedPair);
+
+ for (auto &[_, MCDCCondBitmapAddr] : SortedPair)
+ Result.push_back(MCDCCondBitmapAddr);
+
+ return Result;
+}
+
void CodeGenPGO::emitMCDCTestVectorBitmapUpdate(CGBuilderTy &Builder,
const Expr *S,
- Address MCDCCondBitmapAddr,
CodeGenFunction &CGF) {
if (!canEmitMCDCCoverage(Builder) || !RegionMCDCState)
return;
@@ -1258,6 +1278,10 @@ void
CodeGenPGO::emitMCDCTestVectorBitmapUpdate(CGBuilderTy &Builder,
if (DecisionStateIter == RegionMCDCState->DecisionByStmt.end())
return;
+ auto &MCDCCondBitmapAddr = DecisionStateIter->second.MCDCCondBitmapAddr;
+ if (!MCDCCondBitmapAddr.isValid())
+ return;
+
// Don't create tvbitmap_update if the record is allocated but excluded.
// Or `bitmap |= (1 << 0)` would be wrongly executed to the next bitmap.
if (DecisionStateIter->second.Indices.size() == 0)
@@ -1280,14 +1304,16 @@ void
CodeGenPGO::emitMCDCTestVectorBitmapUpdate(CGBuilderTy &Builder,
CGM.getIntrinsic(llvm::Intrinsic::instrprof_mcdc_tvbitmap_update), Args);
}
-void CodeGenPGO::emitMCDCCondBitmapReset(CGBuilderTy &Builder, const Expr *S,
- Address MCDCCondBitmapAddr) {
+void CodeGenPGO::emitMCDCCondBitmapReset(CGBuilderTy &Builder, const Expr *S) {
if (!canEmitMCDCCoverage(Builder) || !RegionMCDCState)
return;
- S = S->IgnoreParens();
+ auto I = RegionMCDCState->DecisionByStmt.find(S->IgnoreParens());
+ if (I == RegionMCDCState->DecisionByStmt.end())
+ return;
- if (!RegionMCDCState->DecisionByStmt.contains(S))
+ auto &MCDCCondBitmapAddr = I->second.MCDCCondBitmapAddr;
+ if (!MCDCCondBitmapAddr.isValid())
return;
// Emit intrinsic that resets a dedicated temporary value on the stack to 0.
@@ -1295,7 +1321,6 @@ void CodeGenPGO::emitMCDCCondBitmapReset(CGBuilderTy
&Builder, const Expr *S,
}
void CodeGenPGO::emitMCDCCondBitmapUpdate(CGBuilderTy &Builder, const Expr *S,
- Address MCDCCondBitmapAddr,
llvm::Value *Val,
CodeGenFunction &CGF) {
if (!canEmitMCDCCoverage(Builder) || !RegionMCDCState)
@@ -1325,6 +1350,10 @@ void CodeGenPGO::emitMCDCCondBitmapUpdate(CGBuilderTy
&Builder, const Expr *S,
if (DecisionIter == RegionMCDCState->DecisionByStmt.end())
return;
+ auto &MCDCCondBitmapAddr = DecisionIter->second.MCDCCondBitmapAddr;
+ if (!MCDCCondBitmapAddr.isValid())
+ return;
+
const auto &TVIdxs = DecisionIter->second.Indices[Branch.ID];
auto *CurTV = Builder.CreateLoad(MCDCCondBitmapAddr,
diff --git a/clang/lib/CodeGen/CodeGenPGO.h b/clang/lib/CodeGen/CodeGenPGO.h
index 1944b640951d5c..0d7dd13f611a93 100644
--- a/clang/lib/CodeGen/CodeGenPGO.h
+++ b/clang/lib/CodeGen/CodeGenPGO.h
@@ -114,14 +114,12 @@ class CodeGenPGO {
void emitCounterSetOrIncrement(CGBuilderTy &Builder, const Stmt *S,
llvm::Value *StepV);
void emitMCDCTestVectorBitmapUpdate(CGBuilderTy &Builder, const Expr *S,
- Address MCDCCondBitmapAddr,
CodeGenFunction &CGF);
void emitMCDCParameters(CGBuilderTy &Builder);
- void emitMCDCCondBitmapReset(CGBuilderTy &Builder, const Expr *S,
- Address MCDCCondBitmapAddr);
+ std::vector<Address *> getMCDCCondBitmapAddrArray(CGBuilderTy &Builder);
+ void emitMCDCCondBitmapReset(CGBuilderTy &Builder, const Expr *S);
void emitMCDCCondBitmapUpdate(CGBuilderTy &Builder, const Expr *S,
- Address MCDCCondBitmapAddr, llvm::Value *Val,
- CodeGenFunction &CGF);
+ llvm::Value *Val, CodeGenFunction &CGF);
void markStmtAsUsed(bool Skipped, const Stmt *S) {
// Do nothing.
diff --git a/clang/lib/CodeGen/MCDCState.h b/clang/lib/CodeGen/MCDCState.h
index 0b6f5f28235f43..2bb4eaf9fdfc83 100644
--- a/clang/lib/CodeGen/MCDCState.h
+++ b/clang/lib/CodeGen/MCDCState.h
@@ -13,6 +13,7 @@
#ifndef LLVM_CLANG_LIB_CODEGEN_MCDCSTATE_H
#define LLVM_CLANG_LIB_CODEGEN_MCDCSTATE_H
+#include "Address.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ProfileData/Coverage/MCDCTypes.h"
@@ -38,6 +39,7 @@ struct State {
unsigned BitmapIdx;
IndicesTy Indices;
unsigned ID = InvalidID;
+ Address MCDCCondBitmapAddr = Address::invalid();
bool isValid() const { return ID != InvalidID; }
diff --git a/clang/test/CoverageMapping/mcdc-single-cond.cpp
b/clang/test/CoverageMapping/mcdc-single-cond.cpp
new file mode 100644
index 00000000000000..d6c694a63dd285
--- /dev/null
+++ b/clang/test/CoverageMapping/mcdc-single-cond.cpp
@@ -0,0 +1,101 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++11 -fcoverage-mcdc
-fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping
-disable-llvm-passes -emit-llvm -o %t2.ll %s | FileCheck %s
--check-prefixes=MM,MM2
+// RUN: FileCheck %s --check-prefixes=LL,LL2 < %t2.ll
+
+// LL: define{{.+}}func_cond{{.+}}(
+// MM: func_cond{{.*}}:
+int func_cond(bool a, bool b) {
+ // %mcdc.addr* are emitted by static order.
+ // LL: %[[MA4:mcdc.addr.*]] = alloca i32, align 4
+ // LL: %[[MA6:mcdc.addr.*]] = alloca i32, align 4
+ // LL: %[[MA7:mcdc.addr.*]] = alloca i32, align 4
+ // LL: %[[MA9:mcdc.addr.*]] = alloca i32, align 4
+ // LL: %[[MA10:mcdc.addr.*]] = alloca i32, align 4
+ // LL: call void @llvm.instrprof.mcdc.parameters(ptr @[[PROFN:.+]], i64
[[#H:]], i32 [[#BS:]])
+ int count = 0;
+ if (a)
+ // NB=2 Single cond
+ // MM2-NOT: Decision
+ ++count;
+ if (a ? true : false)
+ // NB=2,2 Wider decision comes first.
+ // MA2 has C:2
+ // MA3 has C:1
+ ++count;
+ if (a && b ? true : false)
+ // NB=2,3 Wider decision comes first.
+ // MM2: Decision,File 0, [[@LINE-2]]:7 -> [[#L:@LINE-2]]:13 = M:[[#I:3]],
C:2
+ // MM: Branch,File 0, [[#L]]:7 -> [[#L]]:8 = #6, (#0 - #6) [1,2,0]
+ // MM: Branch,File 0, [[#L]]:12 -> [[#L]]:13 = #7, (#6 - #7) [2,0,0]
+ // LL: store i32 0, ptr %[[MA4]], align 4
+ // LL: = load i32, ptr %[[MA4]], align 4
+ // LL: store i32 %{{.+}}, ptr %[[MA4]], align 4
+ // LL: = load i32, ptr %[[MA4]], align 4
+ // LL: store i32 %{{.+}}, ptr %[[MA4]], align 4
+ // LL2: call void @llvm.instrprof.mcdc.tvbitmap.update(ptr @[[PROFN]],
i64 [[#H]], i32 [[#B:0]], ptr %[[MA4]])
+ ++count;
+ while (a || true) {
+ // NB=3 BinOp only
+ // MM: Decision,File 0, [[@LINE-2]]:10 -> [[#L:@LINE-2]]:19 =
M:[[#I:I+3]], C:2
+ // MM: Branch,File 0, [[#L]]:10 -> [[#L]]:11 = (#0 - #9), #9 [1,0,2]
+ // MM: Branch,File 0, [[#L]]:15 -> [[#L]]:19 = (#9 - #10), 0 [2,0,0]
+ // LL: store i32 0, ptr %[[MA6]], align 4
+ // LL: = load i32, ptr %[[MA6]], align 4
+ // LL: store i32 %{{.+}}, ptr %[[MA6]], align 4
+ // LL: = load i32, ptr %[[MA6]], align 4
+ // LL: store i32 %{{.+}}, ptr %[[MA6]], align 4
+ // LL2: call void @llvm.instrprof.mcdc.tvbitmap.update(ptr @[[PROFN]],
i64 [[#H]], i32 [[#B:B+3]], ptr %[[MA6]])
+ ++count;
+ break;
+ }
+ while (a || true ? false : true) {
+ // Wider decision comes first.
+ // MM2: Decision,File 0, [[@LINE-2]]:10 -> [[#L:@LINE-2]]:19 =
M:[[#I:I+3]], C:2
+ // MM: Branch,File 0, [[#L]]:10 -> [[#L]]:11 = ((#0 + #11) - #13), #13
[1,0,2]
+ // MM: Branch,File 0, [[#L]]:15 -> [[#L]]:19 = (#13 - #14), 0 [2,0,0]
+ // LL: store i32 0, ptr %[[MA7]], align 4
+ // LL: = load i32, ptr %[[MA7]], align 4
+ // LL: store i32 %{{.+}}, ptr %[[MA7]], align 4
+ // LL: = load i32, ptr %[[MA7]], align 4
+ // LL: store i32 %{{.+}}, ptr %[[MA7]], align 4
+ // LL: call void @llvm.instrprof.mcdc.tvbitmap.update(ptr @[[PROFN]],
i64 [[#H]], i32 [[#B:B+3]], ptr %[[MA7]])
+ ++count;
+ }
+ do {
+ ++count;
+ } while (a && false);
+ // BinOp only
+ // MM: Decision,File 0, [[@LINE-2]]:12 -> [[#L:@LINE-2]]:22 =
M:[[#I:I+3]], C:2
+ // MM: Branch,File 0, [[#L]]:12 -> [[#L]]:13 = #16, ((#0 + #15) - #16)
[1,2,0]
+ // MM: Branch,File 0, [[#L]]:17 -> [[#L]]:22 = 0, (#16 - #17) [2,0,0]
+ // LL: store i32 0, ptr %[[MA9]], align 4
+ // LL: = load i32, ptr %[[MA9]], align 4
+ // LL: store i32 %{{.+}}, ptr %[[MA9]], align 4
+ // LL: = load i32, ptr %[[MA9]], align 4
+ // LL: store i32 %{{.+}}, ptr %[[MA9]], align 4
+ // LL2: call void @llvm.instrprof.mcdc.tvbitmap.update(ptr @[[PROFN]], i64
[[#H]], i32 [[#B:B+3]], ptr %[[MA9]])
+ do {
+ ++count;
+ } while (a && false ? true : false);
+ // Wider decision comes first.
+ // MM2: Decision,File 0, [[@LINE-2]]:12 -> [[#L:@LINE-2]]:22 = M:15, C:2
+ // MM: Branch,File 0, [[#L]]:12 -> [[#L]]:13 = #20, ((#0 + #18) - #20)
[1,2,0]
+ // MM: Branch,File 0, [[#L]]:17 -> [[#L]]:22 = 0, (#20 - #21) [2,0,0]
+ // LL: store i32 0, ptr %[[MA10]], align 4
+ // LL: = load i32, ptr %[[MA10]], align 4
+ // LL: store i32 %{{.+}}, ptr %[[MA10]], align 4
+ // LL: = load i32, ptr %[[MA10]], align 4
+ // LL: store i32 %{{.+}}, ptr %[[MA10]], align 4
+ // LL: call void @llvm.instrprof.mcdc.tvbitmap.update(ptr @[[PROFN]], i64
[[#H]], i32 [[#B:B+3]], ptr %[[MA10]])
+ // FIXME: Confirm (B+3==BS)
+ for (int i = 0; i < (a ? 2 : 1); ++i) {
+ // Simple nested decision (different column)
+ // MM2-NOT: Decision
+ // LL2-NOT: call void @llvm.instrprof.mcdc.tvbitmap.update
+ ++count;
+ }
+ for (int i = 0; i >= 4 ? false : true; ++i) {
+ // Wider decision comes first.
+ ++count;
+ }
+ return count;
+}
diff --git a/clang/test/Profile/c-mcdc-logicalop-ternary.c
b/clang/test/Profile/c-mcdc-logicalop-ternary.c
index 18ce0b4e5dc149..18682ffdbfec38 100644
--- a/clang/test/Profile/c-mcdc-logicalop-ternary.c
+++ b/clang/test/Profile/c-mcdc-logicalop-ternary.c
@@ -13,12 +13,14 @@ int test(int a, int b, int c, int d, int e, int f) {
// ALLOCATE MCDC TEMP AND ZERO IT.
// MCDC-LABEL: @test(
-// MCDC: %mcdc.addr = alloca i32, align 4
-// MCDC: store i32 0, ptr %mcdc.addr, align 4
+// MCDC: %[[ADDR0:mcdc.+]] = alloca i32, align 4
+// MCDC: %[[ADDR1:mcdc.+]] = alloca i32, align 4
+// MCDC: %[[ADDR2:mcdc.+]] = alloca i32, align 4
+// MCDC: store i32 0, ptr %[[ADDR0]], align 4
// TERNARY TRUE SHOULD UPDATE THE BITMAP WITH RESULT AT ELEMENT 0.
// MCDC-LABEL: cond.true:
-// MCDC-DAG: %[[TEMP0:mcdc.temp[0-9]*]] = load i32, ptr %mcdc.addr, align 4
+// MCDC-DAG: %[[TEMP0:mcdc.temp[0-9]*]] = load i32, ptr %[[ADDR0]], align 4
// MCDC: %[[TEMP:[0-9]+]] = add i32 %[[TEMP0]], 0
// MCDC: %[[LAB1:[0-9]+]] = lshr i32 %[[TEMP]], 3
// MCDC: %[[LAB4:[0-9]+]] = getelementptr inbounds i8, ptr @__profbm_test,
i32 %[[LAB1]]
@@ -30,12 +32,12 @@ int test(int a, int b, int c, int d, int e, int f) {
// MCDC: store i8 %[[LAB9]], ptr %[[LAB4]], align 1
// CHECK FOR ZERO OF MCDC TEMP
-// MCDC: store i32 0, ptr %mcdc.addr, align 4
+// MCDC: store i32 0, ptr %[[ADDR1]], align 4
// TERNARY TRUE YIELDS TERNARY LHS LOGICAL-AND.
// TERNARY LHS LOGICAL-AND SHOULD UPDATE THE BITMAP WITH RESULT AT ELEMENT 1.
// MCDC-LABEL: land.end:
-// MCDC-DAG: %[[TEMP0:mcdc.temp[0-9]*]] = load i32, ptr %mcdc.addr, align 4
+// MCDC-DAG: %[[TEMP0:mcdc.temp[0-9]*]] = load i32, ptr %[[ADDR1]], align 4
// MCDC: %[[TEMP:[0-9]+]] = add i32 %[[TEMP0]], 3
// MCDC: %[[LAB1:[0-9]+]] = lshr i32 %[[TEMP]], 3
// MCDC: %[[LAB4:[0-9]+]] = getelementptr inbounds i8, ptr @__profbm_test,
i32 %[[LAB1]]
@@ -48,7 +50,7 @@ int test(int a, int b, int c, int d, int e, int f) {
// TERNARY FALSE SHOULD UPDATE THE BITMAP WITH RESULT AT ELEMENT 0.
// MCDC-LABEL: cond.false:
-// MCDC-DAG: %[[TEMP0:mcdc.temp[0-9]*]] = load i32, ptr %mcdc.addr, align 4
+// MCDC-DAG: %[[TEMP0:mcdc.+]] = load i32, ptr %[[ADDR0]], align 4
// MCDC: %[[TEMP:[0-9]+]] = add i32 %[[TEMP0]], 0
// MCDC: %[[LAB1:[0-9]+]] = lshr i32 %[[TEMP]], 3
// MCDC: %[[LAB4:[0-9]+]] = getelementptr inbounds i8, ptr @__profbm_test,
i32 %[[LAB1]]
@@ -60,12 +62,12 @@ int test(int a, int b, int c, int d, int e, int f) {
// MCDC: store i8 %[[LAB9]], ptr %[[LAB4]], align 1
// CHECK FOR ZERO OF MCDC TEMP
-// MCDC: store i32 0, ptr %mcdc.addr, align 4
+// MCDC: store i32 0, ptr %[[ADDR2]], align 4
// TERNARY FALSE YIELDS TERNARY RHS LOGICAL-OR.
// TERNARY RHS LOGICAL-OR SHOULD UPDATE THE BITMAP WITH RESULT AT ELEMENT 2.
// MCDC-LABEL: lor.end:
-// MCDC-DAG: %[[TEMP0:mcdc.temp[0-9]*]] = load i32, ptr %mcdc.addr, align 4
+// MCDC-DAG: %[[TEMP0:mcdc.temp[0-9]*]] = load i32, ptr %[[ADDR2]], align 4
// MCDC: %[[TEMP:[0-9]+]] = add i32 %[[TEMP0]], 6
// MCDC: %[[LAB1:[0-9]+]] = lshr i32 %[[TEMP]], 3
// MCDC: %[[LAB4:[0-9]+]] = getelementptr inbounds i8, ptr @__profbm_test,
i32 %[[LAB1]]
>From c56ecc30e9fd1a674073e362fbfcc6b43f2f52e2 Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi <geek4ci...@gmail.com>
Date: Sun, 2 Feb 2025 22:06:32 +0900
Subject: [PATCH 7/9] [MC/DC] Enable nested expressions
A warning "contains an operation with a nested boolean expression." is
no longer emitter. At the moment, split expressions are treated as
individual Decisions.
---
clang/lib/CodeGen/CodeGenPGO.cpp | 150 ++++++++++--------
.../test/CoverageMapping/mcdc-nested-expr.cpp | 30 +++-
.../Frontend/custom-diag-werror-interaction.c | 4 +-
3 files changed, 109 insertions(+), 75 deletions(-)
diff --git a/clang/lib/CodeGen/CodeGenPGO.cpp b/clang/lib/CodeGen/CodeGenPGO.cpp
index 7d16f673ada419..0c3973aa4dccfd 100644
--- a/clang/lib/CodeGen/CodeGenPGO.cpp
+++ b/clang/lib/CodeGen/CodeGenPGO.cpp
@@ -228,10 +228,17 @@ struct MapRegionCounters : public
RecursiveASTVisitor<MapRegionCounters> {
/// The stacks are also used to find error cases and notify the user. A
/// standard logical operator nest for a boolean expression could be in a
form
/// similar to this: "x = a && b && c && (d || f)"
- unsigned NumCond = 0;
- bool SplitNestedLogicalOp = false;
- SmallVector<const Stmt *, 16> NonLogOpStack;
- SmallVector<const BinaryOperator *, 16> LogOpStack;
+ struct DecisionState {
+ llvm::DenseSet<const Stmt *> Leaves; // Not BinOp
+ const Expr *DecisionExpr; // Root
+ bool Split;
+
+ DecisionState() = delete;
+ DecisionState(const Expr *E, bool Split = false)
+ : DecisionExpr(E), Split(Split) {}
+ };
+
+ SmallVector<DecisionState, 1> DecisionStack;
// Hook: dataTraverseStmtPre() is invoked prior to visiting an AST Stmt node.
bool dataTraverseStmtPre(Stmt *S) {
@@ -239,34 +246,28 @@ struct MapRegionCounters : public
RecursiveASTVisitor<MapRegionCounters> {
if (MCDCMaxCond == 0)
return true;
- /// At the top of the logical operator nest, reset the number of
conditions,
- /// also forget previously seen split nesting cases.
- if (LogOpStack.empty()) {
- NumCond = 0;
- SplitNestedLogicalOp = false;
- }
-
- if (const Expr *E = dyn_cast<Expr>(S)) {
- const BinaryOperator *BinOp =
dyn_cast<BinaryOperator>(E->IgnoreParens());
- if (BinOp && BinOp->isLogicalOp()) {
- /// Check for "split-nested" logical operators. This happens when a new
- /// boolean expression logical-op nest is encountered within an
existing
- /// boolean expression, separated by a non-logical operator. For
- /// example, in "x = (a && b && c && foo(d && f))", the "d && f" case
- /// starts a new boolean expression that is separated from the other
- /// conditions by the operator foo(). Split-nested cases are not
- /// supported by MC/DC.
- SplitNestedLogicalOp = SplitNestedLogicalOp || !NonLogOpStack.empty();
-
- LogOpStack.push_back(BinOp);
+ /// Mark "in splitting" when a leaf is met.
+ if (!DecisionStack.empty()) {
+ auto &StackTop = DecisionStack.back();
+ if (!StackTop.Split) {
+ if (StackTop.Leaves.contains(S)) {
+ assert(!StackTop.Split);
+ StackTop.Split = true;
+ }
return true;
}
+
+ // Split
+ assert(StackTop.Split);
+ assert(!StackTop.Leaves.contains(S));
}
- /// Keep track of non-logical operators. These are OK as long as we don't
- /// encounter a new logical operator after seeing one.
- if (!LogOpStack.empty())
- NonLogOpStack.push_back(S);
+ if (const auto *E = dyn_cast<Expr>(S)) {
+ if (const auto *BinOp =
+ dyn_cast<BinaryOperator>(CodeGenFunction::stripCond(E));
+ BinOp && BinOp->isLogicalOp())
+ DecisionStack.emplace_back(E);
+ }
return true;
}
@@ -275,49 +276,57 @@ struct MapRegionCounters : public
RecursiveASTVisitor<MapRegionCounters> {
// an AST Stmt node. MC/DC will use it to to signal when the top of a
// logical operation (boolean expression) nest is encountered.
bool dataTraverseStmtPost(Stmt *S) {
- /// If MC/DC is not enabled, MCDCMaxCond will be set to 0. Do nothing.
- if (MCDCMaxCond == 0)
+ if (DecisionStack.empty())
return true;
- if (const Expr *E = dyn_cast<Expr>(S)) {
- const BinaryOperator *BinOp =
dyn_cast<BinaryOperator>(E->IgnoreParens());
- if (BinOp && BinOp->isLogicalOp()) {
- assert(LogOpStack.back() == BinOp);
- LogOpStack.pop_back();
-
- /// At the top of logical operator nest:
- if (LogOpStack.empty()) {
- /// Was the "split-nested" logical operator case encountered?
- if (SplitNestedLogicalOp) {
- unsigned DiagID = Diag.getCustomDiagID(
- DiagnosticsEngine::Warning,
- "unsupported MC/DC boolean expression; "
- "contains an operation with a nested boolean expression. "
- "Expression will not be covered");
- Diag.Report(S->getBeginLoc(), DiagID);
- return true;
- }
-
- /// Was the maximum number of conditions encountered?
- if (NumCond > MCDCMaxCond) {
- unsigned DiagID = Diag.getCustomDiagID(
- DiagnosticsEngine::Warning,
- "unsupported MC/DC boolean expression; "
- "number of conditions (%0) exceeds max (%1). "
- "Expression will not be covered");
- Diag.Report(S->getBeginLoc(), DiagID) << NumCond << MCDCMaxCond;
- return true;
- }
-
- // Otherwise, allocate the Decision.
- MCDCState.DecisionByStmt[BinOp].ID = MCDCState.DecisionByStmt.size();
- }
- return true;
+ /// If MC/DC is not enabled, MCDCMaxCond will be set to 0. Do nothing.
+ assert(MCDCMaxCond > 0);
+
+ auto &StackTop = DecisionStack.back();
+
+ if (StackTop.DecisionExpr != S) {
+ if (StackTop.Leaves.contains(S)) {
+ assert(StackTop.Split);
+ StackTop.Split = false;
}
+
+ return true;
+ }
+
+ /// Allocate the entry (with Valid=false)
+ auto &DecisionEntry =
+ MCDCState
+ .DecisionByStmt[CodeGenFunction::stripCond(StackTop.DecisionExpr)];
+
+ /// Was the "split-nested" logical operator case encountered?
+ if (false && DecisionStack.size() > 1) {
+ unsigned DiagID = Diag.getCustomDiagID(
+ DiagnosticsEngine::Warning,
+ "unsupported MC/DC boolean expression; "
+ "contains an operation with a nested boolean expression. "
+ "Expression will not be covered");
+ Diag.Report(S->getBeginLoc(), DiagID);
+ DecisionStack.pop_back();
+ return true;
+ }
+
+ /// Was the maximum number of conditions encountered?
+ auto NumCond = StackTop.Leaves.size();
+ if (NumCond > MCDCMaxCond) {
+ unsigned DiagID =
+ Diag.getCustomDiagID(DiagnosticsEngine::Warning,
+ "unsupported MC/DC boolean expression; "
+ "number of conditions (%0) exceeds max (%1). "
+ "Expression will not be covered");
+ Diag.Report(S->getBeginLoc(), DiagID) << NumCond << MCDCMaxCond;
+ DecisionStack.pop_back();
+ return true;
}
- if (!LogOpStack.empty())
- NonLogOpStack.pop_back();
+ // The Decision is validated.
+ DecisionEntry.ID = MCDCState.DecisionByStmt.size() - 1;
+
+ DecisionStack.pop_back();
return true;
}
@@ -329,14 +338,17 @@ struct MapRegionCounters : public
RecursiveASTVisitor<MapRegionCounters> {
/// order to use MC/DC, count the number of total LHS and RHS conditions.
bool VisitBinaryOperator(BinaryOperator *S) {
if (S->isLogicalOp()) {
- if (CodeGenFunction::isInstrumentedCondition(S->getLHS()))
- NumCond++;
+ if (CodeGenFunction::isInstrumentedCondition(S->getLHS())) {
+ if (!DecisionStack.empty())
+ DecisionStack.back().Leaves.insert(S->getLHS());
+ }
if (CodeGenFunction::isInstrumentedCondition(S->getRHS())) {
if (ProfileVersion >= llvm::IndexedInstrProf::Version7)
CounterMap[S->getRHS()] = NextCounter++;
- NumCond++;
+ if (!DecisionStack.empty())
+ DecisionStack.back().Leaves.insert(S->getRHS());
}
}
return Base::VisitBinaryOperator(S);
diff --git a/clang/test/CoverageMapping/mcdc-nested-expr.cpp
b/clang/test/CoverageMapping/mcdc-nested-expr.cpp
index bb82873e3b600d..8aa31fbdd2ab7b 100644
--- a/clang/test/CoverageMapping/mcdc-nested-expr.cpp
+++ b/clang/test/CoverageMapping/mcdc-nested-expr.cpp
@@ -1,20 +1,42 @@
-// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++11 -fcoverage-mcdc
-fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping
-emit-llvm-only %s 2> %t.stderr.txt | FileCheck %s
-// RUN: FileCheck %s --check-prefix=WARN < %t.stderr.txt
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++11 -fcoverage-mcdc
-fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping
-emit-llvm-only %s 2>&1 | FileCheck %s
+
+// CHECK-NOT: warning: unsupported MC/DC boolean expression; contains an
operation with a nested boolean expression.
// "Split-nest" -- boolean expressions within boolean expressions.
extern bool bar(bool);
// CHECK: func_split_nest{{.*}}:
bool func_split_nest(bool a, bool b, bool c, bool d, bool e, bool f, bool g) {
- // WARN: :[[@LINE+1]]:14: warning: unsupported MC/DC boolean expression;
contains an operation with a nested boolean expression.
bool res = a && b && c && bar(d && e) && f && g;
+ // CHECK: Decision,File 0, [[@LINE-1]]:14 -> [[#L:@LINE-1]]:50 = M:10, C:6
+ // CHECK: Branch,File 0, [[#L]]:14 -> [[#L]]:15 = #9, (#0 - #9) [1,6,0]
+ // CHECK: Branch,File 0, [[#L]]:19 -> [[#L]]:20 = #10, (#9 - #10) [6,5,0]
+ // CHECK: Branch,File 0, [[#L]]:24 -> [[#L]]:25 = #8, (#7 - #8) [5,4,0]
+ // CHECK: Branch,File 0, [[#L]]:29 -> [[#L]]:40 = #6, (#5 - #6) [4,3,0]
+
+ // The inner expr -- "d && e" (w/o parentheses)
+ // CHECK: Decision,File 0, [[#L]]:33 -> [[#L]]:39 = M:3, C:2
+ // CHECK: Branch,File 0, [[#L]]:33 -> [[#L]]:34 = #11, (#5 - #11) [1,2,0]
+ // CHECK: Branch,File 0, [[#L]]:38 -> [[#L]]:39 = #12, (#11 - #12) [2,0,0]
+
+ // CHECK: Branch,File 0, [[#L]]:44 -> [[#L]]:45 = #4, (#3 - #4) [3,2,0]
+ // CHECK: Branch,File 0, [[#L]]:49 -> [[#L]]:50 = #2, (#1 - #2) [2,0,0]
return bar(res);
}
// The inner expr begins with the same Loc as the outer expr
// CHECK: func_condop{{.*}}:
bool func_condop(bool a, bool b, bool c) {
- // WARN: :[[@LINE+1]]:10: warning: unsupported MC/DC boolean expression;
contains an operation with a nested boolean expression.
return (a && b ? true : false) && c;
+ // CHECK: Decision,File 0, [[@LINE-1]]:10 -> [[#L:@LINE-1]]:38 = M:6, C:2
+ // This covers outer parenthses.
+ // CHECK: Branch,File 0, [[#L]]:10 -> [[#L]]:33 = #1, (#0 - #1) [1,2,0]
+
+ // The inner expr "a && b" (w/o parenthses)
+ // CHECK: Decision,File 0, [[#L]]:11 -> [[#L]]:17 = M:3, C:2
+ // CHECK: Branch,File 0, [[#L]]:11 -> [[#L]]:12 = #4, (#0 - #4) [1,2,0]
+ // CHECK: Branch,File 0, [[#L]]:16 -> [[#L]]:17 = #5, (#4 - #5) [2,0,0]
+
+ // CHECK: Branch,File 0, [[#L]]:37 -> [[#L]]:38 = #2, (#1 - #2) [2,0,0]
}
// __builtin_expect
diff --git a/clang/test/Frontend/custom-diag-werror-interaction.c
b/clang/test/Frontend/custom-diag-werror-interaction.c
index 997c8c11ff0e0d..4b02e8fbf328a7 100644
--- a/clang/test/Frontend/custom-diag-werror-interaction.c
+++ b/clang/test/Frontend/custom-diag-werror-interaction.c
@@ -1,9 +1,9 @@
-// RUN: %clang_cc1 -emit-llvm-only -fprofile-instrument=clang -fcoverage-mcdc
-Werror -Wno-unused-value %s -verify
+// RUN: %clang_cc1 -emit-llvm-only -fprofile-instrument=clang -fcoverage-mcdc
-Werror -Wno-unused-value %s -verify -fmcdc-max-conditions=2
int foo(int x);
int main(void) {
int a, b, c;
- a && foo( b && c ); // expected-warning{{unsupported MC/DC boolean
expression; contains an operation with a nested boolean expression. Expression
will not be covered}}
+ a && foo( a && b && c ); // expected-warning{{unsupported MC/DC boolean
expression; number of conditions (3) exceeds max (2). Expression will not be
covered}}
return 0;
}
>From f70a6c8686617963c55854c2d8c6fa8607ca0806 Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi <geek4ci...@gmail.com>
Date: Sun, 2 Feb 2025 22:10:25 +0900
Subject: [PATCH 8/9] [MC/DC] Handle __builtin_expect as if parenthses
Fixes #124565
---
clang/include/clang/AST/IgnoreExpr.h | 9 +++++++++
clang/lib/CodeGen/CodeGenFunction.cpp | 13 ++++++++-----
clang/lib/CodeGen/CodeGenPGO.cpp | 8 +++++---
clang/test/CoverageMapping/mcdc-nested-expr.cpp | 5 ++++-
4 files changed, 26 insertions(+), 9 deletions(-)
diff --git a/clang/include/clang/AST/IgnoreExpr.h
b/clang/include/clang/AST/IgnoreExpr.h
index 917bada61fa6fd..c366aa17667481 100644
--- a/clang/include/clang/AST/IgnoreExpr.h
+++ b/clang/include/clang/AST/IgnoreExpr.h
@@ -134,6 +134,15 @@ inline Expr
*IgnoreElidableImplicitConstructorSingleStep(Expr *E) {
return E;
}
+inline Expr *IgnoreBuiltinExpectSingleStep(Expr *E) {
+ if (auto *CE = dyn_cast<CallExpr>(E)) {
+ if (const FunctionDecl *FD = CE->getDirectCallee())
+ if (FD->getBuiltinID() == Builtin::BI__builtin_expect)
+ return CE->getArg(0);
+ }
+ return E;
+}
+
inline Expr *IgnoreImplicitAsWrittenSingleStep(Expr *E) {
if (auto *ICE = dyn_cast<ImplicitCastExpr>(E))
return ICE->getSubExprAsWritten();
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp
b/clang/lib/CodeGen/CodeGenFunction.cpp
index bbef277a524480..05766aa7693d2e 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -27,6 +27,7 @@
#include "clang/AST/Decl.h"
#include "clang/AST/DeclCXX.h"
#include "clang/AST/Expr.h"
+#include "clang/AST/IgnoreExpr.h"
#include "clang/AST/StmtCXX.h"
#include "clang/AST/StmtObjC.h"
#include "clang/Basic/Builtins.h"
@@ -1748,12 +1749,14 @@ bool
CodeGenFunction::ConstantFoldsToSimpleInteger(const Expr *Cond,
/// Strip parentheses and simplistic logical-NOT operators.
const Expr *CodeGenFunction::stripCond(const Expr *C) {
- while (const UnaryOperator *Op = dyn_cast<UnaryOperator>(C->IgnoreParens()))
{
- if (Op->getOpcode() != UO_LNot)
- break;
- C = Op->getSubExpr();
+ while (true) {
+ const Expr *SC = IgnoreExprNodes(C, IgnoreParensSingleStep,
+ IgnoreBuiltinExpectSingleStep,
+ IgnoreImplicitCastsSingleStep);
+ if (C == SC)
+ return SC;
+ C = SC;
}
- return C->IgnoreParens();
}
/// Determine whether the given condition is an instrumentable condition
diff --git a/clang/lib/CodeGen/CodeGenPGO.cpp b/clang/lib/CodeGen/CodeGenPGO.cpp
index 792373839107f0..0fd49b880bba30 100644
--- a/clang/lib/CodeGen/CodeGenPGO.cpp
+++ b/clang/lib/CodeGen/CodeGenPGO.cpp
@@ -247,8 +247,9 @@ struct MapRegionCounters : public
RecursiveASTVisitor<MapRegionCounters> {
}
if (const Expr *E = dyn_cast<Expr>(S)) {
- const BinaryOperator *BinOp =
dyn_cast<BinaryOperator>(E->IgnoreParens());
- if (BinOp && BinOp->isLogicalOp()) {
+ if (const auto *BinOp =
+ dyn_cast<BinaryOperator>(CodeGenFunction::stripCond(E));
+ BinOp && BinOp->isLogicalOp()) {
/// Check for "split-nested" logical operators. This happens when a new
/// boolean expression logical-op nest is encountered within an
existing
/// boolean expression, separated by a non-logical operator. For
@@ -280,7 +281,8 @@ struct MapRegionCounters : public
RecursiveASTVisitor<MapRegionCounters> {
return true;
if (const Expr *E = dyn_cast<Expr>(S)) {
- const BinaryOperator *BinOp =
dyn_cast<BinaryOperator>(E->IgnoreParens());
+ const BinaryOperator *BinOp =
+ dyn_cast<BinaryOperator>(CodeGenFunction::stripCond(E));
if (BinOp && BinOp->isLogicalOp()) {
assert(LogOpStack.back() == BinOp);
LogOpStack.pop_back();
diff --git a/clang/test/CoverageMapping/mcdc-nested-expr.cpp
b/clang/test/CoverageMapping/mcdc-nested-expr.cpp
index bb82873e3b600d..0614a2b7ab8c10 100644
--- a/clang/test/CoverageMapping/mcdc-nested-expr.cpp
+++ b/clang/test/CoverageMapping/mcdc-nested-expr.cpp
@@ -21,8 +21,11 @@ bool func_condop(bool a, bool b, bool c) {
// Treated as parentheses.
// CHECK: func_expect{{.*}}:
bool func_expect(bool a, bool b, bool c) {
- // WARN: :[[@LINE+1]]:10: warning: unsupported MC/DC boolean expression;
contains an operation with a nested boolean expression.
return a || __builtin_expect(b && c, true);
+ // CHECK: Decision,File 0, [[@LINE-1]]:10 -> [[#L:@LINE-1]]:45 = M:4, C:3
+ // CHECK: Branch,File 0, [[#L]]:10 -> [[#L]]:11 = (#0 - #1), #1 [1,0,2]
+ // CHECK: Branch,File 0, [[#L]]:32 -> [[#L]]:33 = #2, (#1 - #2) [2,3,0]
+ // CHECK: Branch,File 0, [[#L]]:37 -> [[#L]]:38 = #3, (#2 - #3) [3,0,0]
}
// LNot among BinOp(s)
>From f2cf50e10b59d7d461967baef4d589c9282d0f6d Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi <geek4ci...@gmail.com>
Date: Sun, 2 Feb 2025 22:11:51 +0900
Subject: [PATCH 9/9] [MC/DC] Enable usage of `!` among `&&` and `||`
In the current implementation, `!(a || b) && c` was not treated as one
Decision with three terms.
Fixes #124563
---
clang/include/clang/AST/IgnoreExpr.h | 8 ++
clang/lib/CodeGen/CodeGenFunction.cpp | 12 ++-
clang/lib/CodeGen/CodeGenPGO.cpp | 8 +-
clang/lib/CodeGen/CoverageMappingGen.cpp | 12 +++
.../test/CoverageMapping/mcdc-nested-expr.cpp | 6 +-
clang/test/Profile/c-mcdc-not.c | 95 +++++++++++++++++++
6 files changed, 132 insertions(+), 9 deletions(-)
diff --git a/clang/include/clang/AST/IgnoreExpr.h
b/clang/include/clang/AST/IgnoreExpr.h
index 917bada61fa6fd..c48c0c0daf8151 100644
--- a/clang/include/clang/AST/IgnoreExpr.h
+++ b/clang/include/clang/AST/IgnoreExpr.h
@@ -134,6 +134,14 @@ inline Expr
*IgnoreElidableImplicitConstructorSingleStep(Expr *E) {
return E;
}
+inline Expr *IgnoreUOpLNotSingleStep(Expr *E) {
+ if (auto *UO = dyn_cast<UnaryOperator>(E)) {
+ if (UO->getOpcode() == UO_LNot)
+ return UO->getSubExpr();
+ }
+ return E;
+}
+
inline Expr *IgnoreImplicitAsWrittenSingleStep(Expr *E) {
if (auto *ICE = dyn_cast<ImplicitCastExpr>(E))
return ICE->getSubExprAsWritten();
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp
b/clang/lib/CodeGen/CodeGenFunction.cpp
index bbef277a524480..2c380ac926b1e7 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -27,6 +27,7 @@
#include "clang/AST/Decl.h"
#include "clang/AST/DeclCXX.h"
#include "clang/AST/Expr.h"
+#include "clang/AST/IgnoreExpr.h"
#include "clang/AST/StmtCXX.h"
#include "clang/AST/StmtObjC.h"
#include "clang/Basic/Builtins.h"
@@ -1748,12 +1749,13 @@ bool
CodeGenFunction::ConstantFoldsToSimpleInteger(const Expr *Cond,
/// Strip parentheses and simplistic logical-NOT operators.
const Expr *CodeGenFunction::stripCond(const Expr *C) {
- while (const UnaryOperator *Op = dyn_cast<UnaryOperator>(C->IgnoreParens()))
{
- if (Op->getOpcode() != UO_LNot)
- break;
- C = Op->getSubExpr();
+ while (true) {
+ const Expr *SC =
+ IgnoreExprNodes(C, IgnoreParensSingleStep, IgnoreUOpLNotSingleStep);
+ if (C == SC)
+ return SC;
+ C = SC;
}
- return C->IgnoreParens();
}
/// Determine whether the given condition is an instrumentable condition
diff --git a/clang/lib/CodeGen/CodeGenPGO.cpp b/clang/lib/CodeGen/CodeGenPGO.cpp
index 792373839107f0..0fd49b880bba30 100644
--- a/clang/lib/CodeGen/CodeGenPGO.cpp
+++ b/clang/lib/CodeGen/CodeGenPGO.cpp
@@ -247,8 +247,9 @@ struct MapRegionCounters : public
RecursiveASTVisitor<MapRegionCounters> {
}
if (const Expr *E = dyn_cast<Expr>(S)) {
- const BinaryOperator *BinOp =
dyn_cast<BinaryOperator>(E->IgnoreParens());
- if (BinOp && BinOp->isLogicalOp()) {
+ if (const auto *BinOp =
+ dyn_cast<BinaryOperator>(CodeGenFunction::stripCond(E));
+ BinOp && BinOp->isLogicalOp()) {
/// Check for "split-nested" logical operators. This happens when a new
/// boolean expression logical-op nest is encountered within an
existing
/// boolean expression, separated by a non-logical operator. For
@@ -280,7 +281,8 @@ struct MapRegionCounters : public
RecursiveASTVisitor<MapRegionCounters> {
return true;
if (const Expr *E = dyn_cast<Expr>(S)) {
- const BinaryOperator *BinOp =
dyn_cast<BinaryOperator>(E->IgnoreParens());
+ const BinaryOperator *BinOp =
+ dyn_cast<BinaryOperator>(CodeGenFunction::stripCond(E));
if (BinOp && BinOp->isLogicalOp()) {
assert(LogOpStack.back() == BinOp);
LogOpStack.pop_back();
diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp
b/clang/lib/CodeGen/CoverageMappingGen.cpp
index f09157771d2b5c..9bf73cf27a5fa9 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -799,6 +799,12 @@ struct MCDCCoverageBuilder {
/// Return the LHS Decision ([0,0] if not set).
const mcdc::ConditionIDs &back() const { return DecisionStack.back(); }
+ void swapConds() {
+ if (DecisionStack.empty())
+ return;
+
+ std::swap(DecisionStack.back()[false], DecisionStack.back()[true]);
+ }
/// Push the binary operator statement to track the nest level and assign IDs
/// to the operator's LHS and RHS. The RHS may be a larger subtree that is
/// broken up on successive levels.
@@ -2241,6 +2247,12 @@ struct CounterCoverageMappingBuilder
SM.isInSystemHeader(SM.getSpellingLoc(E->getEndLoc())));
}
+ void VisitUnaryLNot(const UnaryOperator *E) {
+ MCDCBuilder.swapConds();
+ Visit(E->getSubExpr());
+ MCDCBuilder.swapConds();
+ }
+
void VisitBinLAnd(const BinaryOperator *E) {
if (isExprInSystemHeader(E)) {
LeafExprSet.insert(E);
diff --git a/clang/test/CoverageMapping/mcdc-nested-expr.cpp
b/clang/test/CoverageMapping/mcdc-nested-expr.cpp
index bb82873e3b600d..a49dad0b336126 100644
--- a/clang/test/CoverageMapping/mcdc-nested-expr.cpp
+++ b/clang/test/CoverageMapping/mcdc-nested-expr.cpp
@@ -29,6 +29,10 @@ bool func_expect(bool a, bool b, bool c) {
// Doesn't split exprs.
// CHECK: func_lnot{{.*}}:
bool func_lnot(bool a, bool b, bool c, bool d) {
- // WARN: :[[@LINE+1]]:10: warning: unsupported MC/DC boolean expression;
contains an operation with a nested boolean expression.
return !(a || b) && !(c && d);
+ // CHECK: Decision,File 0, [[@LINE-1]]:10 -> [[#L:@LINE-1]]:32 = M:5, C:4
+ // CHECK: Branch,File 0, [[#L]]:12 -> [[#L]]:13 = (#0 - #2), #2 [1,0,3]
+ // CHECK: Branch,File 0, [[#L]]:17 -> [[#L]]:18 = (#2 - #3), #3 [3,0,2]
+ // CHECK: Branch,File 0, [[#L]]:25 -> [[#L]]:26 = #4, (#1 - #4) [2,4,0]
+ // CHECK: Branch,File 0, [[#L]]:30 -> [[#L]]:31 = #5, (#4 - #5) [4,0,0]
}
diff --git a/clang/test/Profile/c-mcdc-not.c b/clang/test/Profile/c-mcdc-not.c
index 7083aa226fb952..d6d79bf15cb086 100644
--- a/clang/test/Profile/c-mcdc-not.c
+++ b/clang/test/Profile/c-mcdc-not.c
@@ -85,3 +85,98 @@ int test(int a, int b, int c, int d, int e, int f) {
// MCDC: %[[BITS:.+]] = load i8, ptr %[[LAB4]], align 1
// MCDC: %[[LAB8:[0-9]+]] = or i8 %[[BITS]], %[[LAB7]]
// MCDC: store i8 %[[LAB8]], ptr %[[LAB4]], align 1
+
+int internot(int a, int b, int c, int d, int e, int f) {
+ return !(!(!a && b) || !(!!(!c && d) || !(e && !f)));
+}
+
+// MCDC-LABEL: @internot(
+// MCDC-DAG: store i32 0, ptr %mcdc.addr, align 4
+
+// Branch #2, (#0 - #2) [1,3,0]
+// !a [+0 => b][+6 => END]
+// MCDC-DAG: %[[A:.+]] = load i32, ptr %a.addr, align 4
+// MCDC-DAG: %[[AB:.+]] = icmp ne i32 %[[A]], 0
+// MCDC-DAG: %[[AN:.+]] = xor i1 %[[AB]], true
+// MCDC-DAG: %[[M1:.+]] = load i32, ptr %mcdc.addr, align 4
+// MCDC-DAG: %[[M1T:.+]] = add i32 %[[M1]], 0
+// MCDC-DAG: %[[M1F:.+]] = add i32 %[[M1]], 6
+// MCDC-DAG: %[[M1S:.+]] = select i1 %[[AN]], i32 %[[M1T]], i32 %[[M1F]]
+// MCDC-DAG: store i32 %[[M1S]], ptr %mcdc.addr, align 4
+
+// Branch #3, (#2 - #3) [3,2,0]
+// b [+0 => c][+7 => END]
+// MCDC-DAG: %[[B:.+]] = load i32, ptr %b.addr, align 4
+// MCDC-DAG: %[[BB:.+]] = icmp ne i32 %[[B]], 0
+// MCDC-DAG: %[[M3:.+]] = load i32, ptr %mcdc.addr, align 4
+// MCDC-DAG: %[[M3T:.+]] = add i32 %[[M3]], 0
+// MCDC-DAG: %[[M3F:.+]] = add i32 %[[M3]], 7
+// MCDC-DAG: %[[M3S:.+]] = select i1 %[[BB]], i32 %[[M3T]], i32 %[[M3F]]
+// MCDC-DAG: store i32 %[[M3S]], ptr %mcdc.addr, align 4
+
+// Branch #5, (#1 - #5) [2,5,4]
+// !c [+0 => d][+0 => e]
+// MCDC-DAG: %[[C:.+]] = load i32, ptr %c.addr, align 4
+// MCDC-DAG: %[[CB:.+]] = icmp ne i32 %[[C]], 0
+// MCDC-DAG: %[[CN:.+]] = xor i1 %[[CB]], true
+// MCDC-DAG: %[[M2:.+]] = load i32, ptr %mcdc.addr, align 4
+// MCDC-DAG: %[[M2T:.+]] = add i32 %[[M2]], 0
+// MCDC-DAG: %[[M2F:.+]] = add i32 %[[M2]], 0
+// MCDC-DAG: %[[M2S:.+]] = select i1 %[[CN]], i32 %[[M2T]], i32 %[[M2F]]
+// MCDC-DAG: store i32 %[[M2S]], ptr %mcdc.addr, align 4
+
+// Branch #6, (#5 - #6) [5,0,4]
+// d [+8 => END][+1 => e]]
+// MCDC-DAG: %[[D:.+]] = load i32, ptr %d.addr, align 4
+// MCDC-DAG: %[[DB:.+]] = icmp ne i32 %[[D]], 0
+// MCDC-DAG: %[[M5:.+]] = load i32, ptr %mcdc.addr, align 4
+// MCDC-DAG: %[[M5T:.+]] = add i32 %[[M5]], 8
+// MCDC-DAG: %[[M5F:.+]] = add i32 %[[M5]], 1
+// MCDC-DAG: %[[M5S:.+]] = select i1 %[[DB]], i32 %[[M5T]], i32 %[[M5F]]
+// MCDC-DAG: store i32 %[[M5S]], ptr %mcdc.addr, align 4
+
+// Branch #7, (#4 - #7) [4,6,0]
+// e [+0 => f][+0 => END]
+// from:
+// [c => +0]
+// [d => +1]
+// MCDC-DAG: %[[E:.+]] = load i32, ptr %e.addr, align 4
+// MCDC-DAG: %[[EB:.+]] = icmp ne i32 %[[E]], 0
+// MCDC-DAG: %[[M4:.+]] = load i32, ptr %mcdc.addr, align 4
+// MCDC-DAG: %[[M4T:.+]] = add i32 %[[M4]], 0
+// MCDC-DAG: %[[M4F:.+]] = add i32 %[[M4]], 0
+// MCDC-DAG: %[[M4S:.+]] = select i1 %[[EB]], i32 %[[M4T]], i32 %[[M4F]]
+// MCDC-DAG: store i32 %[[M4S]], ptr %mcdc.addr, align 4
+
+// Branch #8, (#7 - #8) [6,0,0]
+// !f [+4 => END][+2 => END]
+// MCDC-DAG: %[[F:.+]] = load i32, ptr %f.addr, align 4
+// MCDC-DAG: %[[FB:.+]] = icmp ne i32 %[[F]], 0
+// MCDC-DAG: %[[FN:.+]] = xor i1 %[[FB]], true
+// MCDC-DAG: %[[M6:.+]] = load i32, ptr %mcdc.addr, align 4
+// MCDC-DAG: %[[M6T:.+]] = add i32 %[[M6]], 4
+// MCDC-DAG: %[[M6F:.+]] = add i32 %[[M6]], 2
+// MCDC-DAG: %[[M6S:.+]] = select i1 %[[FN]], i32 %[[M6T]], i32 %[[M6F]]
+// MCDC-DAG: store i32 %[[M6S]], ptr %mcdc.addr, align 4
+
+// from:
+// [e => +0]
+// [f => +2]
+// [c => +0]
+// [d => +1]
+// [f => +4]
+// [c => +0]
+// [d => +1]
+// [a => +6]
+// [b => +7]
+// [d => +8]
+// MCDC-DAG: %[[T0:.+]] = load i32, ptr %mcdc.addr, align 4
+// MCDC-DAG: %[[T:.+]] = add i32 %[[T0]], 0
+// MCDC-DAG: %[[TA:.+]] = lshr i32 %[[T]], 3
+// MCDC-DAG: %[[BA:.+]] = getelementptr inbounds i8, ptr @__profbm_internot,
i32 %[[TA]]
+// MCDC-DAG: %[[BI:.+]] = and i32 %[[T]], 7
+// MCDC-DAG: %[[BI1:.+]] = trunc i32 %[[BI]] to i8
+// MCDC-DAG: %[[BM:.+]] = shl i8 1, %[[BI1]]
+// MCDC-DAG: %mcdc.bits = load i8, ptr %[[BA]], align 1
+// MCDC-DAG: %[[BN:.+]] = or i8 %mcdc.bits, %[[BM]]
+// MCDC-DAG: store i8 %[[BN]], ptr %[[BA]], align 1
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
