[clang] [llvm] [MC/DC] Refactor: Introduce `ConditionIDs` as `std::array<2>` (PR #81221)

[Alan Phipps via cfe-commits]

@@ -217,6 +217,9 @@ class CounterExpressionBuilder {
 
 using LineColPair = std::pair;
 
+using MCDCConditionID = unsigned int;
+using MCDCConditionIDs = std::array;

evodius96 wrote:

is `std::array<>` better than `std::pair<>` when it only has two elements?

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

[clang] [llvm] [MC/DC] Refactor: Introduce `ConditionIDs` as `std::array<2>` (PR #81221)

[Alan Phipps via cfe-commits]

@@ -217,6 +217,9 @@ class CounterExpressionBuilder {
 
 using LineColPair = std::pair;
 
+using MCDCConditionID = unsigned int;
+using MCDCConditionIDs = std::array;

evodius96 wrote:

Ah, well it's iterable.

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

[clang] clangCodeGen: Introduce `mcdc::State` with `MCDCState.h` (PR #81497)

[Alan Phipps via cfe-commits]

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

I don't know if anyone else has any preference around the additional 
mcdc-specific header files, but generally it looks good to me.

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

[clang] [llvm] [MC/DC] Refactor: Introduce `ConditionIDs` as `std::array<2>` (PR #81221)

[Alan Phipps via cfe-commits]

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


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

[clang] [llvm] [MC/DC][Coverage] Loosen the limit of NumConds from 6 (PR #82448)

[Alan Phipps via cfe-commits]

@@ -1201,19 +1197,22 @@ void CodeGenPGO::emitMCDCCondBitmapUpdate(CGBuilderTy 
&Builder, const Expr *S,
   // Extract the ID of the condition we are setting in the bitmap.
   const auto &Branch = BranchStateIter->second;
   assert(Branch.ID >= 0 && "Condition has no ID!");
+  assert(Branch.DecisionStmt);
 
-  auto *I8PtrTy = llvm::PointerType::getUnqual(CGM.getLLVMContext());
+  // Cancel the emission if the Decision is erased after the allocation.
+  const auto DecisionIter =
+  RegionMCDCState->DecisionByStmt.find(Branch.DecisionStmt);
+  if (DecisionIter == RegionMCDCState->DecisionByStmt.end())
+return;
 
-  // Emit intrinsic that updates a dedicated temporary value on the stack after
-  // a condition is evaluated. After the set of conditions has been updated,
-  // the resulting value is used to update the boolean expression's bitmap.
-  llvm::Value *Args[5] = {llvm::ConstantExpr::getBitCast(FuncNameVar, I8PtrTy),
-  Builder.getInt64(FunctionHash),
-  Builder.getInt32(Branch.ID),
-  MCDCCondBitmapAddr.getPointer(), Val};
-  Builder.CreateCall(
-  CGM.getIntrinsic(llvm::Intrinsic::instrprof_mcdc_condbitmap_update),
-  Args);
+  const auto &TVIdxs = DecisionIter->second.Indices[Branch.ID];
+
+  auto *CurTV = Builder.CreateLoad(MCDCCondBitmapAddr,
+   "mcdc." + Twine(Branch.ID + 1) + ".cur");
+  auto *NewTV = Builder.CreateAdd(CurTV, Builder.getInt32(TVIdxs[true]));
+  NewTV = Builder.CreateSelect(
+  Val, NewTV, Builder.CreateAdd(CurTV, Builder.getInt32(TVIdxs[false])));
+  Builder.CreateStore(NewTV, MCDCCondBitmapAddr);

evodius96 wrote:

It's probably OK to remove the intrinsic if it is no longer used or needed 
unless you want to change it.

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

[clang] [llvm] [MC/DC][Coverage] Loosen the limit of NumConds from 6 (PR #82448)

[Alan Phipps via cfe-commits]

@@ -983,7 +979,7 @@ void CodeGenPGO::mapRegionCounters(const Decl *D) {
   // for most embedded applications. Setting a maximum value prevents the
   // bitmap footprint from growing too large without the user's knowledge. In
   // the future, this value could be adjusted with a command-line option.
-  unsigned MCDCMaxConditions = (CGM.getCodeGenOpts().MCDCCoverage) ? 6 : 0;
+  unsigned MCDCMaxConditions = (CGM.getCodeGenOpts().MCDCCoverage) ? 32767 : 0;

evodius96 wrote:

I know you will do this later, but I also wanted to echo the desire for a means 
to control the condition limit to warn the user against unintended memory 
footprint expansion, most useful in embedded development contexts.  It may not 
otherwise be obvious to most users what is contributing to larger memory usage. 
We could also override the default in downstream toolchains.

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

[clang] [llvm] [MC/DC][Coverage] Loosen the limit of NumConds from 6 (PR #82448)

[Alan Phipps via cfe-commits]

@@ -1201,19 +1197,22 @@ void CodeGenPGO::emitMCDCCondBitmapUpdate(CGBuilderTy 
&Builder, const Expr *S,
   // Extract the ID of the condition we are setting in the bitmap.
   const auto &Branch = BranchStateIter->second;
   assert(Branch.ID >= 0 && "Condition has no ID!");
+  assert(Branch.DecisionStmt);
 
-  auto *I8PtrTy = llvm::PointerType::getUnqual(CGM.getLLVMContext());
+  // Cancel the emission if the Decision is erased after the allocation.
+  const auto DecisionIter =
+  RegionMCDCState->DecisionByStmt.find(Branch.DecisionStmt);
+  if (DecisionIter == RegionMCDCState->DecisionByStmt.end())
+return;
 
-  // Emit intrinsic that updates a dedicated temporary value on the stack after
-  // a condition is evaluated. After the set of conditions has been updated,
-  // the resulting value is used to update the boolean expression's bitmap.
-  llvm::Value *Args[5] = {llvm::ConstantExpr::getBitCast(FuncNameVar, I8PtrTy),
-  Builder.getInt64(FunctionHash),
-  Builder.getInt32(Branch.ID),
-  MCDCCondBitmapAddr.getPointer(), Val};
-  Builder.CreateCall(
-  CGM.getIntrinsic(llvm::Intrinsic::instrprof_mcdc_condbitmap_update),
-  Args);
+  const auto &TVIdxs = DecisionIter->second.Indices[Branch.ID];
+
+  auto *CurTV = Builder.CreateLoad(MCDCCondBitmapAddr,
+   "mcdc." + Twine(Branch.ID + 1) + ".cur");
+  auto *NewTV = Builder.CreateAdd(CurTV, Builder.getInt32(TVIdxs[true]));
+  NewTV = Builder.CreateSelect(
+  Val, NewTV, Builder.CreateAdd(CurTV, Builder.getInt32(TVIdxs[false])));
+  Builder.CreateStore(NewTV, MCDCCondBitmapAddr);

evodius96 wrote:

Makes sense.  I left enabling atomic operations on tvbitmap update as a TBD.

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

[llvm] [clang] `llvm-cov` assertion failure when handling MC/DC that involves macros (PR #80098)

[Alan Phipps via cfe-commits]

evodius96 wrote:

Hi! I think this may be the same issue as 
https://github.com/llvm/llvm-project/issues/77871 and a fix is under review by 
@chapuni  Can you verify?

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

[libcxx] [llvm] [libc] [flang] [compiler-rt] [clang] [clang-tools-extra] [lldb] [llvm-cov] Simplify and optimize MC/DC computation (PR #79727)

[Alan Phipps via cfe-commits]

evodius96 wrote:

Added a couple of comments, but otherwise LGTM.  Thank you for optimizing this.

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

[compiler-rt] [lldb] [clang] [flang] [llvm] [libc] [libcxx] [clang-tools-extra] [llvm-cov] Simplify and optimize MC/DC computation (PR #79727)

[Alan Phipps via cfe-commits]

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


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

[clang] [llvm] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)

[Alan Phipps via cfe-commits]

https://github.com/evodius96 created 
https://github.com/llvm/llvm-project/pull/78202

Clean-up of the algorithm that assigns MC/DC True/False control-flow condition 
IDs when constructing an MC/DC decision region.  This patch creates a common 
API for setting/getting the condition IDs, making the binary logical operator 
visitor functions much cleaner.

This patch also fixes a bug in which the decision which was malformed due to an 
incorrect calculation of the True/False condition IDs. Previously, this 
calculation was done as the logical operation stack was being popped, yielding 
off-by-one errors.  This is now done when the logical operation stack is being 
pushed.

>From 8751417f4889a120193a604e2ea91627f3b064e8 Mon Sep 17 00:00:00 2001
From: Alan Phipps 
Date: Mon, 15 Jan 2024 12:24:36 -0600
Subject: [PATCH] [clang][CoverageMapping] Refactor when setting MC/DC
 True/False Condition IDs

Clean-up of the algorithm that assigns MC/DC True/False control-flow
condition IDs when constructing an MC/DC decision region.  This patch creates a
common API for setting/getting the condition IDs, making the binary logical
operator visitor functions much cleaner.

This patch also fixes a bug in which the decision which was malformed due to an
incorrect calculation of the True/False condition IDs. Previously, this
calculation was done as the logical operation stack was being popped, yielding
off-by-one errors.  This is now done when the logical operation stack is being
pushed.
---
 clang/lib/CodeGen/CoverageMappingGen.cpp  | 127 +++---
 .../mcdc-logical-stmt-ids-all.cpp |  50 +++
 .../ProfileData/Coverage/CoverageMapping.h|   2 +-
 3 files changed, 133 insertions(+), 46 deletions(-)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp 
b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 916016601a9327..ca7b65e3f4b08d 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -668,6 +668,8 @@ struct MCDCCoverageBuilder {
   llvm::SmallVector NestLevel;
   llvm::DenseMap &CondIDs;
   llvm::DenseMap &MCDCBitmapMap;
+  llvm::DenseMap TrueCondIDs;
+  llvm::DenseMap FalseCondIDs;
   MCDCConditionID NextID = 1;
   bool NotMapped = false;
 
@@ -713,6 +715,21 @@ struct MCDCCoverageBuilder {
 return AndRHS.empty() ? 0 : AndRHS.back();
   }
 
+  /// Set the given condition's ID.
+  void setCondID(const Expr *Cond, MCDCConditionID ID) {
+  CondIDs[CodeGenFunction::stripCond(Cond)] = ID;
+  }
+
+  /// Set the ID of the next condition when the given condition is True.
+  void setTrueCondID(const Expr *Cond, MCDCConditionID ID) {
+  TrueCondIDs[CodeGenFunction::stripCond(Cond)] = ID;
+  }
+
+  /// Set the ID of the next condition when the given condition is False.
+  void setFalseCondID(const Expr *Cond, MCDCConditionID ID) {
+  FalseCondIDs[CodeGenFunction::stripCond(Cond)] = ID;
+  }
+
   /// Return the ID of a given condition.
   MCDCConditionID getCondID(const Expr *Cond) const {
 auto I = CondIDs.find(CodeGenFunction::stripCond(Cond));
@@ -722,6 +739,24 @@ struct MCDCCoverageBuilder {
   return I->second;
   }
 
+  /// Return the ID of the next condition when the given condition is True.
+  MCDCConditionID getNextIfTrueCondID(const Expr *Cond) const {
+auto I = TrueCondIDs.find(CodeGenFunction::stripCond(Cond));
+if (I == TrueCondIDs.end())
+  return 0;
+else
+  return I->second;
+  }
+
+  /// Return the ID of the next condition when the given condition is False.
+  MCDCConditionID getNextIfFalseCondID(const Expr *Cond) const {
+auto I = FalseCondIDs.find(CodeGenFunction::stripCond(Cond));
+if (I == FalseCondIDs.end())
+  return 0;
+else
+  return I->second;
+  }
+
   /// 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.
@@ -746,7 +781,7 @@ struct MCDCCoverageBuilder {
 // assign that ID to its LHS node.  Its RHS will receive a new ID.
 if (CondIDs.contains(CodeGenFunction::stripCond(E))) {
   // If Stmt has an ID, assign its ID to LHS
-  CondIDs[CodeGenFunction::stripCond(E->getLHS())] = CondIDs[E];
+  setCondID(E->getLHS(), getCondID(E));
 
   // Since the operator's LHS assumes the operator's same ID, pop the
   // operator from the RHS stack so that if LHS short-circuits, it won't be
@@ -754,13 +789,44 @@ struct MCDCCoverageBuilder {
   popRHSifTop(E);
 } else {
   // Otherwise, assign ID+1 to LHS.
-  CondIDs[CodeGenFunction::stripCond(E->getLHS())] = NextID++;
+  setCondID(E->getLHS(), NextID++);
 }
 
 // Assign ID+1 to RHS.
-CondIDs[CodeGenFunction::stripCond(E->getRHS())] = NextID++;
+setCondID(E->getRHS(), NextID++);
+
+// Assign the True and False condition IDs for the LHS and RHS.
+if (isLAnd(E)) {
+  // For logical-AND ("LHS && RHS"):
+  // - If LHS is TRU

[llvm] [clang] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)

[Alan Phipps via cfe-commits]

evodius96 wrote:

This fixes issue reported here: 
https://github.com/llvm/llvm-project/issues/77873

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

[clang] [llvm] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)

[Alan Phipps via cfe-commits]

evodius96 wrote:

I'll address the clang-format issues

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

[clang] [llvm] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)

[Alan Phipps via cfe-commits]

https://github.com/evodius96 updated 
https://github.com/llvm/llvm-project/pull/78202

>From 8751417f4889a120193a604e2ea91627f3b064e8 Mon Sep 17 00:00:00 2001
From: Alan Phipps 
Date: Mon, 15 Jan 2024 12:24:36 -0600
Subject: [PATCH 1/2] [clang][CoverageMapping] Refactor when setting MC/DC
 True/False Condition IDs

Clean-up of the algorithm that assigns MC/DC True/False control-flow
condition IDs when constructing an MC/DC decision region.  This patch creates a
common API for setting/getting the condition IDs, making the binary logical
operator visitor functions much cleaner.

This patch also fixes a bug in which the decision which was malformed due to an
incorrect calculation of the True/False condition IDs. Previously, this
calculation was done as the logical operation stack was being popped, yielding
off-by-one errors.  This is now done when the logical operation stack is being
pushed.
---
 clang/lib/CodeGen/CoverageMappingGen.cpp  | 127 +++---
 .../mcdc-logical-stmt-ids-all.cpp |  50 +++
 .../ProfileData/Coverage/CoverageMapping.h|   2 +-
 3 files changed, 133 insertions(+), 46 deletions(-)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp 
b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 916016601a93276..ca7b65e3f4b08d7 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -668,6 +668,8 @@ struct MCDCCoverageBuilder {
   llvm::SmallVector NestLevel;
   llvm::DenseMap &CondIDs;
   llvm::DenseMap &MCDCBitmapMap;
+  llvm::DenseMap TrueCondIDs;
+  llvm::DenseMap FalseCondIDs;
   MCDCConditionID NextID = 1;
   bool NotMapped = false;
 
@@ -713,6 +715,21 @@ struct MCDCCoverageBuilder {
 return AndRHS.empty() ? 0 : AndRHS.back();
   }
 
+  /// Set the given condition's ID.
+  void setCondID(const Expr *Cond, MCDCConditionID ID) {
+  CondIDs[CodeGenFunction::stripCond(Cond)] = ID;
+  }
+
+  /// Set the ID of the next condition when the given condition is True.
+  void setTrueCondID(const Expr *Cond, MCDCConditionID ID) {
+  TrueCondIDs[CodeGenFunction::stripCond(Cond)] = ID;
+  }
+
+  /// Set the ID of the next condition when the given condition is False.
+  void setFalseCondID(const Expr *Cond, MCDCConditionID ID) {
+  FalseCondIDs[CodeGenFunction::stripCond(Cond)] = ID;
+  }
+
   /// Return the ID of a given condition.
   MCDCConditionID getCondID(const Expr *Cond) const {
 auto I = CondIDs.find(CodeGenFunction::stripCond(Cond));
@@ -722,6 +739,24 @@ struct MCDCCoverageBuilder {
   return I->second;
   }
 
+  /// Return the ID of the next condition when the given condition is True.
+  MCDCConditionID getNextIfTrueCondID(const Expr *Cond) const {
+auto I = TrueCondIDs.find(CodeGenFunction::stripCond(Cond));
+if (I == TrueCondIDs.end())
+  return 0;
+else
+  return I->second;
+  }
+
+  /// Return the ID of the next condition when the given condition is False.
+  MCDCConditionID getNextIfFalseCondID(const Expr *Cond) const {
+auto I = FalseCondIDs.find(CodeGenFunction::stripCond(Cond));
+if (I == FalseCondIDs.end())
+  return 0;
+else
+  return I->second;
+  }
+
   /// 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.
@@ -746,7 +781,7 @@ struct MCDCCoverageBuilder {
 // assign that ID to its LHS node.  Its RHS will receive a new ID.
 if (CondIDs.contains(CodeGenFunction::stripCond(E))) {
   // If Stmt has an ID, assign its ID to LHS
-  CondIDs[CodeGenFunction::stripCond(E->getLHS())] = CondIDs[E];
+  setCondID(E->getLHS(), getCondID(E));
 
   // Since the operator's LHS assumes the operator's same ID, pop the
   // operator from the RHS stack so that if LHS short-circuits, it won't be
@@ -754,13 +789,44 @@ struct MCDCCoverageBuilder {
   popRHSifTop(E);
 } else {
   // Otherwise, assign ID+1 to LHS.
-  CondIDs[CodeGenFunction::stripCond(E->getLHS())] = NextID++;
+  setCondID(E->getLHS(), NextID++);
 }
 
 // Assign ID+1 to RHS.
-CondIDs[CodeGenFunction::stripCond(E->getRHS())] = NextID++;
+setCondID(E->getRHS(), NextID++);
+
+// Assign the True and False condition IDs for the LHS and RHS.
+if (isLAnd(E)) {
+  // For logical-AND ("LHS && RHS"):
+  // - If LHS is TRUE, execution goes to the RHS node.
+  // - If LHS is FALSE, execution goes to the LHS node of the next LOr.
+  //   If that does not exist, execution exits (ID == 0).
+  setTrueCondID(E->getLHS(), getCondID(E->getRHS()));
+  setFalseCondID(E->getLHS(), getNextLOrCondID());
+
+  // - If RHS is TRUE, execution goes to LHS node of the next LAnd.
+  //   If that does not exist, execution exits (ID == 0).
+  // - If RHS is FALSE, execution goes to the LHS node of the next LOr.
+  //   If that does not exist, execution exits (ID == 0).
+  setTrueCondI

[clang] [llvm] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)

[Alan Phipps via cfe-commits]

@@ -746,21 +781,52 @@ struct MCDCCoverageBuilder {
 // assign that ID to its LHS node.  Its RHS will receive a new ID.
 if (CondIDs.contains(CodeGenFunction::stripCond(E))) {
   // If Stmt has an ID, assign its ID to LHS
-  CondIDs[CodeGenFunction::stripCond(E->getLHS())] = CondIDs[E];
+  setCondID(E->getLHS(), getCondID(E));
 
   // Since the operator's LHS assumes the operator's same ID, pop the
   // operator from the RHS stack so that if LHS short-circuits, it won't be
   // incorrectly re-used as the node executed next.
   popRHSifTop(E);
 } else {
   // Otherwise, assign ID+1 to LHS.
-  CondIDs[CodeGenFunction::stripCond(E->getLHS())] = NextID++;
+  setCondID(E->getLHS(), NextID++);
 }
 
 // Assign ID+1 to RHS.
-CondIDs[CodeGenFunction::stripCond(E->getRHS())] = NextID++;
+setCondID(E->getRHS(), NextID++);
+
+// Assign the True and False condition IDs for the LHS and RHS.
+if (isLAnd(E)) {

evodius96 wrote:

It's much easier to assigned the True/False as we push onto the stack rather 
than popping. Popping led to off-by-one errors.

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

[clang] [llvm] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)

[Alan Phipps via cfe-commits]

@@ -1847,30 +1916,13 @@ struct CounterCoverageMappingBuilder
 // Extract the Parent Region Counter.
 Counter ParentCnt = getRegion().getCounter();
 
-// Extract the MCDC condition IDs (returns 0 if not needed).

evodius96 wrote:

Since the APIs are all the same and are encapculated within MCDCBuilder, we 
don't need all of this here.  We can just move it down into 
createBranchRegion().

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

[clang] [llvm] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)

[Alan Phipps via cfe-commits]

@@ -298,7 +298,7 @@ struct CounterMappingRegion {
unsigned ExpandedFileID, unsigned LineStart,
unsigned ColumnStart, unsigned LineEnd,
unsigned ColumnEnd, RegionKind Kind)
-  : MCDCParams(MCDCParams), ExpandedFileID(ExpandedFileID),
+  : MCDCParams(MCDCParams), FileID(FileID), ExpandedFileID(ExpandedFileID),

evodius96 wrote:

This is the fix for the first reported issue.

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

[clang] [llvm] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)

[Alan Phipps via cfe-commits]

@@ -668,6 +668,8 @@ struct MCDCCoverageBuilder {
   llvm::SmallVector NestLevel;
   llvm::DenseMap &CondIDs;
   llvm::DenseMap &MCDCBitmapMap;
+  llvm::DenseMap TrueCondIDs;

evodius96 wrote:

We can use these to track a Condition's True/False IDs and make all of this 
self-contained within MCDCBuilder rather than calculating it in the visitor 
routines. Much cleaner.

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

[clang] [llvm] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)

[Alan Phipps via cfe-commits]

@@ -298,7 +298,7 @@ struct CounterMappingRegion {
unsigned ExpandedFileID, unsigned LineStart,
unsigned ColumnStart, unsigned LineEnd,
unsigned ColumnEnd, RegionKind Kind)
-  : MCDCParams(MCDCParams), ExpandedFileID(ExpandedFileID),
+  : MCDCParams(MCDCParams), FileID(FileID), ExpandedFileID(ExpandedFileID),

evodius96 wrote:

Yeah technically it is a separate fix but very minor.  FileID was not being set 
correctly for DecisionRegions.

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

[llvm] [clang] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)

[Alan Phipps via cfe-commits]

@@ -298,7 +298,7 @@ struct CounterMappingRegion {
unsigned ExpandedFileID, unsigned LineStart,
unsigned ColumnStart, unsigned LineEnd,
unsigned ColumnEnd, RegionKind Kind)
-  : MCDCParams(MCDCParams), ExpandedFileID(ExpandedFileID),
+  : MCDCParams(MCDCParams), FileID(FileID), ExpandedFileID(ExpandedFileID),

evodius96 wrote:

You're correct.  I was thinking it might be useful to support ExpandedFileIDs 
in the future, but if that's the case, we can add it.  At the moment, I don't 
see when it would be useful anyway.

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

[clang] [llvm] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)

[Alan Phipps via cfe-commits]

evodius96 wrote:

> I've found this change fails with the expression; `((a && (b || c) || (d && 
> e)) && f)`.

Thank you for pointing this out.  I think you figured out the point I was 
struggling to get right -- when to actually "pop" the child Decision from the 
stack, and also to separate the "pop" operation from the calculation of the 
total conditions.  I was constrained by the design.  I like your cleanup, 
including the removal of the NestLevel stack, so I integrated most of your 
refactor.

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

[clang] [llvm] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)

[Alan Phipps via cfe-commits]

https://github.com/evodius96 updated 
https://github.com/llvm/llvm-project/pull/78202

>From 8751417f4889a120193a604e2ea91627f3b064e8 Mon Sep 17 00:00:00 2001
From: Alan Phipps 
Date: Mon, 15 Jan 2024 12:24:36 -0600
Subject: [PATCH 1/3] [clang][CoverageMapping] Refactor when setting MC/DC
 True/False Condition IDs

Clean-up of the algorithm that assigns MC/DC True/False control-flow
condition IDs when constructing an MC/DC decision region.  This patch creates a
common API for setting/getting the condition IDs, making the binary logical
operator visitor functions much cleaner.

This patch also fixes a bug in which the decision which was malformed due to an
incorrect calculation of the True/False condition IDs. Previously, this
calculation was done as the logical operation stack was being popped, yielding
off-by-one errors.  This is now done when the logical operation stack is being
pushed.
---
 clang/lib/CodeGen/CoverageMappingGen.cpp  | 127 +++---
 .../mcdc-logical-stmt-ids-all.cpp |  50 +++
 .../ProfileData/Coverage/CoverageMapping.h|   2 +-
 3 files changed, 133 insertions(+), 46 deletions(-)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp 
b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 916016601a93276..ca7b65e3f4b08d7 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -668,6 +668,8 @@ struct MCDCCoverageBuilder {
   llvm::SmallVector NestLevel;
   llvm::DenseMap &CondIDs;
   llvm::DenseMap &MCDCBitmapMap;
+  llvm::DenseMap TrueCondIDs;
+  llvm::DenseMap FalseCondIDs;
   MCDCConditionID NextID = 1;
   bool NotMapped = false;
 
@@ -713,6 +715,21 @@ struct MCDCCoverageBuilder {
 return AndRHS.empty() ? 0 : AndRHS.back();
   }
 
+  /// Set the given condition's ID.
+  void setCondID(const Expr *Cond, MCDCConditionID ID) {
+  CondIDs[CodeGenFunction::stripCond(Cond)] = ID;
+  }
+
+  /// Set the ID of the next condition when the given condition is True.
+  void setTrueCondID(const Expr *Cond, MCDCConditionID ID) {
+  TrueCondIDs[CodeGenFunction::stripCond(Cond)] = ID;
+  }
+
+  /// Set the ID of the next condition when the given condition is False.
+  void setFalseCondID(const Expr *Cond, MCDCConditionID ID) {
+  FalseCondIDs[CodeGenFunction::stripCond(Cond)] = ID;
+  }
+
   /// Return the ID of a given condition.
   MCDCConditionID getCondID(const Expr *Cond) const {
 auto I = CondIDs.find(CodeGenFunction::stripCond(Cond));
@@ -722,6 +739,24 @@ struct MCDCCoverageBuilder {
   return I->second;
   }
 
+  /// Return the ID of the next condition when the given condition is True.
+  MCDCConditionID getNextIfTrueCondID(const Expr *Cond) const {
+auto I = TrueCondIDs.find(CodeGenFunction::stripCond(Cond));
+if (I == TrueCondIDs.end())
+  return 0;
+else
+  return I->second;
+  }
+
+  /// Return the ID of the next condition when the given condition is False.
+  MCDCConditionID getNextIfFalseCondID(const Expr *Cond) const {
+auto I = FalseCondIDs.find(CodeGenFunction::stripCond(Cond));
+if (I == FalseCondIDs.end())
+  return 0;
+else
+  return I->second;
+  }
+
   /// 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.
@@ -746,7 +781,7 @@ struct MCDCCoverageBuilder {
 // assign that ID to its LHS node.  Its RHS will receive a new ID.
 if (CondIDs.contains(CodeGenFunction::stripCond(E))) {
   // If Stmt has an ID, assign its ID to LHS
-  CondIDs[CodeGenFunction::stripCond(E->getLHS())] = CondIDs[E];
+  setCondID(E->getLHS(), getCondID(E));
 
   // Since the operator's LHS assumes the operator's same ID, pop the
   // operator from the RHS stack so that if LHS short-circuits, it won't be
@@ -754,13 +789,44 @@ struct MCDCCoverageBuilder {
   popRHSifTop(E);
 } else {
   // Otherwise, assign ID+1 to LHS.
-  CondIDs[CodeGenFunction::stripCond(E->getLHS())] = NextID++;
+  setCondID(E->getLHS(), NextID++);
 }
 
 // Assign ID+1 to RHS.
-CondIDs[CodeGenFunction::stripCond(E->getRHS())] = NextID++;
+setCondID(E->getRHS(), NextID++);
+
+// Assign the True and False condition IDs for the LHS and RHS.
+if (isLAnd(E)) {
+  // For logical-AND ("LHS && RHS"):
+  // - If LHS is TRUE, execution goes to the RHS node.
+  // - If LHS is FALSE, execution goes to the LHS node of the next LOr.
+  //   If that does not exist, execution exits (ID == 0).
+  setTrueCondID(E->getLHS(), getCondID(E->getRHS()));
+  setFalseCondID(E->getLHS(), getNextLOrCondID());
+
+  // - If RHS is TRUE, execution goes to LHS node of the next LAnd.
+  //   If that does not exist, execution exits (ID == 0).
+  // - If RHS is FALSE, execution goes to the LHS node of the next LOr.
+  //   If that does not exist, execution exits (ID == 0).
+  setTrueCondI

[llvm] [clang] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)

[Alan Phipps via cfe-commits]

@@ -676,41 +679,25 @@ struct MCDCCoverageBuilder {
 return E->getOpcode() == BO_LAnd;
   }
 
-  /// Push an ID onto the corresponding RHS stack.
-  void pushRHS(const BinaryOperator *E) {
-llvm::SmallVector &rhs = isLAnd(E) ? AndRHS : OrRHS;
-rhs.push_back(CondIDs[CodeGenFunction::stripCond(E->getRHS())]);
-  }
-
-  /// Pop an ID from the corresponding RHS stack.
-  void popRHS(const BinaryOperator *E) {
-llvm::SmallVector &rhs = isLAnd(E) ? AndRHS : OrRHS;
-if (!rhs.empty())
-  rhs.pop_back();
-  }
-
-  /// If the expected ID is on top, pop it off the corresponding RHS stack.
-  void popRHSifTop(const BinaryOperator *E) {
-if (!OrRHS.empty() && CondIDs[E] == OrRHS.back())
-  OrRHS.pop_back();
-else if (!AndRHS.empty() && CondIDs[E] == AndRHS.back())
-  AndRHS.pop_back();
-  }
-
 public:
   MCDCCoverageBuilder(CodeGenModule &CGM,
   llvm::DenseMap &CondIDMap,
   llvm::DenseMap &MCDCBitmapMap)
-  : CGM(CGM), CondIDs(CondIDMap), MCDCBitmapMap(MCDCBitmapMap) {}
+  : CGM(CGM), DecisionStack(1), CondIDs(CondIDMap),

evodius96 wrote:

Rather than define a `DecisionStackSentinel` equal to 1, I'd prefer to define a 
`constexpr` sentinel value that is explicitly initialized to [0,0] (even though 
this is the default value).  Then it use that to explicitly initialize the 
stack.
```
static constexpr DecisionIDPair DecisionIDPairSentinel{0,0};
...
DecisionStack(1, DecisionIDPairSentinel);
```
I think this makes it clear what is being done and keeps DecisionIDPair 
relatively straightforward.

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

[llvm] [clang] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)

[Alan Phipps via cfe-commits]

https://github.com/evodius96 updated 
https://github.com/llvm/llvm-project/pull/78202

>From 8751417f4889a120193a604e2ea91627f3b064e8 Mon Sep 17 00:00:00 2001
From: Alan Phipps 
Date: Mon, 15 Jan 2024 12:24:36 -0600
Subject: [PATCH 1/4] [clang][CoverageMapping] Refactor when setting MC/DC
 True/False Condition IDs

Clean-up of the algorithm that assigns MC/DC True/False control-flow
condition IDs when constructing an MC/DC decision region.  This patch creates a
common API for setting/getting the condition IDs, making the binary logical
operator visitor functions much cleaner.

This patch also fixes a bug in which the decision which was malformed due to an
incorrect calculation of the True/False condition IDs. Previously, this
calculation was done as the logical operation stack was being popped, yielding
off-by-one errors.  This is now done when the logical operation stack is being
pushed.
---
 clang/lib/CodeGen/CoverageMappingGen.cpp  | 127 +++---
 .../mcdc-logical-stmt-ids-all.cpp |  50 +++
 .../ProfileData/Coverage/CoverageMapping.h|   2 +-
 3 files changed, 133 insertions(+), 46 deletions(-)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp 
b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 916016601a93276..ca7b65e3f4b08d7 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -668,6 +668,8 @@ struct MCDCCoverageBuilder {
   llvm::SmallVector NestLevel;
   llvm::DenseMap &CondIDs;
   llvm::DenseMap &MCDCBitmapMap;
+  llvm::DenseMap TrueCondIDs;
+  llvm::DenseMap FalseCondIDs;
   MCDCConditionID NextID = 1;
   bool NotMapped = false;
 
@@ -713,6 +715,21 @@ struct MCDCCoverageBuilder {
 return AndRHS.empty() ? 0 : AndRHS.back();
   }
 
+  /// Set the given condition's ID.
+  void setCondID(const Expr *Cond, MCDCConditionID ID) {
+  CondIDs[CodeGenFunction::stripCond(Cond)] = ID;
+  }
+
+  /// Set the ID of the next condition when the given condition is True.
+  void setTrueCondID(const Expr *Cond, MCDCConditionID ID) {
+  TrueCondIDs[CodeGenFunction::stripCond(Cond)] = ID;
+  }
+
+  /// Set the ID of the next condition when the given condition is False.
+  void setFalseCondID(const Expr *Cond, MCDCConditionID ID) {
+  FalseCondIDs[CodeGenFunction::stripCond(Cond)] = ID;
+  }
+
   /// Return the ID of a given condition.
   MCDCConditionID getCondID(const Expr *Cond) const {
 auto I = CondIDs.find(CodeGenFunction::stripCond(Cond));
@@ -722,6 +739,24 @@ struct MCDCCoverageBuilder {
   return I->second;
   }
 
+  /// Return the ID of the next condition when the given condition is True.
+  MCDCConditionID getNextIfTrueCondID(const Expr *Cond) const {
+auto I = TrueCondIDs.find(CodeGenFunction::stripCond(Cond));
+if (I == TrueCondIDs.end())
+  return 0;
+else
+  return I->second;
+  }
+
+  /// Return the ID of the next condition when the given condition is False.
+  MCDCConditionID getNextIfFalseCondID(const Expr *Cond) const {
+auto I = FalseCondIDs.find(CodeGenFunction::stripCond(Cond));
+if (I == FalseCondIDs.end())
+  return 0;
+else
+  return I->second;
+  }
+
   /// 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.
@@ -746,7 +781,7 @@ struct MCDCCoverageBuilder {
 // assign that ID to its LHS node.  Its RHS will receive a new ID.
 if (CondIDs.contains(CodeGenFunction::stripCond(E))) {
   // If Stmt has an ID, assign its ID to LHS
-  CondIDs[CodeGenFunction::stripCond(E->getLHS())] = CondIDs[E];
+  setCondID(E->getLHS(), getCondID(E));
 
   // Since the operator's LHS assumes the operator's same ID, pop the
   // operator from the RHS stack so that if LHS short-circuits, it won't be
@@ -754,13 +789,44 @@ struct MCDCCoverageBuilder {
   popRHSifTop(E);
 } else {
   // Otherwise, assign ID+1 to LHS.
-  CondIDs[CodeGenFunction::stripCond(E->getLHS())] = NextID++;
+  setCondID(E->getLHS(), NextID++);
 }
 
 // Assign ID+1 to RHS.
-CondIDs[CodeGenFunction::stripCond(E->getRHS())] = NextID++;
+setCondID(E->getRHS(), NextID++);
+
+// Assign the True and False condition IDs for the LHS and RHS.
+if (isLAnd(E)) {
+  // For logical-AND ("LHS && RHS"):
+  // - If LHS is TRUE, execution goes to the RHS node.
+  // - If LHS is FALSE, execution goes to the LHS node of the next LOr.
+  //   If that does not exist, execution exits (ID == 0).
+  setTrueCondID(E->getLHS(), getCondID(E->getRHS()));
+  setFalseCondID(E->getLHS(), getNextLOrCondID());
+
+  // - If RHS is TRUE, execution goes to LHS node of the next LAnd.
+  //   If that does not exist, execution exits (ID == 0).
+  // - If RHS is FALSE, execution goes to the LHS node of the next LOr.
+  //   If that does not exist, execution exits (ID == 0).
+  setTrueCondI

[clang] [llvm] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)

[Alan Phipps via cfe-commits]

evodius96 wrote:

> Could you mention the issue #77873 in the description or in the subject?

Yes, I will change this!

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

[clang] [llvm] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)

[Alan Phipps via cfe-commits]

@@ -1018,13 +1011,15 @@ struct CounterCoverageMappingBuilder
 return (Cond->EvaluateAsInt(Result, CVM.getCodeGenModule().getContext()));
   }
 
+  using MCDCDecisionIDPair = MCDCCoverageBuilder::DecisionIDPair;
+
   /// Create a Branch Region around an instrumentable condition for coverage
   /// and add it to the function's SourceRegions.  A branch region tracks a
   /// "True" counter and a "False" counter for boolean expressions that
   /// result in the generation of a branch.
-  void createBranchRegion(const Expr *C, Counter TrueCnt, Counter FalseCnt,
-  MCDCConditionID ID = 0, MCDCConditionID TrueID = 0,
-  MCDCConditionID FalseID = 0) {
+  void
+  createBranchRegion(const Expr *C, Counter TrueCnt, Counter FalseCnt,
+ const MCDCDecisionIDPair &IDPair = MCDCDecisionIDPair()) {

evodius96 wrote:

I propose just leaving the initializers.

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

[clang] [llvm] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)

[Alan Phipps via cfe-commits]

@@ -663,54 +668,40 @@ struct MCDCCoverageBuilder {
 private:
   CodeGenModule &CGM;
 
-  llvm::SmallVector AndRHS;
-  llvm::SmallVector OrRHS;
-  llvm::SmallVector NestLevel;
+  llvm::SmallVector DecisionStack;
   llvm::DenseMap &CondIDs;
   llvm::DenseMap &MCDCBitmapMap;
   MCDCConditionID NextID = 1;
   bool NotMapped = false;
 
+  /// Represent a sentinel value of [0,0] for the bottom of DecisionStack.
+  static constexpr DecisionIDPair DecisionIDPairSentinel{0, 0};

evodius96 wrote:

Sure, I can use that name.

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

[clang] [llvm] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)

[Alan Phipps via cfe-commits]

@@ -722,6 +713,9 @@ struct MCDCCoverageBuilder {
   return I->second;
   }
 
+  /// Return the LHS Decision ([0,0] if not set).
+  const DecisionIDPair back() const { return DecisionStack.back(); }

evodius96 wrote:

Ah, you're right.

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

[clang] [llvm] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)

[Alan Phipps via cfe-commits]

@@ -722,6 +713,9 @@ struct MCDCCoverageBuilder {
   return I->second;
   }
 
+  /// Return the LHS Decision ([0,0] if not set).
+  const DecisionIDPair back() const { return DecisionStack.back(); }

evodius96 wrote:

On second thought, could you elaborate on your concern here? I apologize; I 
know DecisionStack.back() returns a reference, but this ought to now copy that 
value, right?

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

[clang] [llvm] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)

[Alan Phipps via cfe-commits]

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

[clang] [llvm] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)

[Alan Phipps via cfe-commits]

@@ -722,6 +713,9 @@ struct MCDCCoverageBuilder {
   return I->second;
   }
 
+  /// Return the LHS Decision ([0,0] if not set).
+  const DecisionIDPair back() const { return DecisionStack.back(); }

evodius96 wrote:

Ah, ok I think I misunderstood.  You're suggesting that the fact taht 
DecisionStack.back() returns byref needs to be reflected by the return type of 
this function. In other words, restore the return type to the way it was:

`const DecisionIDPair &back() const { return DecisionStack.back(); }`

I did change the visitBin* functions to no longer hold a reference. 

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

[clang] [llvm] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)

[Alan Phipps via cfe-commits]

https://github.com/evodius96 updated 
https://github.com/llvm/llvm-project/pull/78202

>From 8751417f4889a120193a604e2ea91627f3b064e8 Mon Sep 17 00:00:00 2001
From: Alan Phipps 
Date: Mon, 15 Jan 2024 12:24:36 -0600
Subject: [PATCH 1/5] [clang][CoverageMapping] Refactor when setting MC/DC
 True/False Condition IDs

Clean-up of the algorithm that assigns MC/DC True/False control-flow
condition IDs when constructing an MC/DC decision region.  This patch creates a
common API for setting/getting the condition IDs, making the binary logical
operator visitor functions much cleaner.

This patch also fixes a bug in which the decision which was malformed due to an
incorrect calculation of the True/False condition IDs. Previously, this
calculation was done as the logical operation stack was being popped, yielding
off-by-one errors.  This is now done when the logical operation stack is being
pushed.
---
 clang/lib/CodeGen/CoverageMappingGen.cpp  | 127 +++---
 .../mcdc-logical-stmt-ids-all.cpp |  50 +++
 .../ProfileData/Coverage/CoverageMapping.h|   2 +-
 3 files changed, 133 insertions(+), 46 deletions(-)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp 
b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 916016601a93276..ca7b65e3f4b08d7 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -668,6 +668,8 @@ struct MCDCCoverageBuilder {
   llvm::SmallVector NestLevel;
   llvm::DenseMap &CondIDs;
   llvm::DenseMap &MCDCBitmapMap;
+  llvm::DenseMap TrueCondIDs;
+  llvm::DenseMap FalseCondIDs;
   MCDCConditionID NextID = 1;
   bool NotMapped = false;
 
@@ -713,6 +715,21 @@ struct MCDCCoverageBuilder {
 return AndRHS.empty() ? 0 : AndRHS.back();
   }
 
+  /// Set the given condition's ID.
+  void setCondID(const Expr *Cond, MCDCConditionID ID) {
+  CondIDs[CodeGenFunction::stripCond(Cond)] = ID;
+  }
+
+  /// Set the ID of the next condition when the given condition is True.
+  void setTrueCondID(const Expr *Cond, MCDCConditionID ID) {
+  TrueCondIDs[CodeGenFunction::stripCond(Cond)] = ID;
+  }
+
+  /// Set the ID of the next condition when the given condition is False.
+  void setFalseCondID(const Expr *Cond, MCDCConditionID ID) {
+  FalseCondIDs[CodeGenFunction::stripCond(Cond)] = ID;
+  }
+
   /// Return the ID of a given condition.
   MCDCConditionID getCondID(const Expr *Cond) const {
 auto I = CondIDs.find(CodeGenFunction::stripCond(Cond));
@@ -722,6 +739,24 @@ struct MCDCCoverageBuilder {
   return I->second;
   }
 
+  /// Return the ID of the next condition when the given condition is True.
+  MCDCConditionID getNextIfTrueCondID(const Expr *Cond) const {
+auto I = TrueCondIDs.find(CodeGenFunction::stripCond(Cond));
+if (I == TrueCondIDs.end())
+  return 0;
+else
+  return I->second;
+  }
+
+  /// Return the ID of the next condition when the given condition is False.
+  MCDCConditionID getNextIfFalseCondID(const Expr *Cond) const {
+auto I = FalseCondIDs.find(CodeGenFunction::stripCond(Cond));
+if (I == FalseCondIDs.end())
+  return 0;
+else
+  return I->second;
+  }
+
   /// 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.
@@ -746,7 +781,7 @@ struct MCDCCoverageBuilder {
 // assign that ID to its LHS node.  Its RHS will receive a new ID.
 if (CondIDs.contains(CodeGenFunction::stripCond(E))) {
   // If Stmt has an ID, assign its ID to LHS
-  CondIDs[CodeGenFunction::stripCond(E->getLHS())] = CondIDs[E];
+  setCondID(E->getLHS(), getCondID(E));
 
   // Since the operator's LHS assumes the operator's same ID, pop the
   // operator from the RHS stack so that if LHS short-circuits, it won't be
@@ -754,13 +789,44 @@ struct MCDCCoverageBuilder {
   popRHSifTop(E);
 } else {
   // Otherwise, assign ID+1 to LHS.
-  CondIDs[CodeGenFunction::stripCond(E->getLHS())] = NextID++;
+  setCondID(E->getLHS(), NextID++);
 }
 
 // Assign ID+1 to RHS.
-CondIDs[CodeGenFunction::stripCond(E->getRHS())] = NextID++;
+setCondID(E->getRHS(), NextID++);
+
+// Assign the True and False condition IDs for the LHS and RHS.
+if (isLAnd(E)) {
+  // For logical-AND ("LHS && RHS"):
+  // - If LHS is TRUE, execution goes to the RHS node.
+  // - If LHS is FALSE, execution goes to the LHS node of the next LOr.
+  //   If that does not exist, execution exits (ID == 0).
+  setTrueCondID(E->getLHS(), getCondID(E->getRHS()));
+  setFalseCondID(E->getLHS(), getNextLOrCondID());
+
+  // - If RHS is TRUE, execution goes to LHS node of the next LAnd.
+  //   If that does not exist, execution exits (ID == 0).
+  // - If RHS is FALSE, execution goes to the LHS node of the next LOr.
+  //   If that does not exist, execution exits (ID == 0).
+  setTrueCondI

[llvm] [clang] [clang][CoverageMapping] Refactor setting MC/DC True/False Condition IDs (PR #78202)

[Alan Phipps via cfe-commits]

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

[clang] [llvm] [clang][CoverageMapping] Refactor setting MC/DC True/False Condition IDs (PR #78202)

[Alan Phipps via cfe-commits]

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

[clang] Ensure bitmap for ternary condition is updated before visiting children (PR #78814)

[Alan Phipps via cfe-commits]

https://github.com/evodius96 created 
https://github.com/llvm/llvm-project/pull/78814

This is a fix for MC/DC issue https://github.com/llvm/llvm-project/issues/78453 
in which a ConditionalOperator that evaluates a complex condition was 
incorrectly updating its global bitmap after visiting its LHS and RHS children. 
 This was wrong because if the LHS or RHS also evaluate a complex condition, 
the MCDC temporary bitmap value will get corrupted.  The fix is to ensure that 
the bitmap is updated prior to visiting the LHS and RHS.

>From cbd5a8caaac0fd3d1dc9d4090d00e5bece6a41cf Mon Sep 17 00:00:00 2001
From: Alan Phipps 
Date: Fri, 19 Jan 2024 17:54:25 -0600
Subject: [PATCH] Ensure bitmap for ternary condition is updated prior to visit
 of LHS and RHS

This is a fix for MC/DC issue https://github.com/llvm/llvm-project/issues/78453 
in
which a ConditionalOperator that evaluates a complex condition was incorrectly
updating its global bitmap after visiting its LHS and RHS children.  This was
wrong because if the LHS or RHS also evaluate a complex condition, the MCDC
temporary bitmap value will get corrupted.  The fix is to ensure that the
bitmap is updated prior to visiting the LHS and RHS.
---
 clang/lib/CodeGen/CGExprScalar.cpp| 18 -
 clang/test/Profile/c-mcdc-logicalop-ternary.c | 81 +++
 2 files changed, 95 insertions(+), 4 deletions(-)
 create mode 100644 clang/test/Profile/c-mcdc-logicalop-ternary.c

diff --git a/clang/lib/CodeGen/CGExprScalar.cpp 
b/clang/lib/CodeGen/CGExprScalar.cpp
index 9ec185153d12b1..181b15e9c7d0a7 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -4960,6 +4960,13 @@ VisitAbstractConditionalOperator(const 
AbstractConditionalOperator *E) {
CGF.getProfileCount(lhsExpr));
 
   CGF.EmitBlock(LHSBlock);
+
+  // 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);
+
   CGF.incrementProfileCounter(E);
   eval.begin(CGF);
   Value *LHS = Visit(lhsExpr);
@@ -4969,6 +4976,13 @@ VisitAbstractConditionalOperator(const 
AbstractConditionalOperator *E) {
   Builder.CreateBr(ContBlock);
 
   CGF.EmitBlock(RHSBlock);
+
+  // 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);
+
   eval.begin(CGF);
   Value *RHS = Visit(rhsExpr);
   eval.end(CGF);
@@ -4987,10 +5001,6 @@ VisitAbstractConditionalOperator(const 
AbstractConditionalOperator *E) {
   PN->addIncoming(LHS, LHSBlock);
   PN->addIncoming(RHS, RHSBlock);
 
-  // If the top of the logical operator nest, update the MCDC bitmap.
-  if (CGF.MCDCLogOpStack.empty())
-CGF.maybeUpdateMCDCTestVectorBitmap(condExpr);
-
   return PN;
 }
 
diff --git a/clang/test/Profile/c-mcdc-logicalop-ternary.c 
b/clang/test/Profile/c-mcdc-logicalop-ternary.c
new file mode 100644
index 00..558643f422021c
--- /dev/null
+++ b/clang/test/Profile/c-mcdc-logicalop-ternary.c
@@ -0,0 +1,81 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple %s -o - -emit-llvm 
-fprofile-instrument=clang -fcoverage-mapping -fcoverage-mcdc | FileCheck %s 
-check-prefix=MCDC
+// RUN: %clang_cc1 -triple %itanium_abi_triple %s -o - -emit-llvm 
-fprofile-instrument=clang -fcoverage-mapping | FileCheck %s 
-check-prefix=NOMCDC
+
+int test(int a, int b, int c, int d, int e, int f) {
+  return ((a || b) ? (c && d) : (e || f));
+}
+
+// NOMCDC-NOT: %mcdc.addr
+// NOMCDC-NOT: __profbm_test
+
+// MCDC BOOKKEEPING.
+// MCDC: @__profbm_test = private global [3 x i8] zeroinitializer
+
+// 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
+
+// TERNARY TRUE SHOULD UPDATE THE BITMAP WITH RESULT AT ELEMENT 0.
+// MCDC-LABEL: cond.true:
+// MCDC-DAG:  %[[TEMP:mcdc.temp[0-9]*]] = load i32, ptr %mcdc.addr, align 4
+// MCDC:  %[[LAB1:[0-9]+]] = lshr i32 %[[TEMP]], 3
+// MCDC:  %[[LAB2:[0-9]+]] = zext i32 %[[LAB1]] to i64
+// MCDC:  %[[LAB3:[0-9]+]] = add i64 ptrtoint (ptr @__profbm_test to i64), 
%[[LAB2]]
+// MCDC:  %[[LAB4:[0-9]+]] = inttoptr i64 %[[LAB3]] to ptr
+// MCDC:  %[[LAB5:[0-9]+]] = and i32 %[[TEMP]], 7
+// MCDC:  %[[LAB6:[0-9]+]] = trunc i32 %[[LAB5]] to i8
+// MCDC:  %[[LAB7:[0-9]+]] = shl i8 1, %[[LAB6]]
+// MCDC:  %[[LAB8:mcdc.bits[0-9]*]] = load i8, ptr %[[LAB4]], align 1
+// MCDC:  %[[LAB9:[0-9]+]] = or i8 %[[LAB8]], %[[LAB7]]
+// MCDC:  store i8 %[[LAB9]], ptr %[[LAB4]], align 1
+
+// CHECK FOR ZERO OF MCDC TEMP
+// MCDC: store i32 0, ptr %mcdc.addr, align 4
+
+// TERNARY TRUE YIELDS TERNARY LHS LOGICAL-AND.
+// TERNARY LHS LOGICAL-

[clang] b920adf - This is a test commit

[Alan Phipps via cfe-commits]

Author: Alan Phipps
Date: 2020-12-23T12:57:27-06:00
New Revision: b920adf3b4f16bef8dde937b67874d8e8ac1030e

URL: 
https://github.com/llvm/llvm-project/commit/b920adf3b4f16bef8dde937b67874d8e8ac1030e
DIFF: 
https://github.com/llvm/llvm-project/commit/b920adf3b4f16bef8dde937b67874d8e8ac1030e.diff

LOG: This is a test commit

Added: 


Modified: 
clang/lib/CodeGen/README.txt

Removed: 




diff  --git a/clang/lib/CodeGen/README.txt b/clang/lib/CodeGen/README.txt
index e6d61095bf23..e4fd7816d65c 100644
--- a/clang/lib/CodeGen/README.txt
+++ b/clang/lib/CodeGen/README.txt
@@ -16,6 +16,7 @@ extension. For example, if the bitfield width is 8 and it is
 appropriately aligned then is is a lot shorter to just load the char
 directly.
 
+
 //===-===//
 
 It may be worth avoiding creation of alloca's for formal arguments



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

[clang] bbd758a - Revert "This is a test commit"

[Alan Phipps via cfe-commits]

Author: Alan Phipps
Date: 2020-12-23T13:04:37-06:00
New Revision: bbd758a7913b1c374ca26e5a734a01200754fe0e

URL: 
https://github.com/llvm/llvm-project/commit/bbd758a7913b1c374ca26e5a734a01200754fe0e
DIFF: 
https://github.com/llvm/llvm-project/commit/bbd758a7913b1c374ca26e5a734a01200754fe0e.diff

LOG: Revert "This is a test commit"

This reverts commit b920adf3b4f16bef8dde937b67874d8e8ac1030e.

Added: 


Modified: 
clang/lib/CodeGen/README.txt

Removed: 




diff  --git a/clang/lib/CodeGen/README.txt b/clang/lib/CodeGen/README.txt
index e4fd7816d65c..e6d61095bf23 100644
--- a/clang/lib/CodeGen/README.txt
+++ b/clang/lib/CodeGen/README.txt
@@ -16,7 +16,6 @@ extension. For example, if the bitfield width is 8 and it is
 appropriately aligned then is is a lot shorter to just load the char
 directly.
 
-
 //===-===//
 
 It may be worth avoiding creation of alloca's for formal arguments



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

[clang] [IRPGO][ValueProfile] Instrument virtual table address that could be used to do virtual table address comparision for indirect-call-promotion. (PR #66825)

[Alan Phipps via cfe-commits]

evodius96 wrote:

Just a warning that I am planning to push a significant change to enable MC/DC 
within the next few hours that will cause some conflicts with what you've done 
here. I apologize for the inconvenience!

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

[clang] [compiler-rt] [llvm] [InstrProf] Single byte counters in coverage (PR #75425)

[Alan Phipps via cfe-commits]

@@ -1519,38 +1543,53 @@ struct CounterCoverageMappingBuilder
 }
 
 // Create Branch Region around condition.
-createBranchRegion(S->getCond(), BodyCount,
-   subtractCounters(CondCount, BodyCount));
+if (!llvm::EnableSingleByteCoverage)
+  createBranchRegion(S->getCond(), BodyCount,
+ subtractCounters(CondCount, BodyCount));

evodius96 wrote:

I may have missed it, but do you plan to handle branch regions at some point 
with explicit byte counters?

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

[clang] [compiler-rt] [llvm] [InstrProf] Single byte counters in coverage (PR #75425)

[Alan Phipps via cfe-commits]

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

LGTM. I would like to see this broadened to include branch coverage as well!

https://github.com/llvm/llvm-project/pull/75425
___
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)

[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)

[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)

[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)

[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)

[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] [llvm] [MC/DC][Coverage] Loosen the limit of NumConds from 6 (PR #82448)

[Alan Phipps via cfe-commits]

@@ -484,10 +484,31 @@ MC/DC Instrumentation
 -
 
 When instrumenting for Modified Condition/Decision Coverage (MC/DC) using the
-clang option ``-fcoverage-mcdc``, users are limited to at most **six** 
leaf-level
-conditions in a boolean expression.  A warning will be generated for boolean
-expressions that contain more than six, and they will not be instrumented for
-MC/DC.
+clang option ``-fcoverage-mcdc``, there are two hard limits.
+
+The maximum number of terms is limited to 32767. It would be practical for
+handwritten expressions. To be more retrictive in order to enfoce conding 
rules,

evodius96 wrote:

Nit: "retrictive" --> "restrictive" and "enfoce" --> "enforce"

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

[clang] [llvm] [MC/DC][Coverage] Loosen the limit of NumConds from 6 (PR #82448)

[Alan Phipps via cfe-commits]

evodius96 wrote:

> Could I update release notes later? I am afraid since I have been always 
> missing release schedules.

I think I'm OK with this; I think the changes make sense. I'd like to see how 
others feel.


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

[clang] [llvm] [MC/DC][Coverage] Loosen the limit of NumConds from 6 (PR #82448)

[Alan Phipps via cfe-commits]

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

LGTM but I'd like to see if @ornata or others can definitively approve.

Thanks for this work! Comprehensively I think LLVM MC/DC is really good, and 
there's still more we can do.

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

[clang] [clang][ARM] Fix warning for using VFP from interrupts. (PR #91870)

[Alan Phipps via cfe-commits]

evodius96 wrote:

Somewhat related to this -- we (TI) are presently in the process of upstreaming 
our mod to save/restore VFP registers with the interrupt_save_fp attribute. See 
#89654.  I think the patch needs to be updated in light of upstream changes to 
the Arm frame lowering code.

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

[clang] [llvm] Cleanup MC/DC intrinsics for #82448 (PR #95496)

[Alan Phipps via cfe-commits]

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

LGTM thanks

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

[clang] [llvm] Update ReleaseNotes for MC/DC changes. (PR #95887)

[Alan Phipps via cfe-commits]

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


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

[clang] [llvm] [Coverage][MC/DC] Show uncoverable and unreachable conditions (PR #94137)

[Alan Phipps via cfe-commits]

evodius96 wrote:

I think this is a useful option, thanks. So the default preserves existing 
behavior? I think that's perfect and amenable to change in the future of needed.

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

[clang] [MC/DC][Coverage] Add assertions into emitSourceRegions() (PR #89572)

[Alan Phipps via cfe-commits]

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


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

[clang] 16f3401 - [Coverage] Fix test failures from commit rG9f2967bcfe2f

[Alan Phipps via cfe-commits]

Author: Alan Phipps
Date: 2021-01-05T13:35:52-06:00
New Revision: 16f3401eae4310c95163269c41d9b45261f0c7c3

URL: 
https://github.com/llvm/llvm-project/commit/16f3401eae4310c95163269c41d9b45261f0c7c3
DIFF: 
https://github.com/llvm/llvm-project/commit/16f3401eae4310c95163269c41d9b45261f0c7c3.diff

LOG: [Coverage] Fix test failures from commit rG9f2967bcfe2f

Fix test failures with Branch Coverage tests from commit rG9f2967bcfe2f
that failed build on builder clang-x64-windows-msvc while building llvm:
http://lab.llvm.org:8011/#builders/123/builds/2155

Added: 


Modified: 
clang/test/CoverageMapping/branch-constfolded.cpp
clang/test/CoverageMapping/branch-macros.cpp
clang/test/CoverageMapping/branch-mincounters.cpp
clang/test/CoverageMapping/branch-templates.cpp
clang/test/Profile/branch-logical-mixed.cpp
clang/test/Profile/branch-profdup.cpp

Removed: 




diff  --git a/clang/test/CoverageMapping/branch-constfolded.cpp 
b/clang/test/CoverageMapping/branch-constfolded.cpp
index bb7c675e3ef7..5173286addbb 100644
--- a/clang/test/CoverageMapping/branch-constfolded.cpp
+++ b/clang/test/CoverageMapping/branch-constfolded.cpp
@@ -1,6 +1,6 @@
 // Test that branch regions are not generated for constant-folded conditions.
 
-// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping 
-dump-coverage-mapping -emit-llvm-only -main-file-name branch-constfolded.cpp 
%s | FileCheck %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++11 
-fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping 
-emit-llvm-only -main-file-name branch-constfolded.cpp %s | FileCheck %s
 
 // CHECK-LABEL: _Z6fand_0b:
 bool fand_0(bool a) {

diff  --git a/clang/test/CoverageMapping/branch-macros.cpp 
b/clang/test/CoverageMapping/branch-macros.cpp
index d5bc47fbef76..0f9ae791a355 100644
--- a/clang/test/CoverageMapping/branch-macros.cpp
+++ b/clang/test/CoverageMapping/branch-macros.cpp
@@ -1,7 +1,7 @@
 // Test that branch regions are generated for conditions in nested macro
 // expansions.
 
-// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping 
-dump-coverage-mapping -emit-llvm-only -main-file-name branch-macros.cpp %s | 
FileCheck %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++11 
-fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping 
-emit-llvm-only -main-file-name branch-macros.cpp %s | FileCheck %s
 
 #define COND1 (a == b)
 #define COND2 (a != b)

diff  --git a/clang/test/CoverageMapping/branch-mincounters.cpp 
b/clang/test/CoverageMapping/branch-mincounters.cpp
index 68e691684970..6d4341cbeafd 100644
--- a/clang/test/CoverageMapping/branch-mincounters.cpp
+++ b/clang/test/CoverageMapping/branch-mincounters.cpp
@@ -1,7 +1,7 @@
 // Test to ensure right number of counters are allocated and used for nested
 // logical operators on branch conditions for branch coverage.
 
-// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping 
-dump-coverage-mapping -emit-llvm-only -main-file-name branch-logical-mixed.cpp 
%s | FileCheck %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++11 
-fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping 
-emit-llvm-only -main-file-name branch-logical-mixed.cpp %s | FileCheck %s
 
 
 // CHECK-LABEL: _Z5func1ii:

diff  --git a/clang/test/CoverageMapping/branch-templates.cpp 
b/clang/test/CoverageMapping/branch-templates.cpp
index 9e312df9b2de..1fd01218c903 100644
--- a/clang/test/CoverageMapping/branch-templates.cpp
+++ b/clang/test/CoverageMapping/branch-templates.cpp
@@ -1,7 +1,7 @@
 // Test that branch regions are generated for conditions in function template
 // instantiations.
 
-// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping 
-dump-coverage-mapping -emit-llvm-only -main-file-name branch-templates.cpp %s 
| FileCheck %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++11 
-fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping 
-emit-llvm-only -main-file-name branch-templates.cpp %s | FileCheck %s
 
 template
 void unused(T x) {

diff  --git a/clang/test/Profile/branch-logical-mixed.cpp 
b/clang/test/Profile/branch-logical-mixed.cpp
index e6f546d3ac4b..cbfcf061f152 100644
--- a/clang/test/Profile/branch-logical-mixed.cpp
+++ b/clang/test/Profile/branch-logical-mixed.cpp
@@ -1,7 +1,7 @@
 // Test to ensure instrumentation of logical operator RHS True/False counters
 // are being instrumented for branch coverage
 
-// RUN: %clang_cc1 -main-file-name branch-logical-mixed.cpp %s -o - -emit-llvm 
-fprofile-instrument=clang | FileCheck -allow-deprecated-dag-overlap %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++11 -main-file-name 
branch-logical-mixed.cpp %s -o - -emit-llvm -fprofile-instrument=clang | 
FileCheck -allow-deprecated-dag-overlap %s
 
 
 // CHECK: @[[FUNC:__profc__Z4funcv]] = private global [61 x i64] 
zeroinitializer

diff  --git a/clang/test/Profil

[clang] 2168942 - [Coverage] Fix Profile test failures from commit rG9f2967bcfe2f

[Alan Phipps via cfe-commits]

Author: Alan Phipps
Date: 2021-01-05T14:53:07-06:00
New Revision: 216894211713bbb1e8beb249f2b008c11a9d8c2c

URL: 
https://github.com/llvm/llvm-project/commit/216894211713bbb1e8beb249f2b008c11a9d8c2c
DIFF: 
https://github.com/llvm/llvm-project/commit/216894211713bbb1e8beb249f2b008c11a9d8c2c.diff

LOG: [Coverage] Fix Profile test failures from commit rG9f2967bcfe2f

Fix test failures with Branch Coverage tests from commit rG9f2967bcfe2f
that failed build on builder clang-x64-windows-msvc while building llvm:
http://lab.llvm.org:8011/#/builders/123/builds/2162

Added: 


Modified: 
clang/test/Profile/branch-logical-mixed.cpp

Removed: 




diff  --git a/clang/test/Profile/branch-logical-mixed.cpp 
b/clang/test/Profile/branch-logical-mixed.cpp
index cbfcf061f152..04b51d81d13b 100644
--- a/clang/test/Profile/branch-logical-mixed.cpp
+++ b/clang/test/Profile/branch-logical-mixed.cpp
@@ -4,7 +4,7 @@
 // RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++11 -main-file-name 
branch-logical-mixed.cpp %s -o - -emit-llvm -fprofile-instrument=clang | 
FileCheck -allow-deprecated-dag-overlap %s
 
 
-// CHECK: @[[FUNC:__profc__Z4funcv]] = private global [61 x i64] 
zeroinitializer
+// CHECK: @[[FUNC:__profc__Z4funcv]] = {{.*}} global [61 x i64] zeroinitializer
 
 
 // CHECK-LABEL: @_Z4funcv()



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

[clang] [llvm] [Coverage][MC/DC] Show uncoverable and unreachable conditions (PR #94137)

[Alan Phipps via cfe-commits]

@@ -1128,15 +1132,22 @@ struct CounterCoverageMappingBuilder
 BranchParams = mcdc::BranchParameters{ID, Conds};
 
   // If a condition can fold to true or false, the corresponding branch
-  // will be removed.  Create a region with both counters hard-coded to
-  // zero. This allows us to visualize them in a special way.
+  // will be removed. Create a region with the relative counter hard-coded
+  // to zero. This allows us to visualize them in a special way.
   // Alternatively, we can prevent any optimization done via
   // constant-folding by ensuring that ConstantFoldsToSimpleInteger() in
   // CodeGenFunction.c always returns false, but that is very heavy-handed.
-  if (ConditionFoldsToBool(C))
-popRegions(pushRegion(Counter::getZero(), getStart(C), getEnd(C),
-  Counter::getZero(), BranchParams));
-  else
+  bool ConstantBool = false;
+  if (ConditionFoldsToBool(C, ConstantBool)) {
+if (ConstantBool) {
+  popRegions(pushRegion(TrueCnt, getStart(C), getEnd(C),
+Counter::getZero(), BranchParams));
+} else {
+  popRegions(pushRegion(Counter::getZero(), getStart(C), getEnd(C),

evodius96 wrote:

Since I added `FalseCount` (i.e. the 2nd Counter for CounterMappingRegion), I 
think overriding what hard "0" means for this purpose is fine. However, for 
`Count` (i.e. the 1st Counter for CounterMappingRegion), we ought to be sure 
that the hard "0" doesn't already mean something.  I suspect that when used 
with the `BranchRegion` or `MCDCBranchRegion`, this is also fine.

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

[clang] [llvm] [Coverage][MC/DC] Show uncoverable and unreachable conditions (PR #94137)

[Alan Phipps via cfe-commits]

@@ -47,7 +47,7 @@
 // CHECK: Branch (103:9): [True: 9, False: 1]
 
 // CHECK: switches()
-// CHECK: Branch (113:3): [True: 1, False: 0]
+// CHECK: Branch (113:3): [Folded - Ignored]

evodius96 wrote:

I'm not sure I see why the evaluation of this case (and line 60) would now be 
considered as folded when prior to this change, it wasn't.  Strictly speaking, 
`weights` isn't constant, though based on its values I can see why these 
conditions always evaluate to true.



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

[clang] [llvm] [Coverage][MC/DC] Show uncoverable and unreachable conditions (PR #94137)

[Alan Phipps via cfe-commits]

evodius96 wrote:

> I have no authority to request reviewers or commit this patch. So let's @ 
> someone, @chapuni @ornata @hanickadot

I had some comments in the pending state that I just now noticed :( sorry about 
that.

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

[clang] [llvm] [Coverage] Introduce "partial fold" on BranchRegion (PR #112694)

[Alan Phipps via cfe-commits]

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


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

[clang] [llvm] [Coverage] Introduce "partial fold" on BranchRegion (PR #112694)

[Alan Phipps via cfe-commits]

evodius96 wrote:

I think this is a meaningful enhancement to branch coverage. I don't have much 
to add to what's been said.  LGTM. Thanks!

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

[clang] [Coverage] Resurrect Branch:FalseCnt in SwitchStmt that was pruned in #112694 (PR #120418)

[Alan Phipps via cfe-commits]

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


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

[clang] [compiler-rt] [llvm] llvm-cov: Merge records for template instantiations (PR #121197)

[Alan Phipps via cfe-commits]

evodius96 wrote:

I agree; I apologize for my silence as I have had several distractions. I plan 
to start looking at more of these next week.

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