From 31b5991ac69c50a046520c4659bd70ece77160fa Mon Sep 17 00:00:00 2001 From: Justus Adam Date: Fri, 28 Jul 2023 10:56:56 -0700 Subject: [PATCH] Fixing Inline Elision (Tests) (#23) 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. --- .github/workflows/rust.yml | 1 + src/ana/algebra.rs | 44 ++++++++++++++------------ src/ana/inline.rs | 14 ++++---- tests/inline-elision-tests/src/main.rs | 3 +- tests/inline_elision_tests.rs | 6 ++-- 5 files changed, 37 insertions(+), 31 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 325bdbdb06..87499be332 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -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 diff --git a/src/ana/algebra.rs b/src/ana/algebra.rs index b3b28aeec7..bab222a264 100644 --- a/src/ana/algebra.rs +++ b/src/ana/algebra.rs @@ -302,6 +302,7 @@ impl Equality { 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 @@ -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)?; @@ -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 @@ -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(']') @@ -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 { diff --git a/src/ana/inline.rs b/src/ana/inline.rs index dd4713ed4a..dbc594c6b7 100644 --- a/src/ana/inline.rs +++ b/src/ana/inline.rs @@ -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); diff --git a/tests/inline-elision-tests/src/main.rs b/tests/inline-elision-tests/src/main.rs index 8975be3e25..1ae7826cc0 100644 --- a/tests/inline-elision-tests/src/main.rs +++ b/tests/inline-elision-tests/src/main.rs @@ -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] diff --git a/tests/inline_elision_tests.rs b/tests/inline_elision_tests.rs index c3f16beab3..cbe31703e3 100644 --- a/tests/inline_elision_tests.rs +++ b/tests/inline_elision_tests.rs @@ -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"]); @@ -22,7 +24,7 @@ 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)) }); @@ -30,7 +32,7 @@ define_test!(basic_elision : graph -> { 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)) });