Skip to content

Commit 5998658

Browse files
Avoid updating irrelevant symbols when handling quantifiers (#4268)
Kani currently has a special `handle_quantifiers()` pass after codegen that we need to recursively inline function calls inside quantifier expressions as needed by our hooks. This has to be done for every value in the symbol table, but our current implementation implicitly clones **all `Stmt` type values**, even if they don't contain any quantifiers and wouldn't need to change. This makes the pass currently take up **~7% of codegen on crates that don't even use quantifiers**. Now, the `handle_quantifiers_in_stmt()` function just returns an option with a new `Stmt` if inlining was needed, or `None` otherwise, allowing us to avoid any allocations and even needing to insert back into the table in the common case where a `Stmt` doesn't contain any quantifiers. ### Results Flamegraphing shows that this change reduces the `handle_quantifiers()` pass from 7.1% to just 0.8% of codegen for the `s2n-codec` crate. There was a much less significant change on larger codebases like the standard library, which only saw **a <1% improvement in end to end compile time** (at std commit [177d0fd](model-checking/verify-rust-std@177d0fd), assuming #4257 & #4259 are merged into Kani). That being said, there was a significant **1-7% decrease in codegen time** for individual crates of the standard library, so it seems to still make a difference, even if not reflected in e2e time. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.
1 parent 7897691 commit 5998658

File tree

1 file changed

+38
-17
lines changed
  • kani-compiler/src/codegen_cprover_gotoc/context

1 file changed

+38
-17
lines changed

kani-compiler/src/codegen_cprover_gotoc/context/goto_ctx.rs

Lines changed: 38 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -352,10 +352,10 @@ impl GotocCtx<'_> {
352352
let mut to_modify: BTreeMap<InternedString, SymbolValues> = BTreeMap::new();
353353
let mut suffix_count: u16 = 0;
354354
for (key, symbol) in self.symbol_table.iter() {
355-
if let SymbolValues::Stmt(stmt) = &symbol.value {
356-
let new_stmt_val =
357-
SymbolValues::Stmt(self.handle_quantifiers_in_stmt(stmt, &mut suffix_count));
358-
to_modify.insert(*key, new_stmt_val);
355+
if let SymbolValues::Stmt(stmt) = &symbol.value
356+
&& let Some(new_stmt) = self.handle_quantifiers_in_stmt(stmt, &mut suffix_count)
357+
{
358+
to_modify.insert(*key, SymbolValues::Stmt(new_stmt));
359359
}
360360
}
361361

@@ -366,7 +366,8 @@ impl GotocCtx<'_> {
366366
}
367367

368368
/// Find all quantifier expressions in `stmt` and recursively inline functions.
369-
fn handle_quantifiers_in_stmt(&self, stmt: &Stmt, suffix_count: &mut u16) -> Stmt {
369+
/// Returns a new [Stmt] if something has been changed (e.g. by inlining), or [None] if it should remain the same.
370+
fn handle_quantifiers_in_stmt(&self, stmt: &Stmt, suffix_count: &mut u16) -> Option<Stmt> {
370371
match &stmt.body() {
371372
// According to the hook handling for quantifiers, quantifier expressions must be of form
372373
// lhs = typecast(qex, c_bool)
@@ -432,24 +433,44 @@ impl GotocCtx<'_> {
432433
);
433434
res.cast_to(Type::CInteger(CIntType::Bool))
434435
}
435-
_ => rhs.clone(),
436+
_ => return None,
436437
},
437-
_ => rhs.clone(),
438+
_ => return None,
438439
};
439-
Stmt::assign(lhs.clone(), new_rhs, *stmt.location())
440+
Some(Stmt::assign(lhs.clone(), new_rhs, *stmt.location()))
440441
}
441442
// Recursively find quantifier expressions.
442-
StmtBody::Block(stmts) => Stmt::block(
443-
stmts
444-
.iter()
445-
.map(|stmt| self.handle_quantifiers_in_stmt(stmt, suffix_count))
446-
.collect(),
447-
*stmt.location(),
448-
),
443+
StmtBody::Block(old_stmts) => {
444+
let mut replaced_sub_stmts: FxHashMap<usize, Stmt> = FxHashMap::default();
445+
446+
// For each block, add it and its index to the map if it should be replaced.
447+
for (i, stmt) in old_stmts.iter().enumerate() {
448+
if let Some(new_stmt) = self.handle_quantifiers_in_stmt(stmt, suffix_count) {
449+
replaced_sub_stmts.insert(i, new_stmt);
450+
}
451+
}
452+
453+
if replaced_sub_stmts.is_empty() {
454+
// We can skip doing anything if none of the blocks have to be replaced.
455+
None
456+
} else {
457+
// Take the replacement block if replaced, otherwise just clone the old value.
458+
Some(Stmt::block(
459+
old_stmts
460+
.iter()
461+
.enumerate()
462+
.map(|(i, old_stmt)| {
463+
replaced_sub_stmts.remove(&i).unwrap_or_else(|| old_stmt.clone())
464+
})
465+
.collect(),
466+
*stmt.location(),
467+
))
468+
}
469+
}
449470
StmtBody::Label { label, body } => {
450-
self.handle_quantifiers_in_stmt(body, suffix_count).with_label(*label)
471+
Some(self.handle_quantifiers_in_stmt(body, suffix_count)?.with_label(*label))
451472
}
452-
_ => stmt.clone(),
473+
_ => None,
453474
}
454475
}
455476

0 commit comments

Comments
 (0)