coverage: Replace `ExpressionOperandId` with enum `Operand` #113428

Zalathar · 2023-07-07T03:08:58Z

This is one step in my larger coverage refactoring ambitions described at rust-lang/compiler-team#645.

LLVM coverage has a concept of “mapping expressions” that allow a span's execution count to be computed as a simple arithmetic expression over other counters/expressions, instead of requiring a dedicated physical counter for every control-flow branch.

These expressions have an operator (+ or -) and two operands. Operands are currently represented as ExpressionOperandId, which wraps a u32 with the following semantics:

0 represents a special counter that always has a value of zero
Values ascending from 1 represent counter IDs
Values descending from u32::MAX represent the IDs of other expressions

This change replaces that whole ExpressionOperandId scheme with a simple enum that explicitly distinguishes between the three cases.

This lets us remove a lot of fiddly code for dealing with the different operand kinds:

Previously it was only possible to distinguish between counter-ID operands and expression-ID operands by comparing the operand ID with the total number of counters in a function. This is unnecessary now that the enum distinguishes them explicitly.
There's no need for expression IDs to descend from u32::MAX and then get translated into zero-based indices in certain places. Now that they ascend from zero, they can be used as indices directly.
There's no need to reserve ID number 0 for the special zero operand, since it can just have its own variant in the enum, so counter IDs can count up from 0.

(Making counter IDs ascend from 0 also lets us fix an off-by-one error in the query for counting the total number of counters, which would cause LLVM to emit an extra unused counter for every instrumented function.)

This PR may be easiest to review as individual patches, since that breaks it up into clearly distinct parts:

Replace a u32 wrapper with an explicit enum, without changing the semantics of the underlying IDs being stored.
Change the numbering scheme used by Operand::Expression to make expression IDs ascend from 0 (instead of descending from u32::MAX).
Change the numbering scheme used by Operand::Counter to make counter IDs ascend from 0 (instead of ascending from 1).

rustbot · 2023-07-07T03:09:06Z

r? @jackh726

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-07-07T03:09:08Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Zalathar · 2023-07-07T03:09:30Z

@rustbot label +A-code-coverage

jackh726 · 2023-07-19T00:54:28Z

r? compiler

Zalathar · 2023-07-19T01:58:57Z

