Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor processing of BranchRegions associated with an MCDCDecisionRegion #78819

Closed
wants to merge 1 commit into from

Conversation

evodius96
Copy link
Contributor

This fixes MC/DC issue #77871 in which branches under ExpansionRegions were not being included in the creation of the MC/DC record. The fix is a slight refactor in how branches associated with an MCDCDecisionRegion are gathered such that ExpansionRegions can be scanned recursively.

@evodius96 evodius96 self-assigned this Jan 20, 2024

const auto &RegionsBegin = Record.MappingRegions.begin();
const auto &RegionsEnd = Record.MappingRegions.end();
for (auto It = RegionsBegin; It != RegionsEnd; ++It) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would've preferred to keep the range-based loop as preferred by the LLVM coding standards, but the iterator is useful in being able to scan ahead in the list.

const auto &RegionsEnd = Record.MappingRegions.end();
for (auto It = RegionsBegin; It != RegionsEnd; ++It) {
const auto &Region = *It;

// If an MCDCDecisionRegion is seen, track the BranchRegions that follow
// it according to Region.NumConditions.
if (Region.Kind == CounterMappingRegion::MCDCDecisionRegion) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the handling of DecisionRegions is much cleaner -- being assembled and processed in one iteration rather than rely on successive iterations of the loop.

// Test visualization of MC/DC constructs for branches in macro expansions.

// RUN: llvm-profdata merge %S/Inputs/mcdc-macro.proftext -o %t.profdata
// RUN: llvm-cov show --show-expansions --show-branches=count --show-mcdc %S/Inputs/mcdc-macro.o -instr-profile %t.profdata -path-equivalence=/tmp,%S/Inputs %S/Inputs/mcdc-macro.c | FileCheck %s
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rely on a pre-built object file for now (as I did in a handful of other cases) because it allows for a targeted check. I know these are tedious to update. Based on suggestions from others in the last code reviews, I'm considering changing these tests to be runtime profile tests in the future, removing the need for the object files. However, for now, I think it's useful for this test.

@evodius96
Copy link
Contributor Author

evodius96 commented Jan 22, 2024

On second thought, I will go ahead and rebase with @chapuni's changes from #78918 now to make it easier to test with.

…egion.

This fixes MC/DC issue llvm#77871 in which
branches under ExpansionRegions were not being included in the creation of the
MC/DC record. The fix is a slight refactor in how branches associated with an
MCDCDecisionRegion are gathered such that ExpansionRegions can be scanned
recursively.
chapuni pushed a commit to chapuni/llvm-project that referenced this pull request Jan 28, 2024
@chapuni
Copy link
Contributor

chapuni commented Jan 28, 2024

@evodius96 This also works for me, with #78966.
One of my issues was seen in the middle of my debugging. It produced like;

Decision
Decision
Branch [1,0,2]
Branch [1,0,2]
Branch [2,0,0]
Branch [2,0,0]

I cannot reproduce any more. AFAIK each group of branch is clustering and your impl can handle them as well.

This is shorter however my impl is more descriptive (I think). Both are working to me. Which could we take?

@evodius96
Copy link
Contributor Author

This is shorter however my impl is more descriptive (I think). Both are working to me. Which could we take?

I'm inclined to go with your approach. I don't see any practical downside to it. Thanks!

@evodius96
Copy link
Contributor Author

Closing in deference to #78969

@evodius96 evodius96 closed this Feb 2, 2024
@evodius96 evodius96 deleted the llvm-cov-mcdc-macros branch February 2, 2024 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants