[clang][CodeGen] Keep processing the rest of AST after encountering unsupported MC/DC expressions

[whentojump]

Currently, upon seeing unsupported decisions (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:

{ // 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.

[llvmbot]

@llvm/pr-subscribers-clang-codegen

Author: Wentao Zhang (whentojump)

Changes

Currently, upon seeing unsupported decisions (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:

{ // 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 --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<MapRegionCounters> {
     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<Expr>(S)) {
       const BinaryOperator *BinOp = dyn_cast<BinaryOperator>(E->IgnoreParens());
@@ -292,7 +295,7 @@ struct MapRegionCounters : public RecursiveASTVisitor<MapRegionCounters> {
                 "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<MapRegionCounters> {
                 "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

[llvmbot]

@llvm/pr-subscribers-clang

Author: Wentao Zhang (whentojump)

Changes

Currently, upon seeing unsupported decisions (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:

{ // 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 --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<MapRegionCounters> {
     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<Expr>(S)) {
       const BinaryOperator *BinOp = dyn_cast<BinaryOperator>(E->IgnoreParens());
@@ -292,7 +295,7 @@ struct MapRegionCounters : public RecursiveASTVisitor<MapRegionCounters> {
                 "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<MapRegionCounters> {
                 "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

[evodius96]

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