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

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

Merged
merged 48 commits into from
Jun 13, 2024

Conversation

chapuni
Copy link
Contributor

@chapuni chapuni commented Feb 21, 2024

By storing possible test vectors instead of combinations of conditions, the restriction is dramatically relaxed.

This introduces two options to cc1:

  • -fmcdc-max-conditions=32767
  • -fmcdc-max-test-vectors=2147483646

This change makes coverage mapping, profraw, and profdata incompatible with Clang-18.

  • Bitmap semantics changed. It is incompatible with previous format.
  • BitmapIdx in Decision points to the end of the bitmap.
  • Bitmap is packed per function.
  • llvm-cov can understand profdata generated by llvm-profdata-18.

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

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
* 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.
@@ -983,7 +979,7 @@ void CodeGenPGO::mapRegionCounters(const Decl *D) {
// for most embedded applications. Setting a maximum value prevents the
// bitmap footprint from growing too large without the user's knowledge. In
// the future, this value could be adjusted with a command-line option.
unsigned MCDCMaxConditions = (CGM.getCodeGenOpts().MCDCCoverage) ? 6 : 0;
unsigned MCDCMaxConditions = (CGM.getCodeGenOpts().MCDCCoverage) ? 32767 : 0;
Copy link

Choose a reason for hiding this comment

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

I think the limit should be controlled by a flag, with a reasonable default value.

E.g.

-mcdc-max-conditions=<value>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MaxCond is one of options. I guess it could be configurable just for theirs coding rules, since I think controlling MaxCond would not make sense.
I know we have to introduce other options as well. I will do later.

  • MaxCond
  • MaxTVs to restrict per-expression number of TVs
  • "Max bytes of bitmap in the CU" to restict inter-function size of the bitmap.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know you will do this later, but I also wanted to echo the desire for a means to control the condition limit to warn the user against unintended memory footprint expansion, most useful in embedded development contexts. It may not otherwise be obvious to most users what is contributing to larger memory usage. We could also override the default in downstream toolchains.

Copy link

Choose a reason for hiding this comment

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

Also why 32767? Max signed 16-bit value? Is there a reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@evodius96 I've introduced -fmcdc-max-conditions=32767.

against unintended memory footprint expansion
I think -fmcdc-max-test-vectors=64 can restrict max bitmap size as the current implementation.

@ornata I have no idea how to determine practical limit for that. So I use SHRT_MAX.

clang/lib/CodeGen/CoverageMappingGen.cpp Outdated Show resolved Hide resolved
llvm/lib/ProfileData/Coverage/CoverageMapping.cpp Outdated Show resolved Hide resolved
llvm/lib/ProfileData/Coverage/CoverageMapping.cpp Outdated Show resolved Hide resolved
llvm/lib/ProfileData/Coverage/CoverageMapping.cpp Outdated Show resolved Hide resolved
clang/lib/CodeGen/CoverageMappingGen.cpp Outdated Show resolved Hide resolved
Copy link
Contributor Author

@chapuni chapuni left a comment

Choose a reason for hiding this comment

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

Thanks for your comments and sorry for my hasty w-i-p work . I know a few rough implementations would be there.

clang/lib/CodeGen/CoverageMappingGen.cpp Outdated Show resolved Hide resolved
clang/lib/CodeGen/CoverageMappingGen.cpp Outdated Show resolved Hide resolved
@chapuni
Copy link
Contributor Author

chapuni commented Feb 21, 2024

@hanickadot Your comments to llvm/CoverageMapping.cpp are not for this PR. See #80676 .

They are my preferences.

@hanickadot
Copy link
Contributor

@hanickadot Your comments to llvm/CoverageMapping.cpp are not for this PR. See #80676 .

They are my preferences.

Didn't know the context, I saw it green as added. Just ignore it.

@chapuni chapuni changed the base branch from main to users/chapuni/mcdc/tvidx February 21, 2024 13:48
mcdc::TVIdxBuilder Builder(CondIDs);
unsigned NumTVs = Builder.NumTestVectors;
unsigned MaxTVs = CVM.getCodeGenModule().getCodeGenOpts().MCDCMaxTVs;
assert(MaxTVs < mcdc::TVIdxBuilder::HardMaxTVs);
Copy link
Contributor

Choose a reason for hiding this comment

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

should it be <= or <?

I'm asking because MaxTVs seems like a count, not id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HardMaxTVs(int_max) is actually the error code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Than I would probably make it !=

@chapuni chapuni merged commit 7ead2d8 into llvm:main Jun 13, 2024
5 of 8 checks passed
chapuni added a commit to chapuni/llvm-project that referenced this pull request Jun 13, 2024
3rd arg of `tvbitmap.update` was made unused. Remove 3rd arg.

Sweep `condbitmap.update`, since it is no longer used.
@chapuni chapuni deleted the mcdc/clangtvidx branch June 13, 2024 12:34
@zmodem
Copy link
Collaborator

zmodem commented Jun 14, 2024

This broke the lit tests on mac: https://green.lab.llvm.org/job/llvm.org/job/clang-stage1-RA/1096/

I'll revert to green for now.

zmodem added a commit that referenced this pull request Jun 14, 2024
This broke the lit tests on Mac:
https://green.lab.llvm.org/job/llvm.org/job/clang-stage1-RA/1096/

> By storing possible test vectors instead of combinations of conditions,
> the restriction is dramatically relaxed.
>
> This introduces two options to `cc1`:
>
> * `-fmcdc-max-conditions=32767`
> * `-fmcdc-max-test-vectors=2147483646`
>
> This change makes coverage mapping, profraw, and profdata incompatible
> with Clang-18.
>
> - Bitmap semantics changed. It is incompatible with previous format.
> - `BitmapIdx` in `Decision` points to the end of the bitmap.
> - Bitmap is packed per function.
> - `llvm-cov` can understand `profdata` generated by `llvm-profdata-18`.
>
> RFC:
> https://discourse.llvm.org/t/rfc-coverage-new-algorithm-and-file-format-for-mc-dc/76798

This reverts commit 7ead2d8.
@chapuni
Copy link
Contributor Author

chapuni commented Jun 14, 2024

Oh, it's apple-specific continuous mode test :(

chapuni added a commit that referenced this pull request Jun 14, 2024
By storing possible test vectors instead of combinations of conditions,
the restriction is dramatically relaxed.

This introduces two options to `cc1`:

* `-fmcdc-max-conditions=32767`
* `-fmcdc-max-test-vectors=2147483646`

This change makes coverage mapping, profraw, and profdata incompatible
with Clang-18.

- Bitmap semantics changed. It is incompatible with previous format.
- `BitmapIdx` in `Decision` points to the end of the bitmap.
- Bitmap is packed per function.
- `llvm-cov` can understand `profdata` generated by `llvm-profdata-18`.

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

--
Change(s) since llvmorg-19-init-14288-g7ead2d8c7e91

- Update compiler-rt/test/profile/ContinuousSyncMode/image-with-mcdc.c
chapuni added a commit that referenced this pull request Jun 16, 2024
3rd arg of `tvbitmap.update` was made unused. Remove 3rd arg.

Sweep `condbitmap.update`, since it is no longer used.
chapuni added a commit that referenced this pull request Jun 18, 2024
Mostly apparent changes (#82448, #95496) are described here.
AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this pull request Jul 9, 2024
Mostly apparent changes (llvm#82448, llvm#95496) are described here.
EthanLuisMcDonough pushed a commit to EthanLuisMcDonough/llvm-project that referenced this pull request Aug 13, 2024
By storing possible test vectors instead of combinations of conditions,
the restriction is dramatically relaxed.

This introduces two options to `cc1`:

* `-fmcdc-max-conditions=32767`
* `-fmcdc-max-test-vectors=2147483646`

This change makes coverage mapping, profraw, and profdata incompatible
with Clang-18.

- Bitmap semantics changed. It is incompatible with previous format.
- `BitmapIdx` in `Decision` points to the end of the bitmap.
- Bitmap is packed per function.
- `llvm-cov` can understand `profdata` generated by `llvm-profdata-18`.

RFC:
https://discourse.llvm.org/t/rfc-coverage-new-algorithm-and-file-format-for-mc-dc/76798
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 7, 2024
[Coverage][MCDC] Adapt mcdc to llvm 19

Related issue: rust-lang#126672

Also finish task 4 at rust-lang#124144

[llvm rust-lang#82448](llvm/llvm-project#82448) has introduced some break changes into mcdc, causing incompatibility between llvm 18 and 19. This draft adapts to that change and gives up supporting for llvm-18.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 8, 2024
[Coverage][MCDC] Adapt mcdc to llvm 19

Related issue: rust-lang#126672

Also finish task 4 at rust-lang#124144

[llvm rust-lang#82448](llvm/llvm-project#82448) has introduced some break changes into mcdc, causing incompatibility between llvm 18 and 19. This draft adapts to that change and gives up supporting for llvm-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.

6 participants