[Coverage] Let `Decision` take account of expansions #78969

chapuni · 2024-01-22T13:09:41Z

The current implementation (D138849) assumes Branch(es) would follow after the corresponding Decision. It is not true if Branch(es) are forwarded to expanded file ID. As a result, consecutive Decision(s) would be confused with insufficient number of Branch(es).

Expansion will point Branch(es) in other file IDs if Expansion is included in the range of Decision.

Fixes #77871

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

evodius96 · 2024-01-23T00:34:27Z

I'd be OK going this direction as opposed to #78819, since it doesn't have to scan ahead to add branch regions, but it does add new complexity to the tracking. Can you also post your tests?

chapuni

I have rewritten DecisionRow and added comments.

Tests are not ready. Please be patient.

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

ornata · 2024-01-23T13:50:04Z

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

+      : DecisionRegion(&Decision), DecisionStartLoc(Decision.startLoc()),
+        DecisionEndLoc(Decision.endLoc()) {}
+
+  /// Determine whether `R` is included in `DecisionRegion`.


Nit:

Use \p or \param in Doxygen. (http://www.doxygen.jp/commands.html#cmdp)

Backticks unnecessary in Doxygen.

E.g.

\returns true if \p R is included in DecisionRegion

I haven't written doxygen comments for long time. Please point out any mistakes.

Doxygen comments are not required. I think much new code doesn't use Doxygen...

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

ornata · 2024-01-23T14:07:47Z

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

+    });
+  }
+
+  enum class UpdateResult {


This should be documented

I didn't do since I thought the enums are descriptive.

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

chapuni

TODO:

Revise comments in the latter half
Construct tests

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

chapuni · 2024-01-23T21:15:42Z

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

+    });
+  }
+
+  enum class UpdateResult {


I didn't do since I thought the enums are descriptive.

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

ornata · 2024-01-24T01:51:57Z

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

-      MCDCDecision = &Region;
-      NumConds = Region.MCDCParams.NumConditions;
+      // Start recording `Region` as the `Decision`
+      Decisions.emplace_back(Region);


why emplace_back instead of push_back?

I thought it would be made smaller if the constructor has default constructors for SmallVector.

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

MaskRay · 2024-01-28T05:12:21Z

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

-      NumConds = Region.MCDCParams.NumConditions;
+    // MCDCDecisionRegion should be handled first since it overlaps with
+    // others inside.
+    if (MCDCDecisions.registerDecision(Region))


I think it reads better if the CounterMappingRegion::MCDCDecisionRegion guard is moved here. The same applies to MCDCDecisions.processRegion(Region);

I.e. we check different types of Region here and dispatch actions to MCDCDecisions.

I did because of symmetry to processRegion. I can rework this.

MaskRay · 2024-01-28T05:15:49Z

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

+  processRegion(const CounterMappingRegion &Region) {
+
+    // Record ExpansionRegion.
+    if (Region.Kind == CounterMappingRegion::ExpansionRegion) {


I think it's clearer to move ExpansionRegion handling to the caller.

This class then doesn't need to check Region.Kind

I wanted to make MCDCDecisionRecorder richer, since it simply encapsulated SmallVector<DecisionRecord> Decisions.

llvm/test/tools/llvm-cov/mcdc-macro.test

MaskRay · 2024-01-29T23:07:38Z

The description is too brief for the involved problem:) Feel free to incorporate my comment #77871 (comment) into the description/commit message.

whentojump · 2024-01-31T07:18:13Z

Hey everyone. Thanks for this great work!

Will you please add this test:

#define C c
#define D 1
#define E (C != a) && (C > a)
#define F E
#define G(x) x

void __attribute__((noinline)) func1(void) { return; }

void __attribute__((noinline)) func(int a, int b, int c) {
  if (a && D && E || b)
    func1();
  if (b && D)
    func1();
  if (a && (b && C) || (D && F))
    func1();
  if (G(a) && G(b))
    func1();
}

int main() {
  func(2, 3, 3);
  return 0;
}

Currently it doesn't seem to work on my side. I'm also glad to investigate that.

Revise: I further minimize the problematic G. As a bit of context, my use case is code like this. (See also one of my immature PR here)

MaskRay · 2024-01-31T07:59:44Z

I think this is correct, but the consumer side complexity makes me wonder whether the additional complexity makes our "boring algorithm" overall worse than GCC https://gcc.gnu.org/pipermail/gcc-patches/2024-January/644269.html

(From https://maskray.me/blog/2024-01-28-mc-dc-and-compiler-implementations)

Pros:

Easier to understand
Each condition instrumentation adds just one single bitwise OR instruction, instead of possibly three (one bitwise AND plus two bitwise OR).

Cons:

More bits to encode N conditions (2**N vs. 2*N)
More metadata to encode the reduced ordered BDDs, required by the reader to compute independence pairs
Determining independent pairs involves a brute-force algorithm in llvm-cov, which has a high time complexity but probably acceptable due to the limited condition count.

chapuni · 2024-01-31T08:52:41Z

@whentojump Could you try #78966, too?

@MaskRay

I think this is correct,

What about "this"?

evodius96 · 2024-01-31T14:45:28Z

I think this is correct, but the consumer side complexity makes me wonder whether the additional complexity makes our "boring algorithm" overall worse than GCC

From the perspective of embedded, I think it makes more sense to keep the instrumentation light and off-load processing in llvm-cov. Also, I think we can look at better modularization of metadata sections (I.e. keeping them in the executable but out of what is loaded in memory, which is what we do downstream and was discussed at the last Developers' meeting embedded workshop). Even going beyond six conditions isn't that bad, but I don't think 32 conditions (as mentioned in GCC context) is at all typical, especially in embedded context. I know we want this to work outside of embedded as well, but we also have to keep in mind when MC/DC is not only useful but practical to fulfill.

Conceivably (just thinking out loud) we may also be able to enable different modes that balance the tradeoffs differently between the different algorithms.

whentojump · 2024-01-31T19:09:09Z

@whentojump Could you try #78966, too?

These two PRs combined worked smoothly for my case. Thanks a lot!

chapuni · 2024-02-01T08:38:19Z

Thanks @MaskRay . I know I have to do;

Rewrite the description (and possibly the subject)
Regenerate and test the testcase.o

By the way, do you have any comments in #78966?

evodius96 · 2024-02-02T14:34:10Z

@chapuni Can you backport this onto 18.x branch along with #78966?

The current implementation (D138849) assumes `Branch`(es) would follow after the corresponding `Decision`. It is not true if `Branch`(es) are forwarded to expanded file ID. As a result, consecutive `Decision`(s) would be confused with insufficient number of `Branch`(es). `Expansion` will point `Branch`(es) in other file IDs if `Expansion` is included in the range of `Decision`. Fixes llvm#77871 --------- Co-authored-by: Alan Phipps <a-phipps@ti.com> (cherry picked from commit d912f1f)

The current implementation (D138849) assumes `Branch`(es) would follow after the corresponding `Decision`. It is not true if `Branch`(es) are forwarded to expanded file ID. As a result, consecutive `Decision`(s) would be confused with insufficient number of `Branch`(es). `Expansion` will point `Branch`(es) in other file IDs if `Expansion` is included in the range of `Decision`. Fixes llvm#77871 --------- Co-authored-by: Alan Phipps <a-phipps@ti.com>

The current implementation (D138849) assumes `Branch`(es) would follow after the corresponding `Decision`. It is not true if `Branch`(es) are forwarded to expanded file ID. As a result, consecutive `Decision`(s) would be confused with insufficient number of `Branch`(es). `Expansion` will point `Branch`(es) in other file IDs if `Expansion` is included in the range of `Decision`. Fixes llvm#77871 --------- Co-authored-by: Alan Phipps <a-phipps@ti.com> (cherry picked from commit d912f1f)

chapuni requested a review from ornata January 22, 2024 13:09

ornata reviewed Jan 22, 2024

View reviewed changes

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

ornata reviewed Jan 22, 2024

View reviewed changes

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

ornata reviewed Jan 22, 2024

View reviewed changes

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

ornata reviewed Jan 22, 2024

View reviewed changes

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

ornata reviewed Jan 22, 2024

View reviewed changes

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

ornata reviewed Jan 22, 2024

View reviewed changes

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

ornata reviewed Jan 22, 2024

View reviewed changes

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

ornata reviewed Jan 22, 2024

View reviewed changes

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

ornata reviewed Jan 22, 2024

View reviewed changes

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

chapuni added 4 commits January 23, 2024 14:26

[Coverage] Rework Decision/Expansion/Branch (llvm#77871)

5d9de08

Confirm that all Decisions have not been resolved.

1564424

DecisionRow: Rework to reflect reviews

f74dec7

Add comments to the upper half.

425185c

chapuni force-pushed the mcdc/expansion77871 branch from 9f97b0b to 425185c Compare January 23, 2024 12:47

chapuni commented Jan 23, 2024

View reviewed changes

ornata reviewed Jan 23, 2024

View reviewed changes

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

ornata reviewed Jan 23, 2024

View reviewed changes

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

ornata reviewed Jan 23, 2024

View reviewed changes

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

ornata reviewed Jan 23, 2024

View reviewed changes

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

chapuni requested review from MaskRay and evodius96 January 23, 2024 21:07

chapuni commented Jan 23, 2024

View reviewed changes

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

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

ornata reviewed Jan 24, 2024

View reviewed changes

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

ornata reviewed Jan 24, 2024

View reviewed changes

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

ornata reviewed Jan 24, 2024

View reviewed changes

MaskRay reviewed Jan 28, 2024

View reviewed changes

chapuni added 2 commits January 29, 2024 00:05

Reflect @MaskRay's suggestions

200a32f

Mark dominates() const

0e4b05b

evodius96 reviewed Jan 29, 2024

View reviewed changes

llvm/test/tools/llvm-cov/mcdc-macro.test Show resolved Hide resolved

MaskRay approved these changes Feb 1, 2024

View reviewed changes

Update mcdc-macro.test

c889c04

chapuni changed the title ~~[Coverage] Rework Decision/Expansion/Branch~~ [Coverage] Let Decision take account of expansions Feb 2, 2024

chapuni merged commit d912f1f into llvm:main Feb 2, 2024
4 checks passed

chapuni deleted the mcdc/expansion77871 branch February 2, 2024 11:34

evodius96 mentioned this pull request Feb 2, 2024

Refactor processing of BranchRegions associated with an MCDCDecisionRegion #78819

Closed

pointhex mentioned this pull request May 7, 2024

getStyleDiagHandler #91314

Closed

aemerson mentioned this pull request May 9, 2024

release/18.x: [AArc64][GlobalISel] Fix legalizer assert for G_INSERT_VECTOR_ELT - manual merge #91672

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Coverage] Let `Decision` take account of expansions #78969

[Coverage] Let `Decision` take account of expansions #78969

chapuni commented Jan 22, 2024 •

edited

Loading

evodius96 commented Jan 23, 2024

chapuni left a comment

ornata Jan 23, 2024

chapuni Jan 25, 2024

MaskRay Jan 28, 2024

ornata Jan 23, 2024

chapuni Jan 23, 2024

chapuni left a comment

chapuni Jan 23, 2024

ornata Jan 24, 2024

chapuni Jan 25, 2024

MaskRay Jan 28, 2024

chapuni Jan 28, 2024

MaskRay Jan 28, 2024 •

edited

Loading

chapuni Jan 28, 2024

MaskRay commented Jan 29, 2024

whentojump commented Jan 31, 2024 •

edited

Loading

MaskRay commented Jan 31, 2024

chapuni commented Jan 31, 2024 •

edited

Loading

evodius96 commented Jan 31, 2024 •

edited

Loading

whentojump commented Jan 31, 2024

chapuni commented Feb 1, 2024

evodius96 commented Feb 2, 2024

[Coverage] Let Decision take account of expansions #78969

[Coverage] Let Decision take account of expansions #78969

Conversation

chapuni commented Jan 22, 2024 • edited Loading

evodius96 commented Jan 23, 2024

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

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

MaskRay Jan 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaskRay commented Jan 29, 2024

whentojump commented Jan 31, 2024 • edited Loading

MaskRay commented Jan 31, 2024

chapuni commented Jan 31, 2024 • edited Loading

evodius96 commented Jan 31, 2024 • edited Loading

whentojump commented Jan 31, 2024

chapuni commented Feb 1, 2024

evodius96 commented Feb 2, 2024

[Coverage] Let `Decision` take account of expansions #78969

[Coverage] Let `Decision` take account of expansions #78969

chapuni commented Jan 22, 2024 •

edited

Loading

MaskRay Jan 28, 2024 •

edited

Loading

whentojump commented Jan 31, 2024 •

edited

Loading

chapuni commented Jan 31, 2024 •

edited

Loading

evodius96 commented Jan 31, 2024 •

edited

Loading