(I've been rebasing this periodically because I have local changes that build on top of it, but the actual changes in this PR should be stable/complete.)

compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs

compiler-errors · 2023-07-26T19:38:01Z

i'm not really qualified to review this

r? compiler

apiraino · 2023-07-27T07:11:20Z

Nominating to signal boost this PR and help find a reviewer with more context

@rustbot label +I-compiler-nominated

Zalathar · 2023-07-27T09:30:00Z

I’m happy to help guide someone through this as necessary.

It’s not actually doing anything fundamentally complicated, since it’s mostly just replacing a u32 with an enum and then tidying up afterwards.

The actual motivation here is to prevent `rustfmt` from suddenly reformatting these enum variants onto a single line, when they become slightly shorter in the future. But there's no harm in adding some helpful documentation at the same time.

Because the three kinds of operand are now distinguished explicitly, we no longer need fiddly code to disambiguate counter IDs and expression IDs based on the total number of counters/expressions in a function. This does increase the size of operands from 4 bytes to 8 bytes, but that shouldn't be a big deal since they are mostly stored inside boxed structures, and the current coverage code is not particularly size-optimized anyway.

Operand types are now tracked explicitly, so there is no need for expression IDs to avoid counter IDs by descending from `u32::MAX`. Instead they can just count up from 0, and can be used directly as indices when necessary.

Operand types are now tracked explicitly, so there is no need to reserve ID 0 for the special always-zero counter. As part of the renumbering, this change fixes an off-by-one error in the way counters were counted by the `coverageinfo` query. As a result, functions should now have exactly the number of counters they actually need, instead of always having an extra counter that is never used.

davidtwco

This is great, I've left one nitpick comment which isn't a big deal so I'll just go ahead and approve this.

davidtwco · 2023-08-01T08:29:56Z

compiler/rustc_middle/src/mir/coverage.rs

+/// expression. Counter/expression operands are referred to by ID.
+#[derive(Copy, Clone, PartialEq, Eq)]
+#[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)]
+pub enum Operand {


nit: maybe we could rename this to CoverageOperand (or something like that), just because mir::Operand is quite common and it could be easy to get mixed up when reading the coverage code for the first time.

Ah, I haven't interacted directly with actual MIR very much, so this didn't occur to me.

I have a bunch of other changes stacked up behind this one that use Operand, so to avoid churning those I definitely won't be renaming it in the near future. But it's something to bear in mind once my current coverage work has settled down.

davidtwco · 2023-08-01T08:36:02Z

@bors r+

bors · 2023-08-01T08:36:04Z

📌 Commit 3920e07 has been approved by davidtwco

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#100455 (Implement RefUnwindSafe for Backtrace) - rust-lang#113428 (coverage: Replace `ExpressionOperandId` with enum `Operand`) - rust-lang#114283 (Use parking lot's rwlock even without parallel-rustc) - rust-lang#114288 (Improve diagnostic for wrong borrow on binary operations) - rust-lang#114296 (interpret: fix alignment handling for Repeat expressions) - rust-lang#114306 ([rustc_data_structures][perf] Simplify base_n::push_str.) - rust-lang#114320 (Cover statements for stable_mir) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned jackh726 Jul 7, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 7, 2023

rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Jul 7, 2023

Zalathar mentioned this pull request Jul 7, 2023

Changes to StatementKind::Coverage rust-lang/compiler-team#645

Closed

3 tasks

Zalathar force-pushed the operand branch 5 times, most recently from d21eb94 to cf38b8d Compare July 13, 2023 01:37

Zalathar changed the title ~~Replace coverage ExpressionOperandId with enum Operand~~ coverage: Replace ExpressionOperandId with enum Operand Jul 13, 2023

Zalathar force-pushed the operand branch 2 times, most recently from 787b627 to 5c4dd5b Compare July 16, 2023 03:48

rustbot assigned compiler-errors and unassigned jackh726 Jul 19, 2023

Zalathar force-pushed the operand branch from 5c4dd5b to 58d651c Compare July 19, 2023 01:56

Zalathar force-pushed the operand branch 4 times, most recently from 0620d11 to b6c0982 Compare July 23, 2023 00:34

Zalathar commented Jul 23, 2023

View reviewed changes

compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs Show resolved Hide resolved

Zalathar force-pushed the operand branch 3 times, most recently from 766b854 to 088d7b0 Compare July 26, 2023 03:16

rustbot assigned davidtwco and unassigned compiler-errors Jul 26, 2023

rustbot added the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Jul 27, 2023

Zalathar force-pushed the operand branch 5 times, most recently from dde3122 to d816611 Compare July 30, 2023 23:31

Zalathar added 4 commits August 1, 2023 11:29

Add some line comments to enum CoverageKind

5a808d4

The actual motivation here is to prevent `rustfmt` from suddenly reformatting these enum variants onto a single line, when they become slightly shorter in the future. But there's no harm in adding some helpful documentation at the same time.

Zalathar force-pushed the operand branch from d816611 to 3920e07 Compare August 1, 2023 01:36

davidtwco approved these changes Aug 1, 2023

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 1, 2023

matthiaskrgr mentioned this pull request Aug 1, 2023

Rollup of 7 pull requests #114331

Merged

bors merged commit 52bfceb into rust-lang:master Aug 1, 2023
11 checks passed

rustbot added this to the 1.73.0 milestone Aug 1, 2023

Zalathar deleted the operand branch August 1, 2023 23:52

apiraino removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Aug 18, 2023

Zalathar mentioned this pull request Sep 6, 2023

coverage: Simplify the coverageinfo query #115586

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

coverage: Replace `ExpressionOperandId` with enum `Operand` #113428

coverage: Replace `ExpressionOperandId` with enum `Operand` #113428

Zalathar commented Jul 7, 2023 •

edited

Loading

rustbot commented Jul 7, 2023

rustbot commented Jul 7, 2023

Zalathar commented Jul 7, 2023

jackh726 commented Jul 19, 2023

Zalathar commented Jul 19, 2023

compiler-errors commented Jul 26, 2023

apiraino commented Jul 27, 2023 •

edited

Loading

Zalathar commented Jul 27, 2023

davidtwco left a comment

davidtwco Aug 1, 2023

Zalathar Aug 1, 2023

davidtwco commented Aug 1, 2023

bors commented Aug 1, 2023

coverage: Replace ExpressionOperandId with enum Operand #113428

coverage: Replace ExpressionOperandId with enum Operand #113428

Conversation

Zalathar commented Jul 7, 2023 • edited Loading

rustbot commented Jul 7, 2023

rustbot commented Jul 7, 2023

Zalathar commented Jul 7, 2023

jackh726 commented Jul 19, 2023

Zalathar commented Jul 19, 2023

compiler-errors commented Jul 26, 2023

apiraino commented Jul 27, 2023 • edited Loading

Zalathar commented Jul 27, 2023

davidtwco left a comment

Choose a reason for hiding this comment

davidtwco Aug 1, 2023

Choose a reason for hiding this comment

Zalathar Aug 1, 2023

Choose a reason for hiding this comment

davidtwco commented Aug 1, 2023

bors commented Aug 1, 2023

coverage: Replace `ExpressionOperandId` with enum `Operand` #113428

coverage: Replace `ExpressionOperandId` with enum `Operand` #113428

Zalathar commented Jul 7, 2023 •

edited

Loading

apiraino commented Jul 27, 2023 •

edited

Loading