CoverageMappingWriter: Emit Decision before Expansion

[chapuni]

To relax scanning record, tweak order by Decision < Expansion, or Expansion could not be distinguished whether it belonged to Decision or not.

Relevant to #77871

[llvmbot]

@llvm/pr-subscribers-pgo

Author: NAKAMURA Takumi (chapuni)

Changes

Test cases (for llvm) would be required.

https://godbolt.org/z/orE99Y7hT

foo:
  Expansion,File 0, 4:11 -&gt; 4:12 = #<!-- -->0 (Expanded file = 1)
  Decision,File 0, 4:11 -&gt; 4:20 = M:0, C:2
  Expansion,File 0, 4:19 -&gt; 4:20 = #<!-- -->1 (Expanded file = 2)
  Branch,File 1, 1:14 -&gt; 1:17 = #<!-- -->1, (#<!-- -->0 - #<!-- -->1) [1,2,0] 
  Branch,File 2, 1:14 -&gt; 1:17 = #<!-- -->2, (#<!-- -->1 - #<!-- -->2) [2,0,0] 

To relax scanning record, tweak order by Decision < Expansion. Relevant to #77871

---

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

2 Files Affected:

• (modified) llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp (+12-1)
• (modified) llvm/unittests/ProfileData/CoverageMappingTest.cpp (+36-1)

diff --git a/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp b/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
index 1c7d8a8909c488..e8597676003a02 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
@@ -167,7 +167,18 @@ void CoverageMappingWriter::write(raw_ostream &OS) {
       return LHS.FileID < RHS.FileID;
     if (LHS.startLoc() != RHS.startLoc())
       return LHS.startLoc() < RHS.startLoc();
-    return LHS.Kind < RHS.Kind;
+
+    // Put `Decision` before `Expansion`.
+    auto getKindKey = [](CounterMappingRegion::RegionKind Kind) {
+      return (Kind == CounterMappingRegion::MCDCDecisionRegion
+                  ? 2 * CounterMappingRegion::ExpansionRegion - 1
+                  : 2 * Kind);
+    };
+
+    auto LHSKindKey = getKindKey(LHS.Kind);
+    auto RHSKindKey = getKindKey(RHS.Kind);
+
+    return LHSKindKey < RHSKindKey;
   });
 
   // Write out the fileid -> filename mapping.
diff --git a/llvm/unittests/ProfileData/CoverageMappingTest.cpp b/llvm/unittests/ProfileData/CoverageMappingTest.cpp
index 1cf497cbdc2e61..1057e94cc45120 100644
--- a/llvm/unittests/ProfileData/CoverageMappingTest.cpp
+++ b/llvm/unittests/ProfileData/CoverageMappingTest.cpp
@@ -311,7 +311,6 @@ TEST_P(CoverageMappingTest, basic_write_read) {
     ASSERT_EQ(Input.Regions[I].Kind, Output.Regions[I].Kind);
   }
 }
-
 TEST_P(CoverageMappingTest, correct_deserialize_for_more_than_two_files) {
   const char *FileNames[] = {"bar", "baz", "foo"};
   static const unsigned N = std::size(FileNames);
@@ -874,6 +873,42 @@ TEST_P(CoverageMappingTest, non_code_region_bitmask) {
   ASSERT_EQ(1U, Names.size());
 }
 
+// Test the order of MCDCDecision before Expansion
+TEST_P(CoverageMappingTest, decision_before_expansion) {
+  startFunction("foo", 0x1234);
+  addCMR(Counter::getCounter(0), "foo", 3, 23, 5, 2);
+
+  // This(4:11) was put after Expansion(4:11) before the fix
+  addMCDCDecisionCMR(0, 2, "foo", 4, 11, 4, 20);
+
+  addExpansionCMR("foo", "A", 4, 11, 4, 12);
+  addExpansionCMR("foo", "B", 4, 19, 4, 20);
+  addCMR(Counter::getCounter(0), "A", 1, 14, 1, 17);
+  addCMR(Counter::getCounter(0), "A", 1, 14, 1, 17);
+  addMCDCBranchCMR(Counter::getCounter(0), Counter::getCounter(1), 1, 2, 0, "A",
+                   1, 14, 1, 17);
+  addCMR(Counter::getCounter(1), "B", 1, 14, 1, 17);
+  addMCDCBranchCMR(Counter::getCounter(1), Counter::getCounter(2), 2, 0, 0, "B",
+                   1, 14, 1, 17);
+
+  // InputFunctionCoverageData::Regions is rewritten after the write.
+  auto InputRegions = InputFunctions.back().Regions;
+
+  writeAndReadCoverageRegions();
+
+  const auto &OutputRegions = OutputFunctions.back().Regions;
+
+  size_t N = ArrayRef(InputRegions).size();
+  ASSERT_EQ(N, OutputRegions.size());
+  for (size_t I = 0; I < N; ++I) {
+    ASSERT_EQ(InputRegions[I].Kind, OutputRegions[I].Kind);
+    ASSERT_EQ(InputRegions[I].FileID, OutputRegions[I].FileID);
+    ASSERT_EQ(InputRegions[I].ExpandedFileID, OutputRegions[I].ExpandedFileID);
+    ASSERT_EQ(InputRegions[I].startLoc(), OutputRegions[I].startLoc());
+    ASSERT_EQ(InputRegions[I].endLoc(), OutputRegions[I].endLoc());
+  }
+}
+
 TEST_P(CoverageMappingTest, strip_filename_prefix) {
   ProfileWriter.addRecord({"file1:func", 0x1234, {0}}, Err);
 

[chapuni]

The testcase (was in the initial description)

(https://godbolt.org/z/orE99Y7hT)

foo:
  File 0, 3:23 -> 5:2 = #0
  Expansion,File 0, 4:11 -> 4:12 = #0 (Expanded file = 1)
  Decision,File 0, 4:11 -> 4:20 = M:0, C:2
  Expansion,File 0, 4:19 -> 4:20 = #1 (Expanded file = 2)
  File 1, 1:14 -> 1:17 = #0
  File 1, 1:14 -> 1:17 = #0
  Branch,File 1, 1:14 -> 1:17 = #1, (#0 - #1) [1,2,0] 
  File 2, 1:14 -> 1:17 = #1
  Branch,File 2, 1:14 -> 1:17 = #2, (#1 - #2) [2,0,0] 

[ornata]

Would it make sense to make this into a real function, so that its return value can be tested in mock?

[chapuni]

I think it'd not be required till whole reordering would come.

[ornata]

LGTM

[MaskRay]

To relax scanning record, tweak order by Decision < Expansion.

I think this is fine, but the rationale should be clarified.