llvm-project (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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().

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 asSmallVector<ConditionIDs>, for TVIdxBuilder to use it before MCDCRecordProcessor(). It was BranchParamsMap or Map as DenseMap<Branch>.
NodeIDs and Fetcher function are deprecated.

// 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;

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>

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.

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.

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?

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.

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.

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

They are my preferences.

@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 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);

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

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.

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 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.

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.

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.

This was referenced

Jun 19, 2024

AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this pull request

Jul 9, 2024

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

bors added a commit to rust-lang-ci/rust that referenced this pull request

Oct 8, 2024