Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#7954 alias analysis attempt #9213

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

#7954 alias analysis attempt #9213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions cranelift/codegen/src/alias_analysis.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@
use crate::{
cursor::{Cursor, FuncCursor},
dominator_tree::DominatorTree,
dominator_tree::DominatorTreePreorder,
inst_predicates::{
has_memory_fence_semantics, inst_addr_offset_type, inst_store_data, visit_block_succs,
},
Expand Down Expand Up @@ -176,7 +176,7 @@ struct MemoryLoc {
/// An alias-analysis pass.
pub struct AliasAnalysis<'a> {
/// The domtree for the function.
domtree: &'a DominatorTree,
domtree: &'a DominatorTreePreorder,

/// Input state to a basic block.
block_input: FxHashMap<Block, LastStores>,
Expand All @@ -191,7 +191,7 @@ pub struct AliasAnalysis<'a> {

impl<'a> AliasAnalysis<'a> {
/// Perform an alias analysis pass.
pub fn new(func: &Function, domtree: &'a DominatorTree) -> AliasAnalysis<'a> {
pub fn new(func: &Function, domtree: &'a DominatorTreePreorder) -> AliasAnalysis<'a> {
trace!("alias analysis: input is:\n{:?}", func);
let mut analysis = AliasAnalysis {
domtree,
Expand Down Expand Up @@ -333,7 +333,7 @@ impl<'a> AliasAnalysis<'a> {
value.index(),
def_inst.index()
);
if self.domtree.dominates(def_inst, inst, &func.layout) {
if self.domtree.dominates_inst(def_inst, inst, &func.layout) {
trace!(
" -> dominates; value equiv from v{} to v{} inserted",
load_result.index(),
Expand Down
15 changes: 11 additions & 4 deletions cranelift/codegen/src/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
//! single ISA instance.

use crate::alias_analysis::AliasAnalysis;
use crate::dominator_tree::DominatorTree;
use crate::dominator_tree::{DominatorTree, DominatorTreePreorder};
use crate::egraph::EgraphPass;
use crate::flowgraph::ControlFlowGraph;
use crate::ir::Function;
Expand Down Expand Up @@ -46,6 +46,9 @@ pub struct Context {
/// Dominator tree for `func`.
pub domtree: DominatorTree,

/// Domniator tree preorder, be on look out to merge with domtree, although unlikely
pub domtree_preorder: DominatorTreePreorder,

/// Loop analysis of `func`.
pub loop_analysis: LoopAnalysis,

Expand Down Expand Up @@ -74,6 +77,7 @@ impl Context {
func,
cfg: ControlFlowGraph::new(),
domtree: DominatorTree::new(),
domtree_preorder: DominatorTreePreorder::new(),
loop_analysis: LoopAnalysis::new(),
compiled_code: None,
want_disasm: false,
Expand Down Expand Up @@ -300,7 +304,9 @@ impl Context {

/// Compute dominator tree.
pub fn compute_domtree(&mut self) {
self.domtree.compute(&self.func, &self.cfg)
self.domtree.compute(&self.func, &self.cfg);
self.domtree_preorder
.compute(&self.domtree, &self.func.layout);
}

/// Compute the loop analysis.
Expand Down Expand Up @@ -330,7 +336,7 @@ impl Context {
/// by a store instruction to the same instruction (so-called
/// "store-to-load forwarding").
pub fn replace_redundant_loads(&mut self) -> CodegenResult<()> {
let mut analysis = AliasAnalysis::new(&self.func, &self.domtree);
let mut analysis = AliasAnalysis::new(&self.func, &self.domtree_preorder);
analysis.compute_and_update_aliases(&mut self.func);
Ok(())
}
Expand Down Expand Up @@ -362,7 +368,8 @@ impl Context {
);
let fisa = fisa.into();
self.compute_loop_analysis();
let mut alias_analysis = AliasAnalysis::new(&self.func, &self.domtree);
let mut alias_analysis = AliasAnalysis::new(&self.func, &self.domtree_preorder);

let mut pass = EgraphPass::new(
&mut self.func,
&self.domtree,
Expand Down
36 changes: 35 additions & 1 deletion cranelift/codegen/src/dominator_tree.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,41 @@ impl DominatorTreePreorder {
pub fn dominates(&self, a: Block, b: Block) -> bool {
let na = &self.nodes[a];
let nb = &self.nodes[b];
na.pre_number <= nb.pre_number && na.pre_max >= nb.pre_max
// debug_assert!(na.pre_number != 0);
// debug_assert!(nb.pre_number != 0);
Comment on lines +485 to +486
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the test suite pass with these assertions uncommented? You're testing nb explicitly for being unreachable on line 491, so I'm guessing that at least that one doesn't pass?

Copy link
Author

@badumbatish badumbatish Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi Elliot, one of the test suite failed with this assertion. Here is the summary of the test suite through local ci:

failures:

---- dominator_tree::tests::unreachable_node stdout ----
thread 'dominator_tree::tests::unreachable_node' panicked at cranelift/codegen/src/dominator_tree.rs:486:9:
assertion failed: nb.pre_number != 0


failures:
    dominator_tree::tests::unreachable_node

test result: FAILED. 187 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.21s

error: test failed, to rerun pass `-p cranelift-codegen --lib`

if a == b {
// check first if they're the same block
// if they are then they dominate themselves
true
} else if nb.pre_number == 0 {
// if the supposedly dominated is not reachable (pre_number = 0), is it
// really being dominated though ?
false
} else {
na.pre_number <= nb.pre_number && na.pre_max >= nb.pre_max
}
}

/// Checks if one instruction dominates another.
///
/// Instruction a dominates instruction b if either of these conditions are true:
/// - a and b are in the same block, and a is not after b.
/// - The block containing a dominates the block containing b.
///
/// This helper function is placed next to `fn dominates` for spatial locality of usages
pub fn dominates_inst(&self, a: Inst, b: Inst, layout: &Layout) -> bool {
match (layout.inst_block(a), layout.inst_block(b)) {
(Some(block_a), Some(block_b)) => {
if block_a == block_b {
// Case 1
layout.pp_cmp(a, b) != Ordering::Greater
} else {
// Case 2
self.dominates(block_a, block_b)
}
}
_ => false,
}
}

/// Compare two blocks according to the dominator pre-order.
Expand Down