[Coverage] MCDC: Move findIndependencePairs deferred into MCDCRecord #121188

chapuni · 2024-12-27T08:32:23Z

The result of "Independence pairs" is not mergeable. This change makes defers re-calculation of "Independence pairs" after merging test vectors.

No apparent behavior changes.

llvmbot · 2024-12-27T08:33:01Z

@llvm/pr-subscribers-pgo

Author: NAKAMURA Takumi (chapuni)

Changes

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

2 Files Affected:

(modified) llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h (+16-11)
(modified) llvm/lib/ProfileData/Coverage/CoverageMapping.cpp (+35-32)

diff --git a/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h b/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
index 42da188fef34ee..4f28f5da6cf81f 100644
--- a/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
+++ b/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
@@ -35,6 +35,7 @@
 #include <cstdint>
 #include <iterator>
 #include <memory>
+#include <optional>
 #include <sstream>
 #include <string>
 #include <system_error>
@@ -451,19 +452,22 @@ struct MCDCRecord {
 private:
   CounterMappingRegion Region;
   TestVectors TV;
-  TVPairMap IndependencePairs;
+  std::optional<TVPairMap> IndependencePairs;
   BoolVector Folded;
   CondIDMap PosToID;
   LineColPairMap CondLoc;
 
 public:
   MCDCRecord(const CounterMappingRegion &Region, TestVectors &&TV,
-             TVPairMap &&IndependencePairs, BoolVector &&Folded,
-             CondIDMap &&PosToID, LineColPairMap &&CondLoc)
-      : Region(Region), TV(std::move(TV)),
-        IndependencePairs(std::move(IndependencePairs)),
-        Folded(std::move(Folded)), PosToID(std::move(PosToID)),
-        CondLoc(std::move(CondLoc)){};
+             BoolVector &&Folded, CondIDMap &&PosToID, LineColPairMap &&CondLoc)
+      : Region(Region), TV(std::move(TV)), Folded(std::move(Folded)),
+        PosToID(std::move(PosToID)), CondLoc(std::move(CondLoc)) {
+    findIndependencePairs();
+  }
+
+  // Compare executed test vectors against each other to find an independence
+  // pairs for each condition.  This processing takes the most time.
+  void findIndependencePairs();
 
   const CounterMappingRegion &getDecisionRegion() const { return Region; }
   unsigned getNumConditions() const {
@@ -494,10 +498,10 @@ struct MCDCRecord {
   /// TestVectors requires a translation from a ordinal position to actual
   /// condition ID. This is done via PosToID[].
   bool isConditionIndependencePairCovered(unsigned Condition) const {
+    assert(IndependencePairs);
     auto It = PosToID.find(Condition);
-    if (It != PosToID.end())
-      return IndependencePairs.contains(It->second);
-    llvm_unreachable("Condition ID without an Ordinal mapping");
+    assert(It != PosToID.end() && "Condition ID without an Ordinal mapping");
+    return IndependencePairs->contains(It->second);
   }
 
   /// Return the Independence Pair that covers the given condition. Because
@@ -507,7 +511,8 @@ struct MCDCRecord {
   /// via PosToID[].
   TVRowPair getConditionIndependencePair(unsigned Condition) {
     assert(isConditionIndependencePairCovered(Condition));
-    return IndependencePairs[PosToID[Condition]];
+    assert(IndependencePairs);
+    return (*IndependencePairs)[PosToID[Condition]];
   }
 
   float getPercentCovered() const {
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
index 87d8bb1bbb79c7..2c7a77bac09328 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -221,6 +221,40 @@ Expected<int64_t> CounterMappingContext::evaluate(const Counter &C) const {
   return LastPoppedValue;
 }
 
+// Find an independence pair for each condition:
+// - The condition is true in one test and false in the other.
+// - The decision outcome is true one test and false in the other.
+// - All other conditions' values must be equal or marked as "don't care".
+void MCDCRecord::findIndependencePairs() {
+  if (IndependencePairs)
+    return;
+
+  IndependencePairs.emplace();
+
+  unsigned NumTVs = TV.size();
+  // Will be replaced to shorter expr.
+  unsigned TVTrueIdx = std::distance(
+      TV.begin(),
+      std::find_if(TV.begin(), TV.end(),
+                   [&](auto I) { return (I.second == MCDCRecord::MCDC_True); })
+
+  );
+  for (unsigned I = TVTrueIdx; I < NumTVs; ++I) {
+    const auto &[A, ACond] = TV[I];
+    assert(ACond == MCDCRecord::MCDC_True);
+    for (unsigned J = 0; J < TVTrueIdx; ++J) {
+      const auto &[B, BCond] = TV[J];
+      assert(BCond == MCDCRecord::MCDC_False);
+      // If the two vectors differ in exactly one condition, ignoring DontCare
+      // conditions, we have found an independence pair.
+      auto AB = A.getDifferences(B);
+      if (AB.count() == 1)
+        IndependencePairs->insert(
+            {AB.find_first(), std::make_pair(J + 1, I + 1)});
+    }
+  }
+}
+
 mcdc::TVIdxBuilder::TVIdxBuilder(const SmallVectorImpl<ConditionIDs> &NextIDs,
                                  int Offset)
     : Indices(NextIDs.size()) {
@@ -375,9 +409,6 @@ class MCDCRecordProcessor : NextIDsBuilder, mcdc::TVIdxBuilder {
   /// ExecutedTestVectorBitmap.
   MCDCRecord::TestVectors &ExecVectors;
 
-  /// Number of False items in ExecVectors
-  unsigned NumExecVectorsF;
-
 #ifndef NDEBUG
   DenseSet<unsigned> TVIdxs;
 #endif
@@ -446,34 +477,11 @@ class MCDCRecordProcessor : NextIDsBuilder, mcdc::TVIdxBuilder {
     // Fill ExecVectors order by False items and True items.
     // ExecVectors is the alias of ExecVectorsByCond[false], so
     // Append ExecVectorsByCond[true] on it.
-    NumExecVectorsF = ExecVectors.size();
     auto &ExecVectorsT = ExecVectorsByCond[true];
     ExecVectors.append(std::make_move_iterator(ExecVectorsT.begin()),
                        std::make_move_iterator(ExecVectorsT.end()));
   }
 
-  // Find an independence pair for each condition:
-  // - The condition is true in one test and false in the other.
-  // - The decision outcome is true one test and false in the other.
-  // - All other conditions' values must be equal or marked as "don't care".
-  void findIndependencePairs() {
-    unsigned NumTVs = ExecVectors.size();
-    for (unsigned I = NumExecVectorsF; I < NumTVs; ++I) {
-      const auto &[A, ACond] = ExecVectors[I];
-      assert(ACond == MCDCRecord::MCDC_True);
-      for (unsigned J = 0; J < NumExecVectorsF; ++J) {
-        const auto &[B, BCond] = ExecVectors[J];
-        assert(BCond == MCDCRecord::MCDC_False);
-        // If the two vectors differ in exactly one condition, ignoring DontCare
-        // conditions, we have found an independence pair.
-        auto AB = A.getDifferences(B);
-        if (AB.count() == 1)
-          IndependencePairs.insert(
-              {AB.find_first(), std::make_pair(J + 1, I + 1)});
-      }
-    }
-  }
-
 public:
   /// Process the MC/DC Record in order to produce a result for a boolean
   /// expression. This process includes tracking the conditions that comprise
@@ -509,13 +517,8 @@ class MCDCRecordProcessor : NextIDsBuilder, mcdc::TVIdxBuilder {
     // Using Profile Bitmap from runtime, mark the executed test vectors.
     findExecutedTestVectors();
 
-    // Compare executed test vectors against each other to find an independence
-    // pairs for each condition.  This processing takes the most time.
-    findIndependencePairs();
-
     // Record Test vectors, executed vectors, and independence pairs.
-    return MCDCRecord(Region, std::move(ExecVectors),
-                      std::move(IndependencePairs), std::move(Folded),
+    return MCDCRecord(Region, std::move(ExecVectors), std::move(Folded),
                       std::move(PosToID), std::move(CondLoc));
   }
 };

ornata · 2025-01-06T04:49:32Z

llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h

-    if (It != PosToID.end())
-      return IndependencePairs.contains(It->second);
-    llvm_unreachable("Condition ID without an Ordinal mapping");
+    assert(It != PosToID.end() && "Condition ID without an Ordinal mapping");


why is this changed to an assert from unreachable?

Do we expect "satisfies or crash" here? I wanted to eliminate the condition.

ornata · 2025-01-06T04:50:18Z

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

@@ -221,6 +221,40 @@ Expected<int64_t> CounterMappingContext::evaluate(const Counter &C) const {
  return LastPoppedValue;
 }

+// Find an independence pair for each condition:


can this be a doxygen comment in CoverageMapping.h?

ornata · 2025-01-06T04:52:05Z

llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h

+
+  // Compare executed test vectors against each other to find an independence
+  // pairs for each condition.  This processing takes the most time.
+  void findIndependencePairs();


would it make sense to have this actually return the std::optional<TVPairMap>?

It'd make sense since it always flush and reconstruct the map.

ornata · 2025-01-06T04:52:55Z

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

+
+  IndependencePairs.emplace();
+
+  unsigned NumTVs = TV.size();


ornata · 2025-01-06T04:53:25Z

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

+
+  unsigned NumTVs = TV.size();
+  // Will be replaced to shorter expr.
+  unsigned TVTrueIdx = std::distance(


ornata · 2025-01-06T04:59:26Z

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

+  for (unsigned I = TVTrueIdx; I < NumTVs; ++I) {
+    const auto &[A, ACond] = TV[I];
+    assert(ACond == MCDCRecord::MCDC_True);
+    for (unsigned J = 0; J < TVTrueIdx; ++J) {


if I understand correctly, we assume that

The outer loop handles true conditions

The inner loop handles false conditions

I think this renaming may be clearer:

// All true test vectors are in the range [FirstTrueTVIdx, NumTVs). for (unsigned TrueTVIdx = FirstTrueTVIdx; TrueTVIdx < NumTVs; ++TrueTVIdx) { ... // All false test vectors are in the range [0, FirstTrueTVIdx) for (unsigned FalseTVIdx = 0; FalseTVIdx < FirstTrueTVIdx; ++ FalseTVIdx) { ... } }

Ah this is all code movement. Duh. Perhaps we can address this in a follow-up NFC commit.

ornata · 2025-01-06T05:00:55Z

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

+// - The decision outcome is true one test and false in the other.
+// - All other conditions' values must be equal or marked as "don't care".
+void MCDCRecord::findIndependencePairs() {
+  if (IndependencePairs)


Debug output for something like "independence pairs already computed; not finding anything"?

"Soft updates" is assumed. It is invalidated if "merging" is executed.

ornata · 2025-01-06T05:04:25Z

This is all code movement so most my comments are probably irrelevant.

This looks fine.

MaskRay · 2025-01-06T06:57:44Z

llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h

@@ -507,7 +511,8 @@ struct MCDCRecord {
  /// via PosToID[].
  TVRowPair getConditionIndependencePair(unsigned Condition) {
    assert(isConditionIndependencePairCovered(Condition));
-    return IndependencePairs[PosToID[Condition]];
+    assert(IndependencePairs);


assert is unneeded due to immediate dereference

It's redundant since isConditionIndependencePairCovered checks IndependencePairs. That said, I want to reconfirm before its use *IndependencePairs.

[Coverage] MCDC: Move findIndependencePairs into MCDCRecord

978070d

chapuni requested review from MaskRay, ornata and evodius96 December 27, 2024 08:32

llvmbot added the PGO Profile Guided Optimizations label Dec 27, 2024

This was referenced Dec 27, 2024

llvm-cov: Show FileCoverageSummary with getCoverageForFile() #121192

Open

[Coverage] Sort MCDCRecord::ExecVectors order by Bitmap index #121195

Open

llvm-cov: Merge records for template instantiations #121197

Draft

chapuni changed the title ~~[Coverage] MCDC: Move findIndependencePairs into MCDCRecord~~ [Coverage] MCDC: Move findIndependencePairs deferred into MCDCRecord Jan 6, 2025

ornata reviewed Jan 6, 2025

View reviewed changes

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

IndependencePairs.emplace();

unsigned NumTVs = TV.size();

Copy link

ornata Jan 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const?

ornata reviewed Jan 6, 2025

View reviewed changes

ornata approved these changes Jan 6, 2025

View reviewed changes

MaskRay reviewed Jan 6, 2025

View reviewed changes

chapuni merged commit 9709795 into main Jan 6, 2025
10 checks passed

chapuni deleted the users/chapuni/cov/merge/mov_ind branch January 6, 2025 23:52

[Coverage] MCDC: Move findIndependencePairs deferred into MCDCRecord #121188

[Coverage] MCDC: Move findIndependencePairs deferred into MCDCRecord #121188

Uh oh!

Conversation

chapuni commented Dec 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Dec 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ornata commented Jan 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chapuni commented Dec 27, 2024 •

edited

Loading