Introduce mcdc::TVIdxBuilder (LLVM side, NFC) #80676

chapuni · 2024-02-05T13:00:33Z

This is a preparation of incoming Clang changes (#82448) and just checks TVIdx is calculated correctly. NFC.

TVIdxBuilder calculates deterministic Indices for each Condition Node. It is used for clang to emit TestVector indices (aka ID) and for llvm-cov to reconstruct TestVectors.

This includes the unittest CoverageMappingTest.TVIdxBuilder.

See also
https://discourse.llvm.org/t/rfc-coverage-new-algorithm-and-file-format-for-mc-dc/76798

This accept current version of profdata. The output might be different. See also https://discourse.llvm.org/t/rfc-coverage-new-algorithm-and-file-format-for-mc-dc/76798

…de)" This reverts commit d168e0c.

Deprecate `TestVectors`, since no one uses it. This affects the output order of ExecVectors. The current impl emits sorted by binary value of ExecVector. This impl emits along the traversal of `buildTestVector()`.

This accepts current version of profdata. The output might be different. See also https://discourse.llvm.org/t/rfc-coverage-new-algorithm-and-file-format-for-mc-dc/76798

chapuni · 2024-02-06T10:17:28Z

I reworked to align the current implementation.

evodius96 · 2024-02-08T23:58:50Z

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

+  Nodes[0].Width = 1;
+  Q.push_back(0);
+
+  unsigned Ord = 0;


Can you add a bit more commentary on the overall process of the algorithm (and MCDCTVIdxBuilder)? What are the expected inputs and the expected output, generally? I think it would help with the readability a bit more.

I've added comments and the unittest. Hope it helps!

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

evodius96 · 2024-02-09T00:05:42Z

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

    // `Index` encodes the bitmask of true values and is initially 0.
    MCDCRecord::TestVector TV(NumConditions, MCDCRecord::MCDC_DontCare);
-    buildTestVector(TV, 1, 0);
+    buildTestVector(TV, 0, 0, 0);
+    assert(TVIdxs.size() == NumTestVectors && "TVIdxs wasn't fulfilled");


Good assert

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

chapuni

Thanks for taking a look. I will update this evening.

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

ornata · 2024-02-12T02:59:11Z

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

+public:
+  struct MCDCNode {
+    int InCount = 0; /// Reference count; temporary use
+    int Width;       /// Number of paths (>= 1)


Why not name it NumPaths?

It was W in my prototype design. I have to update my imagination if this would be renamed.
Or, does the comment mislead?

https://en.wikipedia.org/wiki/Width_(disambiguation) Width has many meanings for graphs. :)

Which is better for me to rename to easy N or to define Width here.
I haven't learnt the graph theory though, looks like I have introduced similar concept, as far as I read introductions of the theory.

Let me think more.

* Split out `Indices[ID][Cond]` * Let `Nodes` debug-only. * Introduce `Offset` * Introduce `HardMaxTVs`

* Sink `TVIdxBuilder` into `mcdc::`. * The ctor accepts `SmallVector<ConditionIDs>` indexed by `ID`. * `class NextIDsBuilder` provides `NextIDs` as`SmallVector<ConditionIDs>`, for `TVIdxBuilder` to use it before `MCDCRecordProcessor()`. It was `BranchParamsMap` or `Map` as `DenseMap<Branch>`. * `NodeIDs` and `Fetcher` function are deprecated.

ornata · 2024-02-21T01:38:38Z

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

+      Indices[ID][I] = NextNode.Width;
+      auto NextWidth = int64_t(NextNode.Width) + Node.Width;
+      if (NextWidth > HardMaxTVs) {
+        NumTestVectors = HardMaxTVs; // Overflow


Would it be possible to add debug output when there is an overflow?

I guess overflow is impossible in llvm-cov.
OTOH, Clang side will catch such a overflow and tell it with HardMaxTVs for warnings/errors. See MCDCCoverageBuilder in CoverageMappingGen.cpp, and mcdc-error-conditions.cpp.

I wonder it would be valuable to add debug message here.

ornata · 2024-02-21T01:40:04Z

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

+    }
+  }
+
+  std::sort(Decisions.begin(), Decisions.end());


