Skip to content

Commit

Permalink
Fixing Inline Elision (Tests) (#23)
Browse files Browse the repository at this point in the history
Some of the inline elision tests fail.

```
test no_elision_without_output ... FAILED
test no_elision_without_input ... FAILED
test basic_elision_mut ... ok
test basic_elision ... ok
test connection_precision_self ... ok
test unelision ... ok
test connection_precision_args ... ok
```

Even more mysterious: If I disable elision (not pass the
`--inline-elision` argument), only three of them fail.

```
test no_elision_without_input ... FAILED
test unelision ... ok
test connection_precision_self ... FAILED
test no_elision_without_output ... FAILED
test connection_precision_args ... ok
test basic_elision ... ok
test basic_elision_mut ... ok
```

Maybe this is mean to happen, but need to investigate. Already found one
niche (but very dumb) error in the algebra.

---

So here is what happened.

1. My merge broke things a bit and the flag `--inline-elision` was no
longer being heeded. Inline elision would simply always be performed.
Worse still it would also forget to check if the function carries a
marker in the inliner. This might not actually have manifested
negatively but it feels dangerous at least.
2. The tests were actually wrong. Whether or not inline elision happens,
the (elided) function is gone from the graph whether its body is inlined
*or* elided. However the test cases were checking for exactly that,
whether the function occurs in the graph. I changed them to just call
`std::convert::identity` (an external function and therefore neither
inlined, nor elided) inside and then check if *that* function is in the
graph.

Now the test cases predictably succeed when `--inline-elision` is active
and fail if not.
  • Loading branch information
JustusAdam authored Jul 28, 2023
1 parent 10cf23c commit 31b5991
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 31 deletions.
1 change: 1 addition & 0 deletions .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ jobs:
cargo test --test control_flow_tests
cargo test --test new_alias_analysis_tests
cargo test --test async_tests
cargo test --test inline_elision_tests
intergration-tests:
name: Integration Tests
Expand Down
44 changes: 23 additions & 21 deletions src/ana/algebra.rs
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,7 @@ impl<B, F: Copy> Equality<B, F> {
self.rhs
.terms
.extend(self.lhs.terms.drain(..).rev().map(Operator::flip));
assert!(self.lhs.terms.is_empty());
}

/// Swap the left and right hand side terms
Expand Down Expand Up @@ -376,10 +377,12 @@ pub mod graph {
for eq in equations {
let mut eq: Equality<_, _> = eq.borrow().clone();
eq.rearrange_left_to_right();
let from = *eq.rhs.base();
let to = *eq.lhs.base();
debug!(
"Adding {} -> {} {} ({})",
eq.rhs.base(),
eq.lhs.base(),
to,
from,
Print(|fmt| {
fmt.write_char('[')?;
write_sep(fmt, ", ", eq.rhs.terms.iter(), Display::fmt)?;
Expand All @@ -391,14 +394,10 @@ pub mod graph {
fmt.write_char(']')
}),
);
if let Some(w) = graph.edge_weight_mut(*eq.lhs.base(), *eq.rhs.base()) {
if let Some(w) = graph.edge_weight_mut(from, to) {
w.0.push(eq.rhs.terms)
} else {
graph.add_edge(
*eq.rhs.base(),
*eq.lhs.base(),
Operators(SmallVec::from_iter([eq.rhs.terms])),
);
graph.add_edge(from, to, Operators(SmallVec::from_iter([eq.rhs.terms])));
}
}
graph
Expand Down Expand Up @@ -478,21 +477,24 @@ pub mod graph {
graph,
&[],
&|_, _| "".to_string(),
&|_, (n, _)| if seen.contains(graph.to_index(n)) {
"shape=box,color=blue".to_string()
} else if n == to {
"shape=box,color=red".to_string()
} else if is_target(n) {
"shape=box,color=green".to_string()
} else if n == from {
"shape=box,color=aqua".to_string()
} else {
"shape=box".to_string()
}
&|_, (n, _)| "shape=box,color=".to_string()
+ if n == to {
"red"
} else if is_target(n) {
"green"
} else if n == from {
"yellow"
} else if n == node {
"purple"
} else if seen.contains(graph.to_index(n)) {
"blue"
} else {
"black"
}
)
)
.unwrap();
panic!("Encountered invalid operator combination {one} {two} in {projections}: as op chain {}. The state of the search on the operator graph at the time the error as found has been dumped to {path}.", Print(|fmt| {
panic!("Encountered invalid operator combination {one} {two} in {projections}: as op chain {}.\n The state of the search on the operator graph at the time the error as found has been dumped to {path}.\n Yellow is where the search started,\n blue nodes were seen during the search,\n the target is green,\n the red node is the one we were trying to reach and\n the purple node is where we tried to reach it from.", Print(|fmt| {
fmt.write_char('[')?;
write_sep(fmt, ", ", projections.terms.iter(), Display::fmt)?;
fmt.write_char(']')
Expand Down Expand Up @@ -1020,7 +1022,7 @@ impl<'tcx> mir::visit::Visitor<'tcx> for Extractor<'tcx> {
[op1, op2]
.into_iter()
.flat_map(|op| op.place().into_iter())
.map(|op| op.into()),
.map(|op| MirTerm::from(op).add_contains_at(crate::rustc_abi::FieldIdx::from(0_usize).into())),
)
as Box<_>,
Aggregate(box kind, ops) => match kind {
Expand Down
14 changes: 7 additions & 7 deletions src/ana/inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1076,18 +1076,18 @@ impl<'tcx, 'g, 's> Inliner<'tcx, 'g, 's> {
})
.filter_map(|(id, location, function)| {
if recursive_analysis_enabled {
match function.as_local() {
Some(local_id) if self.oracle.should_inline(local_id) => {
debug!("Inlining {function:?}");
return Some((id, *location, InlineAction::SimpleInline(local_id)));
}
_ => (),
}
if let Some(ac) =
self.classify_special_function_handling(*function, id, &i_graph.graph)
{
return Some((id, *location, InlineAction::Drop(ac)));
}
if let Some(local_id) = function.as_local()
&& self.oracle.should_inline(local_id)
&& (!self.ana_ctrl.avoid_inlining() || self.marker_carrying.body_carries_marker(local_id))
{
debug!("Inlining {function:?}");
return Some((id, *location, InlineAction::SimpleInline(local_id)));
}
}
let local_as_global = GlobalLocal::at_root;
let call = self.get_call(*location);
Expand Down
3 changes: 2 additions & 1 deletion tests/inline-elision-tests/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@ fn inner() -> u32 {
}

fn elide_me(i: u32) -> u32 {
i
std::convert::identity(i)
}

fn elide_me2(i: &mut u32) {
*i = std::convert::identity(*i);
}

#[dfpp::analyze]
Expand Down
6 changes: 4 additions & 2 deletions tests/inline_elision_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ use helpers::*;

const CRATE_DIR: &str = "tests/inline-elision-tests";

const TARGET_FN: &str = "std::convert::identity";

lazy_static! {
static ref TEST_CRATE_ANALYZED: bool =
run_dfpp_with_graph_dump_and(CRATE_DIR, ["--inline-elision"]);
Expand All @@ -22,15 +24,15 @@ macro_rules! define_test {
define_test!(basic_elision : graph -> {
let input = graph.function_call("input");
let receiver = graph.function_call("receive_touched");
assert!(graph.function_calls("elide_me").is_empty() || graph.connects_none(&graph.function_call("elide_me")));
assert!(graph.function_calls(TARGET_FN).is_empty() || graph.connects_none(&graph.function_call(TARGET_FN)));
assert!(graph.connects(&input, &receiver))

});

define_test!(basic_elision_mut : graph -> {
let input = graph.function_call("input");
let receiver = graph.function_call("receive_touched");
assert!(graph.function_calls("elide_me").is_empty() || graph.connects_none(&graph.function_call("elide_me")));
assert!(graph.function_calls(TARGET_FN).is_empty() || graph.connects_none(&graph.function_call(TARGET_FN)));
assert!(graph.connects(&input, &receiver))

});
Expand Down

0 comments on commit 31b5991

Please sign in to comment.