[clang] [llvm] [MC/DC] Refactor: Make `MCDCParams` as `std::variant` (PR #81227)

2024-02-15 Thread via cfe-commits 

LW-archlinux wrote:

Commit 75f0d40507ea3f7c99dd3250ff0fbe6dab341910 breaks build for a multilib 
32-bit clang build.

See https://github.com/llvm/llvm-project/issues/81880

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

[clang] [llvm] [MC/DC] Refactor: Make `MCDCParams` as `std::variant` (PR #81227)

2024-02-13 Thread NAKAMURA Takumi via cfe-commits 

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

[clang] [llvm] [MC/DC] Refactor: Make `MCDCParams` as `std::variant` (PR #81227)

2024-02-13 Thread NAKAMURA Takumi via cfe-commits 

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

[clang] [llvm] [MC/DC] Refactor: Make `MCDCParams` as `std::variant` (PR #81227)

2024-02-13 Thread NAKAMURA Takumi via cfe-commits 

https://github.com/chapuni updated 
https://github.com/llvm/llvm-project/pull/81227

>From c2b49a5317bf5b8af419cba814f95cc9305bec21 Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi 
Date: Thu, 8 Feb 2024 23:12:54 +0900
Subject: [PATCH 1/3] [MC/DC] Refactor: Make `MCDCParams` as `std::variant`

Introduce `MCDCDecisionParameters` and `MCDCBranchParameters` and
make sure them not initialized as zero.

FIXME: Could we make `CoverageMappingRegion` as a smart tagged union?
---
 clang/lib/CodeGen/CoverageMappingGen.cpp  | 69 +
 .../ProfileData/Coverage/CoverageMapping.h| 77 ---
 .../ProfileData/Coverage/CoverageMapping.cpp  | 53 +++--
 .../Coverage/CoverageMappingReader.cpp| 17 ++--
 .../Coverage/CoverageMappingWriter.cpp| 23 --
 .../ProfileData/CoverageMappingTest.cpp   |  8 +-
 6 files changed, 149 insertions(+), 98 deletions(-)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp 
b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 0c43317642bca4..da5d43cda91209 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -97,6 +97,8 @@ void CoverageSourceInfo::updateNextTokLoc(SourceLocation Loc) 
{
 namespace {
 using MCDCConditionID = CounterMappingRegion::MCDCConditionID;
 using MCDCParameters = CounterMappingRegion::MCDCParameters;
+using MCDCBranchParameters = CounterMappingRegion::MCDCBranchParameters;
+using MCDCDecisionParameters = CounterMappingRegion::MCDCDecisionParameters;
 
 /// A region of source code that can be mapped to a counter.
 class SourceMappingRegion {
@@ -185,7 +187,17 @@ class SourceMappingRegion {
 
   bool isBranch() const { return FalseCount.has_value(); }
 
-  bool isMCDCDecision() const { return MCDCParams.NumConditions != 0; }
+  bool isMCDCDecision() const {
+const auto *DecisionParams =
+std::get_if(&MCDCParams);
+assert(!DecisionParams || DecisionParams->NumConditions > 0);
+return DecisionParams;
+  }
+
+  const auto &getMCDCDecisionParams() const {
+return CounterMappingRegion::getParams(
+MCDCParams);
+  }
 
   const MCDCParameters &getMCDCParams() const { return MCDCParams; }
 };
@@ -483,13 +495,13 @@ class CoverageMappingBuilder {
 SR.ColumnEnd));
   } else if (Region.isBranch()) {
 MappingRegions.push_back(CounterMappingRegion::makeBranchRegion(
-Region.getCounter(), Region.getFalseCounter(),
-Region.getMCDCParams(), *CovFileID, SR.LineStart, SR.ColumnStart,
-SR.LineEnd, SR.ColumnEnd));
+Region.getCounter(), Region.getFalseCounter(), *CovFileID,
+SR.LineStart, SR.ColumnStart, SR.LineEnd, SR.ColumnEnd,
+Region.getMCDCParams()));
   } else if (Region.isMCDCDecision()) {
 MappingRegions.push_back(CounterMappingRegion::makeDecisionRegion(
-Region.getMCDCParams(), *CovFileID, SR.LineStart, SR.ColumnStart,
-SR.LineEnd, SR.ColumnEnd));
+Region.getMCDCDecisionParams(), *CovFileID, SR.LineStart,
+SR.ColumnStart, SR.LineEnd, SR.ColumnEnd));
   } else {
 MappingRegions.push_back(CounterMappingRegion::makeRegion(
 Region.getCounter(), *CovFileID, SR.LineStart, SR.ColumnStart,
@@ -865,8 +877,7 @@ struct CounterCoverageMappingBuilder
 std::optional StartLoc = std::nullopt,
 std::optional EndLoc = std::nullopt,
 std::optional FalseCount = std::nullopt,
-MCDCConditionID ID = 0, MCDCConditionID TrueID = 0,
-MCDCConditionID FalseID = 0) {
+const MCDCParameters &BranchParams = std::monostate()) {
 
 if (StartLoc && !FalseCount) {
   MostRecentLocation = *StartLoc;
@@ -885,9 +896,7 @@ struct CounterCoverageMappingBuilder
   StartLoc = std::nullopt;
 if (EndLoc && EndLoc->isInvalid())
   EndLoc = std::nullopt;
-RegionStack.emplace_back(Count, FalseCount,
- MCDCParameters{0, 0, ID, TrueID, FalseID},
- StartLoc, EndLoc);
+RegionStack.emplace_back(Count, FalseCount, BranchParams, StartLoc, 
EndLoc);
 
 return RegionStack.size() - 1;
   }
@@ -896,8 +905,8 @@ struct CounterCoverageMappingBuilder
 std::optional StartLoc = std::nullopt,
 std::optional EndLoc = std::nullopt) {
 
-RegionStack.emplace_back(MCDCParameters{BitmapIdx, Conditions}, StartLoc,
- EndLoc);
+RegionStack.emplace_back(MCDCDecisionParameters{BitmapIdx, Conditions},
+ StartLoc, EndLoc);
 
 return RegionStack.size() - 1;
   }
@@ -1042,9 +1051,10 @@ struct CounterCoverageMappingBuilder
 // function's SourceRegions) because it doesn't apply to any other source
 // code other than the Condition.
 if (CodeGenFunction::isInstrumentedCondition(C)) {
+  MCDCParameters BranchParams;
   MCD

[clang] [llvm] [MC/DC] Refactor: Make `MCDCParams` as `std::variant` (PR #81227)

2024-02-13 Thread Jessica Paquette via cfe-commits 

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

LGTM

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

[clang] [llvm] [MC/DC] Refactor: Make `MCDCParams` as `std::variant` (PR #81227)

2024-02-12 Thread NAKAMURA Takumi via cfe-commits 

https://github.com/chapuni updated 
https://github.com/llvm/llvm-project/pull/81227

>From c2b49a5317bf5b8af419cba814f95cc9305bec21 Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi 
Date: Thu, 8 Feb 2024 23:12:54 +0900
Subject: [PATCH 1/3] [MC/DC] Refactor: Make `MCDCParams` as `std::variant`

Introduce `MCDCDecisionParameters` and `MCDCBranchParameters` and
make sure them not initialized as zero.

FIXME: Could we make `CoverageMappingRegion` as a smart tagged union?
---
 clang/lib/CodeGen/CoverageMappingGen.cpp  | 69 +
 .../ProfileData/Coverage/CoverageMapping.h| 77 ---
 .../ProfileData/Coverage/CoverageMapping.cpp  | 53 +++--
 .../Coverage/CoverageMappingReader.cpp| 17 ++--
 .../Coverage/CoverageMappingWriter.cpp| 23 --
 .../ProfileData/CoverageMappingTest.cpp   |  8 +-
 6 files changed, 149 insertions(+), 98 deletions(-)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp 
b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 0c43317642bca4..da5d43cda91209 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -97,6 +97,8 @@ void CoverageSourceInfo::updateNextTokLoc(SourceLocation Loc) 
{
 namespace {
 using MCDCConditionID = CounterMappingRegion::MCDCConditionID;
 using MCDCParameters = CounterMappingRegion::MCDCParameters;
+using MCDCBranchParameters = CounterMappingRegion::MCDCBranchParameters;
+using MCDCDecisionParameters = CounterMappingRegion::MCDCDecisionParameters;
 
 /// A region of source code that can be mapped to a counter.
 class SourceMappingRegion {
@@ -185,7 +187,17 @@ class SourceMappingRegion {
 
   bool isBranch() const { return FalseCount.has_value(); }
 
-  bool isMCDCDecision() const { return MCDCParams.NumConditions != 0; }
+  bool isMCDCDecision() const {
+const auto *DecisionParams =
+std::get_if(&MCDCParams);
+assert(!DecisionParams || DecisionParams->NumConditions > 0);
+return DecisionParams;
+  }
+
+  const auto &getMCDCDecisionParams() const {
+return CounterMappingRegion::getParams(
+MCDCParams);
+  }
 
   const MCDCParameters &getMCDCParams() const { return MCDCParams; }
 };
@@ -483,13 +495,13 @@ class CoverageMappingBuilder {
 SR.ColumnEnd));
   } else if (Region.isBranch()) {
 MappingRegions.push_back(CounterMappingRegion::makeBranchRegion(
-Region.getCounter(), Region.getFalseCounter(),
-Region.getMCDCParams(), *CovFileID, SR.LineStart, SR.ColumnStart,
-SR.LineEnd, SR.ColumnEnd));
+Region.getCounter(), Region.getFalseCounter(), *CovFileID,
+SR.LineStart, SR.ColumnStart, SR.LineEnd, SR.ColumnEnd,
+Region.getMCDCParams()));
   } else if (Region.isMCDCDecision()) {
 MappingRegions.push_back(CounterMappingRegion::makeDecisionRegion(
-Region.getMCDCParams(), *CovFileID, SR.LineStart, SR.ColumnStart,
-SR.LineEnd, SR.ColumnEnd));
+Region.getMCDCDecisionParams(), *CovFileID, SR.LineStart,
+SR.ColumnStart, SR.LineEnd, SR.ColumnEnd));
   } else {
 MappingRegions.push_back(CounterMappingRegion::makeRegion(
 Region.getCounter(), *CovFileID, SR.LineStart, SR.ColumnStart,
@@ -865,8 +877,7 @@ struct CounterCoverageMappingBuilder
 std::optional StartLoc = std::nullopt,
 std::optional EndLoc = std::nullopt,
 std::optional FalseCount = std::nullopt,
-MCDCConditionID ID = 0, MCDCConditionID TrueID = 0,
-MCDCConditionID FalseID = 0) {
+const MCDCParameters &BranchParams = std::monostate()) {
 
 if (StartLoc && !FalseCount) {
   MostRecentLocation = *StartLoc;
@@ -885,9 +896,7 @@ struct CounterCoverageMappingBuilder
   StartLoc = std::nullopt;
 if (EndLoc && EndLoc->isInvalid())
   EndLoc = std::nullopt;
-RegionStack.emplace_back(Count, FalseCount,
- MCDCParameters{0, 0, ID, TrueID, FalseID},
- StartLoc, EndLoc);
+RegionStack.emplace_back(Count, FalseCount, BranchParams, StartLoc, 
EndLoc);
 
 return RegionStack.size() - 1;
   }
@@ -896,8 +905,8 @@ struct CounterCoverageMappingBuilder
 std::optional StartLoc = std::nullopt,
 std::optional EndLoc = std::nullopt) {
 
-RegionStack.emplace_back(MCDCParameters{BitmapIdx, Conditions}, StartLoc,
- EndLoc);
+RegionStack.emplace_back(MCDCDecisionParameters{BitmapIdx, Conditions},
+ StartLoc, EndLoc);
 
 return RegionStack.size() - 1;
   }
@@ -1042,9 +1051,10 @@ struct CounterCoverageMappingBuilder
 // function's SourceRegions) because it doesn't apply to any other source
 // code other than the Condition.
 if (CodeGenFunction::isInstrumentedCondition(C)) {
+  MCDCParameters BranchParams;
   MCD

[clang] [llvm] [MC/DC] Refactor: Make `MCDCParams` as `std::variant` (PR #81227)

2024-02-11 Thread NAKAMURA Takumi via cfe-commits 


@@ -308,13 +309,21 @@ Error 
RawCoverageMappingReader::readMappingRegionsSubArray(
 return Err;
   if (auto Err = readIntMax(FID, std::numeric_limits::max()))
 return Err;
+  if (ID == 0)
+return make_error(
+coveragemap_error::malformed,
+"MCDCConditionID shouldn't be zero");
+  Params = CounterMappingRegion::MCDCBranchParameters{
+  (unsigned)ID, (unsigned)TID, (unsigned)FID};

chapuni wrote:

I was so lazy since their casts would be removed by other PRs. I'll do shortly.

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

[clang] [llvm] [MC/DC] Refactor: Make `MCDCParams` as `std::variant` (PR #81227)

2024-02-11 Thread NAKAMURA Takumi via cfe-commits 

https://github.com/chapuni commented:

@ornata I want to isolate Decision stuff and Branch stuff in `MCDCParams`.
Also I'd like to encapsulate params into each record. Once parameters are set, 
they are expected to hold "valid" values. Zeroing really confused me, (possibly 
us).

I've found `std::variant` works good for this area. I expect it will mitigate 
confusions little a bit.

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

[clang] [llvm] [MC/DC] Refactor: Make `MCDCParams` as `std::variant` (PR #81227)

2024-02-11 Thread NAKAMURA Takumi via cfe-commits 

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

[clang] [llvm] [MC/DC] Refactor: Make `MCDCParams` as `std::variant` (PR #81227)

2024-02-11 Thread Jessica Paquette via cfe-commits 

ornata wrote:

Is the main benefit of this avoiding zero initialization?

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

[clang] [llvm] [MC/DC] Refactor: Make `MCDCParams` as `std::variant` (PR #81227)

2024-02-11 Thread Jessica Paquette via cfe-commits 


@@ -308,13 +309,21 @@ Error 
RawCoverageMappingReader::readMappingRegionsSubArray(
 return Err;
   if (auto Err = readIntMax(FID, std::numeric_limits::max()))
 return Err;
+  if (ID == 0)
+return make_error(
+coveragemap_error::malformed,
+"MCDCConditionID shouldn't be zero");
+  Params = CounterMappingRegion::MCDCBranchParameters{
+  (unsigned)ID, (unsigned)TID, (unsigned)FID};

ornata wrote:

`static_cast`?

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

[clang] [llvm] [MC/DC] Refactor: Make `MCDCParams` as `std::variant` (PR #81227)

2024-02-10 Thread NAKAMURA Takumi via cfe-commits 

https://github.com/chapuni updated 
https://github.com/llvm/llvm-project/pull/81227

>From c2b49a5317bf5b8af419cba814f95cc9305bec21 Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi 
Date: Thu, 8 Feb 2024 23:12:54 +0900
Subject: [PATCH 1/2] [MC/DC] Refactor: Make `MCDCParams` as `std::variant`

Introduce `MCDCDecisionParameters` and `MCDCBranchParameters` and
make sure them not initialized as zero.

FIXME: Could we make `CoverageMappingRegion` as a smart tagged union?
---
 clang/lib/CodeGen/CoverageMappingGen.cpp  | 69 +
 .../ProfileData/Coverage/CoverageMapping.h| 77 ---
 .../ProfileData/Coverage/CoverageMapping.cpp  | 53 +++--
 .../Coverage/CoverageMappingReader.cpp| 17 ++--
 .../Coverage/CoverageMappingWriter.cpp| 23 --
 .../ProfileData/CoverageMappingTest.cpp   |  8 +-
 6 files changed, 149 insertions(+), 98 deletions(-)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp 
b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 0c43317642bca4..da5d43cda91209 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -97,6 +97,8 @@ void CoverageSourceInfo::updateNextTokLoc(SourceLocation Loc) 
{
 namespace {
 using MCDCConditionID = CounterMappingRegion::MCDCConditionID;
 using MCDCParameters = CounterMappingRegion::MCDCParameters;
+using MCDCBranchParameters = CounterMappingRegion::MCDCBranchParameters;
+using MCDCDecisionParameters = CounterMappingRegion::MCDCDecisionParameters;
 
 /// A region of source code that can be mapped to a counter.
 class SourceMappingRegion {
@@ -185,7 +187,17 @@ class SourceMappingRegion {
 
   bool isBranch() const { return FalseCount.has_value(); }
 
-  bool isMCDCDecision() const { return MCDCParams.NumConditions != 0; }
+  bool isMCDCDecision() const {
+const auto *DecisionParams =
+std::get_if(&MCDCParams);
+assert(!DecisionParams || DecisionParams->NumConditions > 0);
+return DecisionParams;
+  }
+
+  const auto &getMCDCDecisionParams() const {
+return CounterMappingRegion::getParams(
+MCDCParams);
+  }
 
   const MCDCParameters &getMCDCParams() const { return MCDCParams; }
 };
@@ -483,13 +495,13 @@ class CoverageMappingBuilder {
 SR.ColumnEnd));
   } else if (Region.isBranch()) {
 MappingRegions.push_back(CounterMappingRegion::makeBranchRegion(
-Region.getCounter(), Region.getFalseCounter(),
-Region.getMCDCParams(), *CovFileID, SR.LineStart, SR.ColumnStart,
-SR.LineEnd, SR.ColumnEnd));
+Region.getCounter(), Region.getFalseCounter(), *CovFileID,
+SR.LineStart, SR.ColumnStart, SR.LineEnd, SR.ColumnEnd,
+Region.getMCDCParams()));
   } else if (Region.isMCDCDecision()) {
 MappingRegions.push_back(CounterMappingRegion::makeDecisionRegion(
-Region.getMCDCParams(), *CovFileID, SR.LineStart, SR.ColumnStart,
-SR.LineEnd, SR.ColumnEnd));
+Region.getMCDCDecisionParams(), *CovFileID, SR.LineStart,
+SR.ColumnStart, SR.LineEnd, SR.ColumnEnd));
   } else {
 MappingRegions.push_back(CounterMappingRegion::makeRegion(
 Region.getCounter(), *CovFileID, SR.LineStart, SR.ColumnStart,
@@ -865,8 +877,7 @@ struct CounterCoverageMappingBuilder
 std::optional StartLoc = std::nullopt,
 std::optional EndLoc = std::nullopt,
 std::optional FalseCount = std::nullopt,
-MCDCConditionID ID = 0, MCDCConditionID TrueID = 0,
-MCDCConditionID FalseID = 0) {
+const MCDCParameters &BranchParams = std::monostate()) {
 
 if (StartLoc && !FalseCount) {
   MostRecentLocation = *StartLoc;
@@ -885,9 +896,7 @@ struct CounterCoverageMappingBuilder
   StartLoc = std::nullopt;
 if (EndLoc && EndLoc->isInvalid())
   EndLoc = std::nullopt;
-RegionStack.emplace_back(Count, FalseCount,
- MCDCParameters{0, 0, ID, TrueID, FalseID},
- StartLoc, EndLoc);
+RegionStack.emplace_back(Count, FalseCount, BranchParams, StartLoc, 
EndLoc);
 
 return RegionStack.size() - 1;
   }
@@ -896,8 +905,8 @@ struct CounterCoverageMappingBuilder
 std::optional StartLoc = std::nullopt,
 std::optional EndLoc = std::nullopt) {
 
-RegionStack.emplace_back(MCDCParameters{BitmapIdx, Conditions}, StartLoc,
- EndLoc);
+RegionStack.emplace_back(MCDCDecisionParameters{BitmapIdx, Conditions},
+ StartLoc, EndLoc);
 
 return RegionStack.size() - 1;
   }
@@ -1042,9 +1051,10 @@ struct CounterCoverageMappingBuilder
 // function's SourceRegions) because it doesn't apply to any other source
 // code other than the Condition.
 if (CodeGenFunction::isInstrumentedCondition(C)) {
+  MCDCParameters BranchParams;
   MCD

[clang] [llvm] [MC/DC] Refactor: Make `MCDCParams` as `std::variant` (PR #81227)

2024-02-10 Thread NAKAMURA Takumi via cfe-commits 


@@ -250,18 +251,27 @@ struct CounterMappingRegion {
   };
 
   using MCDCConditionID = unsigned int;
-  struct MCDCParameters {
+  struct MCDCDecisionParameters {
 /// Byte Index of Bitmap Coverage Object for a Decision Region.
-unsigned BitmapIdx = 0;
+unsigned BitmapIdx;
 
 /// Number of Conditions used for a Decision Region.
-unsigned NumConditions = 0;
+unsigned NumConditions;
 
+MCDCDecisionParameters() = delete;
+  };
+
+  struct MCDCBranchParameters {
 /// IDs used to represent a branch region and other branch regions
 /// evaluated based on True and False branches.
-MCDCConditionID ID = 0, TrueID = 0, FalseID = 0;
+MCDCConditionID ID, TrueID, FalseID;
+
+MCDCBranchParameters() = delete;
   };
 
+  using MCDCParameters = std::variant;

chapuni wrote:

FYI, the size of `MCDCParameters` will be `12` (gnu) or `8` (libc++) finally, 
with shrinking a few members to `int16_t`. (#81257)

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

[clang] [llvm] [MC/DC] Refactor: Make `MCDCParams` as `std::variant` (PR #81227)

2024-02-09 Thread NAKAMURA Takumi via cfe-commits 

https://github.com/chapuni updated 
https://github.com/llvm/llvm-project/pull/81227

>From c2b49a5317bf5b8af419cba814f95cc9305bec21 Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi 
Date: Thu, 8 Feb 2024 23:12:54 +0900
Subject: [PATCH] [MC/DC] Refactor: Make `MCDCParams` as `std::variant`

Introduce `MCDCDecisionParameters` and `MCDCBranchParameters` and
make sure them not initialized as zero.

FIXME: Could we make `CoverageMappingRegion` as a smart tagged union?
---
 clang/lib/CodeGen/CoverageMappingGen.cpp  | 69 +
 .../ProfileData/Coverage/CoverageMapping.h| 77 ---
 .../ProfileData/Coverage/CoverageMapping.cpp  | 53 +++--
 .../Coverage/CoverageMappingReader.cpp| 17 ++--
 .../Coverage/CoverageMappingWriter.cpp| 23 --
 .../ProfileData/CoverageMappingTest.cpp   |  8 +-
 6 files changed, 149 insertions(+), 98 deletions(-)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp 
b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 0c43317642bca..da5d43cda9120 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -97,6 +97,8 @@ void CoverageSourceInfo::updateNextTokLoc(SourceLocation Loc) 
{
 namespace {
 using MCDCConditionID = CounterMappingRegion::MCDCConditionID;
 using MCDCParameters = CounterMappingRegion::MCDCParameters;
+using MCDCBranchParameters = CounterMappingRegion::MCDCBranchParameters;
+using MCDCDecisionParameters = CounterMappingRegion::MCDCDecisionParameters;
 
 /// A region of source code that can be mapped to a counter.
 class SourceMappingRegion {
@@ -185,7 +187,17 @@ class SourceMappingRegion {
 
   bool isBranch() const { return FalseCount.has_value(); }
 
-  bool isMCDCDecision() const { return MCDCParams.NumConditions != 0; }
+  bool isMCDCDecision() const {
+const auto *DecisionParams =
+std::get_if(&MCDCParams);
+assert(!DecisionParams || DecisionParams->NumConditions > 0);
+return DecisionParams;
+  }
+
+  const auto &getMCDCDecisionParams() const {
+return CounterMappingRegion::getParams(
+MCDCParams);
+  }
 
   const MCDCParameters &getMCDCParams() const { return MCDCParams; }
 };
@@ -483,13 +495,13 @@ class CoverageMappingBuilder {
 SR.ColumnEnd));
   } else if (Region.isBranch()) {
 MappingRegions.push_back(CounterMappingRegion::makeBranchRegion(
-Region.getCounter(), Region.getFalseCounter(),
-Region.getMCDCParams(), *CovFileID, SR.LineStart, SR.ColumnStart,
-SR.LineEnd, SR.ColumnEnd));
+Region.getCounter(), Region.getFalseCounter(), *CovFileID,
+SR.LineStart, SR.ColumnStart, SR.LineEnd, SR.ColumnEnd,
+Region.getMCDCParams()));
   } else if (Region.isMCDCDecision()) {
 MappingRegions.push_back(CounterMappingRegion::makeDecisionRegion(
-Region.getMCDCParams(), *CovFileID, SR.LineStart, SR.ColumnStart,
-SR.LineEnd, SR.ColumnEnd));
+Region.getMCDCDecisionParams(), *CovFileID, SR.LineStart,
+SR.ColumnStart, SR.LineEnd, SR.ColumnEnd));
   } else {
 MappingRegions.push_back(CounterMappingRegion::makeRegion(
 Region.getCounter(), *CovFileID, SR.LineStart, SR.ColumnStart,
@@ -865,8 +877,7 @@ struct CounterCoverageMappingBuilder
 std::optional StartLoc = std::nullopt,
 std::optional EndLoc = std::nullopt,
 std::optional FalseCount = std::nullopt,
-MCDCConditionID ID = 0, MCDCConditionID TrueID = 0,
-MCDCConditionID FalseID = 0) {
+const MCDCParameters &BranchParams = std::monostate()) {
 
 if (StartLoc && !FalseCount) {
   MostRecentLocation = *StartLoc;
@@ -885,9 +896,7 @@ struct CounterCoverageMappingBuilder
   StartLoc = std::nullopt;
 if (EndLoc && EndLoc->isInvalid())
   EndLoc = std::nullopt;
-RegionStack.emplace_back(Count, FalseCount,
- MCDCParameters{0, 0, ID, TrueID, FalseID},
- StartLoc, EndLoc);
+RegionStack.emplace_back(Count, FalseCount, BranchParams, StartLoc, 
EndLoc);
 
 return RegionStack.size() - 1;
   }
@@ -896,8 +905,8 @@ struct CounterCoverageMappingBuilder
 std::optional StartLoc = std::nullopt,
 std::optional EndLoc = std::nullopt) {
 
-RegionStack.emplace_back(MCDCParameters{BitmapIdx, Conditions}, StartLoc,
- EndLoc);
+RegionStack.emplace_back(MCDCDecisionParameters{BitmapIdx, Conditions},
+ StartLoc, EndLoc);
 
 return RegionStack.size() - 1;
   }
@@ -1042,9 +1051,10 @@ struct CounterCoverageMappingBuilder
 // function's SourceRegions) because it doesn't apply to any other source
 // code other than the Condition.
 if (CodeGenFunction::isInstrumentedCondition(C)) {
+  MCDCParameters BranchParams;
   MCDCCondi

[clang] [llvm] [MC/DC] Refactor: Make `MCDCParams` as `std::variant` (PR #81227)

2024-02-08 Thread NAKAMURA Takumi via cfe-commits 


@@ -264,9 +266,10 @@ class MCDCRecordProcessor {
   MCDCRecordProcessor(const BitVector &Bitmap,
   const CounterMappingRegion &Region,
   ArrayRef Branches)
-  : Bitmap(Bitmap), Region(Region), Branches(Branches),
-NumConditions(Region.MCDCParams.NumConditions),
-BitmapIdx(Region.MCDCParams.BitmapIdx * CHAR_BIT),
+  : Bitmap(Bitmap), Region(Region),
+DecisionParams(Region.getDecisionParams()), Branches(Branches),
+NumConditions(DecisionParams.NumConditions),
+BitmapIdx(DecisionParams.BitmapIdx * CHAR_BIT),
 Folded(NumConditions, false), IndependencePairs(NumConditions),
 TestVectors((size_t)1 << NumConditions) {}

chapuni wrote:

I'll make this mergeable later, possibly tonight.

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

[clang] [llvm] [MC/DC] Refactor: Make `MCDCParams` as `std::variant` (PR #81227)

2024-02-08 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-pgo

Author: NAKAMURA Takumi (chapuni)


Changes

Introduce `MCDCDecisionParameters` and `MCDCBranchParameters` and make sure 
them not initialized as zero.

FIXME: Could we make `CoverageMappingRegion` as a smart tagged union?

---

Patch is 25.91 KiB, truncated to 20.00 KiB below, full version: 
https://github.com/llvm/llvm-project/pull/81227.diff


6 Files Affected:

- (modified) clang/lib/CodeGen/CoverageMappingGen.cpp (+39-30) 
- (modified) llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h (+49-28) 
- (modified) llvm/lib/ProfileData/Coverage/CoverageMapping.cpp (+30-23) 
- (modified) llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp (+9-8) 
- (modified) llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp (+18-5) 
- (modified) llvm/unittests/ProfileData/CoverageMappingTest.cpp (+4-4) 


``diff
diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp 
b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 0c43317642bca4..da5d43cda91209 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -97,6 +97,8 @@ void CoverageSourceInfo::updateNextTokLoc(SourceLocation Loc) 
{
 namespace {
 using MCDCConditionID = CounterMappingRegion::MCDCConditionID;
 using MCDCParameters = CounterMappingRegion::MCDCParameters;
+using MCDCBranchParameters = CounterMappingRegion::MCDCBranchParameters;
+using MCDCDecisionParameters = CounterMappingRegion::MCDCDecisionParameters;
 
 /// A region of source code that can be mapped to a counter.
 class SourceMappingRegion {
@@ -185,7 +187,17 @@ class SourceMappingRegion {
 
   bool isBranch() const { return FalseCount.has_value(); }
 
-  bool isMCDCDecision() const { return MCDCParams.NumConditions != 0; }
+  bool isMCDCDecision() const {
+const auto *DecisionParams =
+std::get_if(&MCDCParams);
+assert(!DecisionParams || DecisionParams->NumConditions > 0);
+return DecisionParams;
+  }
+
+  const auto &getMCDCDecisionParams() const {
+return CounterMappingRegion::getParams(
+MCDCParams);
+  }
 
   const MCDCParameters &getMCDCParams() const { return MCDCParams; }
 };
@@ -483,13 +495,13 @@ class CoverageMappingBuilder {
 SR.ColumnEnd));
   } else if (Region.isBranch()) {
 MappingRegions.push_back(CounterMappingRegion::makeBranchRegion(
-Region.getCounter(), Region.getFalseCounter(),
-Region.getMCDCParams(), *CovFileID, SR.LineStart, SR.ColumnStart,
-SR.LineEnd, SR.ColumnEnd));
+Region.getCounter(), Region.getFalseCounter(), *CovFileID,
+SR.LineStart, SR.ColumnStart, SR.LineEnd, SR.ColumnEnd,
+Region.getMCDCParams()));
   } else if (Region.isMCDCDecision()) {
 MappingRegions.push_back(CounterMappingRegion::makeDecisionRegion(
-Region.getMCDCParams(), *CovFileID, SR.LineStart, SR.ColumnStart,
-SR.LineEnd, SR.ColumnEnd));
+Region.getMCDCDecisionParams(), *CovFileID, SR.LineStart,
+SR.ColumnStart, SR.LineEnd, SR.ColumnEnd));
   } else {
 MappingRegions.push_back(CounterMappingRegion::makeRegion(
 Region.getCounter(), *CovFileID, SR.LineStart, SR.ColumnStart,
@@ -865,8 +877,7 @@ struct CounterCoverageMappingBuilder
 std::optional StartLoc = std::nullopt,
 std::optional EndLoc = std::nullopt,
 std::optional FalseCount = std::nullopt,
-MCDCConditionID ID = 0, MCDCConditionID TrueID = 0,
-MCDCConditionID FalseID = 0) {
+const MCDCParameters &BranchParams = std::monostate()) {
 
 if (StartLoc && !FalseCount) {
   MostRecentLocation = *StartLoc;
@@ -885,9 +896,7 @@ struct CounterCoverageMappingBuilder
   StartLoc = std::nullopt;
 if (EndLoc && EndLoc->isInvalid())
   EndLoc = std::nullopt;
-RegionStack.emplace_back(Count, FalseCount,
- MCDCParameters{0, 0, ID, TrueID, FalseID},
- StartLoc, EndLoc);
+RegionStack.emplace_back(Count, FalseCount, BranchParams, StartLoc, 
EndLoc);
 
 return RegionStack.size() - 1;
   }
@@ -896,8 +905,8 @@ struct CounterCoverageMappingBuilder
 std::optional StartLoc = std::nullopt,
 std::optional EndLoc = std::nullopt) {
 
-RegionStack.emplace_back(MCDCParameters{BitmapIdx, Conditions}, StartLoc,
- EndLoc);
+RegionStack.emplace_back(MCDCDecisionParameters{BitmapIdx, Conditions},
+ StartLoc, EndLoc);
 
 return RegionStack.size() - 1;
   }
@@ -1042,9 +1051,10 @@ struct CounterCoverageMappingBuilder
 // function's SourceRegions) because it doesn't apply to any other source
 // code other than the Condition.
 if (CodeGenFunction::isInstrumentedCondition(C)) {
+  MCDCParameters BranchParams;
   MCDCConditionID ID = MCDCBuilder.get

[clang] [llvm] [MC/DC] Refactor: Make `MCDCParams` as `std::variant` (PR #81227)

2024-02-08 Thread NAKAMURA Takumi via cfe-commits 

https://github.com/chapuni created 
https://github.com/llvm/llvm-project/pull/81227

Introduce `MCDCDecisionParameters` and `MCDCBranchParameters` and make sure 
them not initialized as zero.

FIXME: Could we make `CoverageMappingRegion` as a smart tagged union?

>From c2b49a5317bf5b8af419cba814f95cc9305bec21 Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi 
Date: Thu, 8 Feb 2024 23:12:54 +0900
Subject: [PATCH] [MC/DC] Refactor: Make `MCDCParams` as `std::variant`

Introduce `MCDCDecisionParameters` and `MCDCBranchParameters` and
make sure them not initialized as zero.

FIXME: Could we make `CoverageMappingRegion` as a smart tagged union?
---
 clang/lib/CodeGen/CoverageMappingGen.cpp  | 69 +
 .../ProfileData/Coverage/CoverageMapping.h| 77 ---
 .../ProfileData/Coverage/CoverageMapping.cpp  | 53 +++--
 .../Coverage/CoverageMappingReader.cpp| 17 ++--
 .../Coverage/CoverageMappingWriter.cpp| 23 --
 .../ProfileData/CoverageMappingTest.cpp   |  8 +-
 6 files changed, 149 insertions(+), 98 deletions(-)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp 
b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 0c43317642bca4..da5d43cda91209 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -97,6 +97,8 @@ void CoverageSourceInfo::updateNextTokLoc(SourceLocation Loc) 
{
 namespace {
 using MCDCConditionID = CounterMappingRegion::MCDCConditionID;
 using MCDCParameters = CounterMappingRegion::MCDCParameters;
+using MCDCBranchParameters = CounterMappingRegion::MCDCBranchParameters;
+using MCDCDecisionParameters = CounterMappingRegion::MCDCDecisionParameters;
 
 /// A region of source code that can be mapped to a counter.
 class SourceMappingRegion {
@@ -185,7 +187,17 @@ class SourceMappingRegion {
 
   bool isBranch() const { return FalseCount.has_value(); }
 
-  bool isMCDCDecision() const { return MCDCParams.NumConditions != 0; }
+  bool isMCDCDecision() const {
+const auto *DecisionParams =
+std::get_if(&MCDCParams);
+assert(!DecisionParams || DecisionParams->NumConditions > 0);
+return DecisionParams;
+  }
+
+  const auto &getMCDCDecisionParams() const {
+return CounterMappingRegion::getParams(
+MCDCParams);
+  }
 
   const MCDCParameters &getMCDCParams() const { return MCDCParams; }
 };
@@ -483,13 +495,13 @@ class CoverageMappingBuilder {
 SR.ColumnEnd));
   } else if (Region.isBranch()) {
 MappingRegions.push_back(CounterMappingRegion::makeBranchRegion(
-Region.getCounter(), Region.getFalseCounter(),
-Region.getMCDCParams(), *CovFileID, SR.LineStart, SR.ColumnStart,
-SR.LineEnd, SR.ColumnEnd));
+Region.getCounter(), Region.getFalseCounter(), *CovFileID,
+SR.LineStart, SR.ColumnStart, SR.LineEnd, SR.ColumnEnd,
+Region.getMCDCParams()));
   } else if (Region.isMCDCDecision()) {
 MappingRegions.push_back(CounterMappingRegion::makeDecisionRegion(
-Region.getMCDCParams(), *CovFileID, SR.LineStart, SR.ColumnStart,
-SR.LineEnd, SR.ColumnEnd));
+Region.getMCDCDecisionParams(), *CovFileID, SR.LineStart,
+SR.ColumnStart, SR.LineEnd, SR.ColumnEnd));
   } else {
 MappingRegions.push_back(CounterMappingRegion::makeRegion(
 Region.getCounter(), *CovFileID, SR.LineStart, SR.ColumnStart,
@@ -865,8 +877,7 @@ struct CounterCoverageMappingBuilder
 std::optional StartLoc = std::nullopt,
 std::optional EndLoc = std::nullopt,
 std::optional FalseCount = std::nullopt,
-MCDCConditionID ID = 0, MCDCConditionID TrueID = 0,
-MCDCConditionID FalseID = 0) {
+const MCDCParameters &BranchParams = std::monostate()) {
 
 if (StartLoc && !FalseCount) {
   MostRecentLocation = *StartLoc;
@@ -885,9 +896,7 @@ struct CounterCoverageMappingBuilder
   StartLoc = std::nullopt;
 if (EndLoc && EndLoc->isInvalid())
   EndLoc = std::nullopt;
-RegionStack.emplace_back(Count, FalseCount,
- MCDCParameters{0, 0, ID, TrueID, FalseID},
- StartLoc, EndLoc);
+RegionStack.emplace_back(Count, FalseCount, BranchParams, StartLoc, 
EndLoc);
 
 return RegionStack.size() - 1;
   }
@@ -896,8 +905,8 @@ struct CounterCoverageMappingBuilder
 std::optional StartLoc = std::nullopt,
 std::optional EndLoc = std::nullopt) {
 
-RegionStack.emplace_back(MCDCParameters{BitmapIdx, Conditions}, StartLoc,
- EndLoc);
+RegionStack.emplace_back(MCDCDecisionParameters{BitmapIdx, Conditions},
+ StartLoc, EndLoc);
 
 return RegionStack.size() - 1;
   }
@@ -1042,9 +1051,10 @@ struct CounterCoverageMappingBuilder
 // function's SourceRegions) because it