Can you use llvm::sort?

I think it takes ranges so you can just do llvm::sort(Decisions)

I've forgot it.

ZhuUx · 2024-06-26T09:46:16Z

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

+    }
+  }
+
+  // Sort key ordered by <-Width, Ord>


Excuse me. Why end nodes of the decision need to be sorted in descending order of width?

https://discourse.llvm.org/t/rfc-coverage-new-algorithm-and-file-format-for-mc-dc/76798#other-minor-implementations-5

I think it may be available for integrating branch and mcdc.

See also;
https://discourse.llvm.org/t/rfc-region-branch-coverage-by-bitmap/79629#branch-coverage-and-mcdc-test-vectors-4

So motivation of the sort is to prepare for truncating all conditions with width=1, is it?
If so, why not just remove them then with an O(n) method?

Why did you think they could be removed? It's the prerequisite to assign identical IDs between clang and llvm-cov.

I see, if we did not sort them, we can not reduce size of bitmaps effectively since there might be other conditions with width>1 getting large index.

I'm trying to remove the sort because I find it breaks some nature if the topological relationship of conditions is abnormal (e.g conditions from pattern matching in rust) and generates troubled index maps. I would investigate it more. Thanks!

Interesting. Could you introduce me the discussion, or paste the problematic cond tree anywhere?

☺️ Sorry I found I made a mistake. I previously encountered a example with decision tree:

flowchart TD C0 -- T --> C1 C1 -- F --> F1 C1 -- T --> C3 C0 -- F --> C2 C2 -- T --> C3 C2 -- F --> F2 C3 -- F --> C4 C3 -- T --> T3 C4 -- T --> T4 C4 -- F --> F4

Loading

Normal boolean expressions can not generate such decision tree. In normal boolean decisions, either true next of a condition is a next (true or false) of its false next, or false next of a condition is a next of its true next.

I tried to disable the sort and fixed some cases. So I guessed something was broken by the sort. But that's wrong. The real problem is the way to update cond bitmap, which is specified for previous cond bitmap but is inappropriate for current. This implementation still works for pattern matching, thanks to your excellent efforts!

At a glance, F1 and F2 have w=1, and others have w=2. Correct?

Yes. The index map is right. The key difference is at most one of C0 and C2 can be true, which not likes (C0 && C1) || C2. In previous I updated false bit of C2 when C0 is true since if C0 is true, C2 must be false. While at present we can not do this.

ZhuUx · 2024-06-26T11:14:46Z

Sure, I’d collate it. Please wait some time.

chapuni added 5 commits February 5, 2024 21:58

Implement MCDCTVIdxBuilder and MCDCTestVectorBuilder (LLVM side)

d168e0c

This accept current version of profdata. The output might be different. See also https://discourse.llvm.org/t/rfc-coverage-new-algorithm-and-file-format-for-mc-dc/76798

Revert "Implement MCDCTVIdxBuilder and MCDCTestVectorBuilder (LLVM si…

35b19ea

…de)" This reverts commit d168e0c.

[Coverage] MCDCRecordProcessor: Find ExecVectors directly

8c777eb

Deprecate `TestVectors`, since no one uses it. This affects the output order of ExecVectors. The current impl emits sorted by binary value of ExecVector. This impl emits along the traversal of `buildTestVector()`.

Merge branch 'mcdc/xv' into HEAD

56042d3

Implement MCDCTVIdxBuilder (LLVM side)

5432aec

This accepts current version of profdata. The output might be different. See also https://discourse.llvm.org/t/rfc-coverage-new-algorithm-and-file-format-for-mc-dc/76798

chapuni changed the title ~~Implement MCDCTVIdxBuilder and MCDCTestVectorBuilder (LLVM side)~~ Implement MCDCTVIdxBuilder (LLVM side) Feb 6, 2024

chapuni requested review from ornata, MaskRay and evodius96 February 6, 2024 10:20

chapuni marked this pull request as ready for review February 6, 2024 10:20

chapuni added 2 commits February 6, 2024 21:42

Update comments and assertions

