[clang] [clang][CodeGen] Keep processing the rest of AST after encountering unsupported MC/DC expressions (PR #82464)

2024-02-22 Thread Wentao Zhang via cfe-commits 

https://github.com/whentojump closed 
https://github.com/llvm/llvm-project/pull/82464
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][CodeGen] Keep processing the rest of AST after encountering unsupported MC/DC expressions (PR #82464)

2024-02-22 Thread Alan Phipps via cfe-commits 


@@ -292,7 +295,7 @@ struct MapRegionCounters : public 
RecursiveASTVisitor {
 "contains an operation with a nested boolean expression. "
 "Expression will not be covered");
 Diag.Report(S->getBeginLoc(), DiagID);
-return false;
+return true;

evodius96 wrote:

Yes, that's what I mean.  There was no benefit in returning false to begin 
with; you have to return true.  The checking that the pass does to catch the 
case, issue the diagnostic, and avoid processing of the data for the expression 
is sufficient to handle the case.

https://github.com/llvm/llvm-project/pull/82464
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][CodeGen] Keep processing the rest of AST after encountering unsupported MC/DC expressions (PR #82464)

2024-02-22 Thread Wentao Zhang via cfe-commits 


@@ -292,7 +295,7 @@ struct MapRegionCounters : public 
RecursiveASTVisitor {
 "contains an operation with a nested boolean expression. "
 "Expression will not be covered");
 Diag.Report(S->getBeginLoc(), DiagID);
-return false;
+return true;

whentojump wrote:

Actually my point is: returning `false` would exit early from the AST 
traversal, and I suppose both places *have to* return `true`. Otherwise we'd 
observe things in my original post.




https://github.com/llvm/llvm-project/pull/82464
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][CodeGen] Keep processing the rest of AST after encountering unsupported MC/DC expressions (PR #82464)

2024-02-22 Thread Alan Phipps via cfe-commits 

https://github.com/evodius96 approved this pull request.

LGTM. thanks for catching and fixing this.  Would also be good to get this onto 
18.x if possible.

https://github.com/llvm/llvm-project/pull/82464
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][CodeGen] Keep processing the rest of AST after encountering unsupported MC/DC expressions (PR #82464)

2024-02-22 Thread Alan Phipps via cfe-commits 


@@ -239,9 +239,12 @@ struct MapRegionCounters : public 
RecursiveASTVisitor {
 if (MCDCMaxCond == 0)
   return true;
 
-/// At the top of the logical operator nest, reset the number of 
conditions.
-if (LogOpStack.empty())
+/// 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;

evodius96 wrote:

Definitely a bug that we weren't resetting this. 

https://github.com/llvm/llvm-project/pull/82464
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][CodeGen] Keep processing the rest of AST after encountering unsupported MC/DC expressions (PR #82464)

2024-02-22 Thread Alan Phipps via cfe-commits 


@@ -292,7 +295,7 @@ struct MapRegionCounters : public 
RecursiveASTVisitor {
 "contains an operation with a nested boolean expression. "
 "Expression will not be covered");
 Diag.Report(S->getBeginLoc(), DiagID);
-return false;
+return true;

evodius96 wrote:

Yes, I guess there is no value in returning false in either of these cases.  
This is enough to avoid processing the data for the expression.

https://github.com/llvm/llvm-project/pull/82464
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][CodeGen] Keep processing the rest of AST after encountering unsupported MC/DC expressions (PR #82464)

2024-02-22 Thread Alan Phipps via cfe-commits 


@@ -239,9 +239,12 @@ struct MapRegionCounters : public 
RecursiveASTVisitor {
 if (MCDCMaxCond == 0)
   return true;
 
-/// At the top of the logical operator nest, reset the number of 
conditions.
-if (LogOpStack.empty())
+/// 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;

evodius96 wrote:

Reset of SplitNestedLogicalOp could also probably be done in 
dataTraverseStmtPost() at the point where the diagnostic is emitted, but this 
is OK.

https://github.com/llvm/llvm-project/pull/82464
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][CodeGen] Keep processing the rest of AST after encountering unsupported MC/DC expressions (PR #82464)

2024-02-20 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: Wentao Zhang (whentojump)


Changes

Currently, upon seeing [unsupported 
decisions](https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#mc-dc-instrumentation)
 (more than 6 conditions, or split nesting), the post-visitor hook 
`dataTraverseStmtPost()` returns a `false`. As a result, in the rest of tree 
even supported decisions will be skipped as well. Like in the below code:

```c
{ // CompoundStmt
  a && b;   // 1: BinaryOperator (supported)
  a && foo(b && c); // 2: BinaryOperator (not yet supported due 
to split nesting)
  a && b;   // 3: BinaryOperator (supported)
}
```

Decision 3 will not be processed at all. And only one "Decision" region will be 
emitted. Compiler explorer example: https://godbolt.org/z/Px61sesoo

We hope to process such cases and emit two "Decision" regions (1 and 3) in the 
above example.

---
Full diff: https://github.com/llvm/llvm-project/pull/82464.diff


1 Files Affected:

- (modified) clang/lib/CodeGen/CodeGenPGO.cpp (+7-4) 


``diff
diff --git a/clang/lib/CodeGen/CodeGenPGO.cpp b/clang/lib/CodeGen/CodeGenPGO.cpp
index 48c5e68a3b7ba4..1ef7be3c72593d 100644
--- a/clang/lib/CodeGen/CodeGenPGO.cpp
+++ b/clang/lib/CodeGen/CodeGenPGO.cpp
@@ -239,9 +239,12 @@ struct MapRegionCounters : public 
RecursiveASTVisitor {
 if (MCDCMaxCond == 0)
   return true;
 
-/// At the top of the logical operator nest, reset the number of 
conditions.
-if (LogOpStack.empty())
+/// 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(S)) {
   const BinaryOperator *BinOp = 
dyn_cast(E->IgnoreParens());
@@ -292,7 +295,7 @@ struct MapRegionCounters : public 
RecursiveASTVisitor {
 "contains an operation with a nested boolean expression. "
 "Expression will not be covered");
 Diag.Report(S->getBeginLoc(), DiagID);
-return false;
+return true;
   }
 
   /// Was the maximum number of conditions encountered?
@@ -303,7 +306,7 @@ struct MapRegionCounters : public 
RecursiveASTVisitor {
 "number of conditions (%0) exceeds max (%1). "
 "Expression will not be covered");
 Diag.Report(S->getBeginLoc(), DiagID) << NumCond << MCDCMaxCond;
-return false;
+return true;
   }
 
   // Otherwise, allocate the number of bytes required for the bitmap

``




https://github.com/llvm/llvm-project/pull/82464
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][CodeGen] Keep processing the rest of AST after encountering unsupported MC/DC expressions (PR #82464)

2024-02-20 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-clang-codegen

Author: Wentao Zhang (whentojump)


Changes

Currently, upon seeing [unsupported 
decisions](https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#mc-dc-instrumentation)
 (more than 6 conditions, or split nesting), the post-visitor hook 
`dataTraverseStmtPost()` returns a `false`. As a result, in the rest of tree 
even supported decisions will be skipped as well. Like in the below code:

```c
{ // CompoundStmt
  a && b;   // 1: BinaryOperator (supported)
  a && foo(b && c); // 2: BinaryOperator (not yet supported due 
to split nesting)
  a && b;   // 3: BinaryOperator (supported)
}
```

Decision 3 will not be processed at all. And only one "Decision" region will be 
emitted. Compiler explorer example: https://godbolt.org/z/Px61sesoo

We hope to process such cases and emit two "Decision" regions (1 and 3) in the 
above example.

---
Full diff: https://github.com/llvm/llvm-project/pull/82464.diff


1 Files Affected:

- (modified) clang/lib/CodeGen/CodeGenPGO.cpp (+7-4) 


``diff
diff --git a/clang/lib/CodeGen/CodeGenPGO.cpp b/clang/lib/CodeGen/CodeGenPGO.cpp
index 48c5e68a3b7ba4..1ef7be3c72593d 100644
--- a/clang/lib/CodeGen/CodeGenPGO.cpp
+++ b/clang/lib/CodeGen/CodeGenPGO.cpp
@@ -239,9 +239,12 @@ struct MapRegionCounters : public 
RecursiveASTVisitor {
 if (MCDCMaxCond == 0)
   return true;
 
-/// At the top of the logical operator nest, reset the number of 
conditions.
-if (LogOpStack.empty())
+/// 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(S)) {
   const BinaryOperator *BinOp = 
dyn_cast(E->IgnoreParens());
@@ -292,7 +295,7 @@ struct MapRegionCounters : public 
RecursiveASTVisitor {
 "contains an operation with a nested boolean expression. "
 "Expression will not be covered");
 Diag.Report(S->getBeginLoc(), DiagID);
-return false;
+return true;
   }
 
   /// Was the maximum number of conditions encountered?
@@ -303,7 +306,7 @@ struct MapRegionCounters : public 
RecursiveASTVisitor {
 "number of conditions (%0) exceeds max (%1). "
 "Expression will not be covered");
 Diag.Report(S->getBeginLoc(), DiagID) << NumCond << MCDCMaxCond;
-return false;
+return true;
   }
 
   // Otherwise, allocate the number of bytes required for the bitmap

``




https://github.com/llvm/llvm-project/pull/82464
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][CodeGen] Keep processing the rest of AST after encountering unsupported MC/DC expressions (PR #82464)

2024-02-20 Thread Wentao Zhang via cfe-commits 

https://github.com/whentojump created 
https://github.com/llvm/llvm-project/pull/82464

Currently, upon seeing [unsupported 
decisions](https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#mc-dc-instrumentation)
 (more than 6 conditions, or split nesting), the post-visitor hook 
`dataTraverseStmtPost()` returns a `false`. As a result, in the rest of tree 
even supported decisions will be skipped as well. Like in the below code:

```c
{ // CompoundStmt
  a && b;   // 1: BinaryOperator (supported)
  a && foo(b && c); // 2: BinaryOperator (not yet supported due to split 
nesting)
  a && b;   // 3: BinaryOperator (supported)
}
```

Decision 3 will not be processed at all. And only one "Decision" region will be 
emitted. Compiler explorer example: https://godbolt.org/z/Px61sesoo

We hope to process such cases and emit two "Decision" regions (1 and 3) in the 
above example.

>From 67a2c10b044e0e9c2eceb6646300e546ee990e0b Mon Sep 17 00:00:00 2001
From: Wentao Zhang 
Date: Wed, 21 Feb 2024 01:48:11 -0500
Subject: [PATCH] [clang][CodeGen] Keep processing the rest of AST after
 encountering unsupported MC/DC expressions

---
 clang/lib/CodeGen/CodeGenPGO.cpp | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/clang/lib/CodeGen/CodeGenPGO.cpp b/clang/lib/CodeGen/CodeGenPGO.cpp
index 48c5e68a3b7ba4..1ef7be3c72593d 100644
--- a/clang/lib/CodeGen/CodeGenPGO.cpp
+++ b/clang/lib/CodeGen/CodeGenPGO.cpp
@@ -239,9 +239,12 @@ struct MapRegionCounters : public 
RecursiveASTVisitor {
 if (MCDCMaxCond == 0)
   return true;
 
-/// At the top of the logical operator nest, reset the number of 
conditions.
-if (LogOpStack.empty())
+/// 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(S)) {
   const BinaryOperator *BinOp = 
dyn_cast(E->IgnoreParens());
@@ -292,7 +295,7 @@ struct MapRegionCounters : public 
RecursiveASTVisitor {
 "contains an operation with a nested boolean expression. "
 "Expression will not be covered");
 Diag.Report(S->getBeginLoc(), DiagID);
-return false;
+return true;
   }
 
   /// Was the maximum number of conditions encountered?
@@ -303,7 +306,7 @@ struct MapRegionCounters : public 
RecursiveASTVisitor {
 "number of conditions (%0) exceeds max (%1). "
 "Expression will not be covered");
 Diag.Report(S->getBeginLoc(), DiagID) << NumCond << MCDCMaxCond;
-return false;
+return true;
   }
 
   // Otherwise, allocate the number of bytes required for the bitmap

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits