Skip to content

Commit 4099ab1

Browse files
committed
coverage: Make expression simplification non-destructive
Instead of modifying the accumulated expressions in-place, we now build a set of expressions that are known to be zero, and then consult that set on the fly when converting the expression data for FFI. This will be necessary when moving mappings and expression data into function coverage info, which can't be mutated during codegen.
1 parent 8efdd4c commit 4099ab1

File tree

1 file changed

+58
-22
lines changed
  • compiler/rustc_codegen_llvm/src/coverageinfo

1 file changed

+58
-22
lines changed

compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs

Lines changed: 58 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -134,8 +134,6 @@ impl<'tcx> FunctionCoverage<'tcx> {
134134
}
135135

136136
pub(crate) fn finalize(&mut self) {
137-
self.simplify_expressions();
138-
139137
// Reorder the collected mappings so that counter mappings are first and
140138
// zero mappings are last, matching the historical order.
141139
self.mappings.sort_by_key(|mapping| match mapping.term {
@@ -145,56 +143,76 @@ impl<'tcx> FunctionCoverage<'tcx> {
145143
});
146144
}
147145

148-
/// Perform some simplifications to make the final coverage mappings
149-
/// slightly smaller.
146+
/// Identify expressions that will always have a value of zero, and note
147+
/// their IDs in [`ZeroExpressions`]. Mappings that refer to a zero expression
148+
/// can instead become mappings to a constant zero value.
150149
///
151150
/// This method mainly exists to preserve the simplifications that were
152151
/// already being performed by the Rust-side expression renumbering, so that
153152
/// the resulting coverage mappings don't get worse.
154-
fn simplify_expressions(&mut self) {
153+
fn identify_zero_expressions(&self) -> ZeroExpressions {
155154
// The set of expressions that either were optimized out entirely, or
156155
// have zero as both of their operands, and will therefore always have
157156
// a value of zero. Other expressions that refer to these as operands
158157
// can have those operands replaced with `CovTerm::Zero`.
159158
let mut zero_expressions = FxIndexSet::default();
160159

161-
// For each expression, perform simplifications based on lower-numbered
162-
// expressions, and then update the set of always-zero expressions if
163-
// necessary.
160+
// Simplify a copy of each expression based on lower-numbered expressions,
161+
// and then update the set of always-zero expressions if necessary.
164162
// (By construction, expressions can only refer to other expressions
165-
// that have lower IDs, so one simplification pass is sufficient.)
166-
for (id, maybe_expression) in self.expressions.iter_enumerated_mut() {
163+
// that have lower IDs, so one pass is sufficient.)
164+
for (id, maybe_expression) in self.expressions.iter_enumerated() {
167165
let Some(expression) = maybe_expression else {
168166
// If an expression is missing, it must have been optimized away,
169167
// so any operand that refers to it can be replaced with zero.
170168
zero_expressions.insert(id);
171169
continue;
172170
};
173171

172+
// We don't need to simplify the actual expression data in the
173+
// expressions list; we can just simplify a temporary copy and then
174+
// use that to update the set of always-zero expressions.
175+
let Expression { mut lhs, op, mut rhs } = *expression;
176+
177+
// If an expression has an operand that is also an expression, the
178+
// operand's ID must be strictly lower. This is what lets us find
179+
// all zero expressions in one pass.
180+
let assert_operand_expression_is_lower = |operand_id: ExpressionId| {
181+
assert!(
182+
operand_id < id,
183+
"Operand {operand_id:?} should be less than {id:?} in {expression:?}",
184+
)
185+
};
186+
174187
// If an operand refers to an expression that is always zero, then
175188
// that operand can be replaced with `CovTerm::Zero`.
176-
let maybe_set_operand_to_zero = |operand: &mut CovTerm| match &*operand {
177-
CovTerm::Expression(id) if zero_expressions.contains(id) => {
178-
*operand = CovTerm::Zero;
189+
let maybe_set_operand_to_zero = |operand: &mut CovTerm| match *operand {
190+
CovTerm::Expression(id) => {
191+
assert_operand_expression_is_lower(id);
192+
if zero_expressions.contains(&id) {
193+
*operand = CovTerm::Zero;
194+
}
179195
}
180196
_ => (),
181197
};
182-
maybe_set_operand_to_zero(&mut expression.lhs);
183-
maybe_set_operand_to_zero(&mut expression.rhs);
198+
maybe_set_operand_to_zero(&mut lhs);
199+
maybe_set_operand_to_zero(&mut rhs);
184200

185201
// Coverage counter values cannot be negative, so if an expression
186202
// involves subtraction from zero, assume that its RHS must also be zero.
187203
// (Do this after simplifications that could set the LHS to zero.)
188-
if let Expression { lhs: CovTerm::Zero, op: Op::Subtract, .. } = expression {
189-
expression.rhs = CovTerm::Zero;
204+
if lhs == CovTerm::Zero && op == Op::Subtract {
205+
rhs = CovTerm::Zero;
190206
}
191207

192208
// After the above simplifications, if both operands are zero, then
193209
// we know that this expression is always zero too.
194-
if let Expression { lhs: CovTerm::Zero, rhs: CovTerm::Zero, .. } = expression {
210+
if lhs == CovTerm::Zero && rhs == CovTerm::Zero {
195211
zero_expressions.insert(id);
196212
}
197213
}
214+
215+
ZeroExpressions(zero_expressions)
198216
}
199217

200218
/// Return the source hash, generated from the HIR node structure, and used to indicate whether
@@ -209,7 +227,9 @@ impl<'tcx> FunctionCoverage<'tcx> {
209227
pub fn get_expressions_and_counter_regions(
210228
&self,
211229
) -> (Vec<CounterExpression>, impl Iterator<Item = (Counter, &CodeRegion)>) {
212-
let counter_expressions = self.counter_expressions();
230+
let zero_expressions = self.identify_zero_expressions();
231+
232+
let counter_expressions = self.counter_expressions(&zero_expressions);
213233
// Expression IDs are indices into `self.expressions`, and on the LLVM
214234
// side they will be treated as indices into `counter_expressions`, so
215235
// the two vectors should correspond 1:1.
@@ -222,12 +242,17 @@ impl<'tcx> FunctionCoverage<'tcx> {
222242

223243
/// Convert this function's coverage expression data into a form that can be
224244
/// passed through FFI to LLVM.
225-
fn counter_expressions(&self) -> Vec<CounterExpression> {
245+
fn counter_expressions(&self, zero_expressions: &ZeroExpressions) -> Vec<CounterExpression> {
226246
// We know that LLVM will optimize out any unused expressions before
227247
// producing the final coverage map, so there's no need to do the same
228248
// thing on the Rust side unless we're confident we can do much better.
229249
// (See `CounterExpressionsMinimizer` in `CoverageMappingWriter.cpp`.)
230250

251+
let counter_from_operand = |operand: CovTerm| match operand {
252+
CovTerm::Expression(id) if zero_expressions.contains(id) => Counter::ZERO,
253+
_ => Counter::from_term(operand),
254+
};
255+
231256
self.expressions
232257
.iter()
233258
.map(|expression| match expression {
@@ -241,12 +266,12 @@ impl<'tcx> FunctionCoverage<'tcx> {
241266
&Some(Expression { lhs, op, rhs, .. }) => {
242267
// Convert the operands and operator as normal.
243268
CounterExpression::new(
244-
Counter::from_term(lhs),
269+
counter_from_operand(lhs),
245270
match op {
246271
Op::Add => ExprKind::Add,
247272
Op::Subtract => ExprKind::Subtract,
248273
},
249-
Counter::from_term(rhs),
274+
counter_from_operand(rhs),
250275
)
251276
}
252277
})
@@ -262,3 +287,14 @@ impl<'tcx> FunctionCoverage<'tcx> {
262287
})
263288
}
264289
}
290+
291+
/// Set of expression IDs that are known to always evaluate to zero.
292+
/// Any mapping or expression operand that refers to these expressions can have
293+
/// that reference replaced with a constant zero value.
294+
struct ZeroExpressions(FxIndexSet<ExpressionId>);
295+
296+
impl ZeroExpressions {
297+
fn contains(&self, id: ExpressionId) -> bool {
298+
self.0.contains(&id)
299+
}
300+
}

0 commit comments

Comments
 (0)