3ee8a61

Merge remote-tracking branch 'origin/main' into mcdc/tvidx

2fd504a

evodius96 reviewed Feb 8, 2024

View reviewed changes

evodius96 reviewed Feb 9, 2024

View reviewed changes

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp Outdated Show resolved Hide resolved

evodius96 reviewed Feb 9, 2024

View reviewed changes

ornata reviewed Feb 12, 2024

View reviewed changes

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp Outdated Show resolved Hide resolved

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp Show resolved Hide resolved

chapuni commented Feb 12, 2024

View reviewed changes

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp Outdated Show resolved Hide resolved

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp Show resolved Hide resolved

ornata reviewed Feb 12, 2024

View reviewed changes

chapuni added 7 commits February 12, 2024 18:31

Reorganize TVIdxBuilder

1f0f3fc

* Split out `Indices[ID][Cond]` * Let `Nodes` debug-only. * Introduce `Offset` * Introduce `HardMaxTVs`

Merge remote-tracking branch 'origin/main' into mcdc/tvidx

06c0801

Merge remote-tracking branch 'origin/main' into HEAD

aa5b2f5

remove <functional>

753d0ad

Update comments.

17cbac7

Add unittest

1a4ffa7

chapuni mentioned this pull request Feb 21, 2024

[MC/DC][Coverage] Loosen the limit of NumConds from 6 #82448

Merged

ornata reviewed Feb 21, 2024

View reviewed changes

chapuni requested a review from hanickadot February 21, 2024 15:17

chapuni added 3 commits February 22, 2024 01:15

Use llvm::sort

c96fd2c

EXPECT_

357a693

Merge remote-tracking branch 'chapuni/main' into mcdc/tvidx

b6c1174

hanickadot approved these changes Feb 25, 2024

View reviewed changes

evodius96 approved these changes Feb 25, 2024

View reviewed changes

chapuni changed the title ~~Implement MCDCTVIdxBuilder (LLVM side)~~ Introduce mcdc::TVIdxBuilder (LLVM side, NFC) Feb 26, 2024

ornata approved these changes Feb 26, 2024

View reviewed changes

chapuni merged commit c087beb into llvm:main Feb 26, 2024
3 of 4 checks passed

chapuni deleted the mcdc/tvidx branch February 26, 2024 04:23

ZhuUx reviewed Jun 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce mcdc::TVIdxBuilder (LLVM side, NFC) #80676

Introduce mcdc::TVIdxBuilder (LLVM side, NFC) #80676

chapuni commented Feb 5, 2024 •

edited

Loading

chapuni commented Feb 6, 2024

evodius96 Feb 8, 2024

chapuni Feb 21, 2024

evodius96 Feb 9, 2024

chapuni left a comment

ornata Feb 12, 2024

chapuni Feb 12, 2024

ornata Feb 15, 2024

chapuni Feb 20, 2024

ornata Feb 21, 2024

chapuni Feb 21, 2024

ornata Feb 21, 2024

chapuni Feb 21, 2024

ZhuUx Jun 26, 2024 •

edited

Loading

chapuni Jun 26, 2024

ZhuUx Jun 26, 2024

chapuni Jun 26, 2024

ZhuUx Jun 26, 2024

chapuni Jun 26, 2024

ZhuUx Jun 27, 2024 •

edited

Loading

chapuni Jun 27, 2024

ZhuUx Jun 27, 2024

ZhuUx commented Jun 26, 2024 via email •

edited

Loading

Introduce mcdc::TVIdxBuilder (LLVM side, NFC) #80676

Introduce mcdc::TVIdxBuilder (LLVM side, NFC) #80676

Conversation

chapuni commented Feb 5, 2024 • edited Loading

chapuni commented Feb 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chapuni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZhuUx Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZhuUx Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZhuUx commented Jun 26, 2024 via email • edited Loading

chapuni commented Feb 5, 2024 •

edited

Loading

ZhuUx Jun 26, 2024 •

edited

Loading

ZhuUx Jun 27, 2024 •

edited

Loading

ZhuUx commented Jun 26, 2024 via email •

edited

Loading