Skip to content

Commit

Permalink
Merge branch 'master' into gd/issue_6946
Browse files Browse the repository at this point in the history
  • Loading branch information
TomAFrench authored Jan 14, 2025
2 parents ca52a8e + a0ffedf commit 4283acf
Show file tree
Hide file tree
Showing 14 changed files with 363 additions and 62 deletions.
37 changes: 37 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions compiler/noirc_evaluator/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ chrono = "0.4.37"
rayon.workspace = true
cfg-if.workspace = true
smallvec = { version = "1.13.2", features = ["serde"] }
vec-collections = "0.4.3"

[dev-dependencies]
proptest.workspace = true
Expand Down
6 changes: 4 additions & 2 deletions compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ mod block;
use std::collections::{BTreeMap, BTreeSet};

use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet};
use vec_collections::VecSet;

use crate::ssa::{
ir::{
Expand Down Expand Up @@ -619,7 +620,7 @@ impl<'f> PerFunctionContext<'f> {
// then those parameters also alias each other.
// We save parameters with repeat arguments to later mark those
// parameters as aliasing one another.
let mut arg_set: HashMap<ValueId, BTreeSet<ValueId>> = HashMap::default();
let mut arg_set = HashMap::default();

// Add an alias for each reference parameter
for (parameter, argument) in destination_parameters.iter().zip(arguments) {
Expand All @@ -632,7 +633,8 @@ impl<'f> PerFunctionContext<'f> {
aliases.insert(*parameter);

// Check if we have seen the same argument
let seen_parameters = arg_set.entry(argument).or_default();
let seen_parameters =
arg_set.entry(argument).or_insert_with(VecSet::empty);
// Add the current parameter to the parameters we have seen for this argument.
// The previous parameters and the current one alias one another.
seen_parameters.insert(*parameter);
Expand Down
32 changes: 22 additions & 10 deletions compiler/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::collections::BTreeSet;
use vec_collections::{AbstractVecSet, VecSet};

use crate::ssa::ir::value::ValueId;

Expand All @@ -10,7 +10,7 @@ use crate::ssa::ir::value::ValueId;
/// "unknown which aliases this may refer to" - `None`.
#[derive(Debug, Default, Clone)]
pub(super) struct AliasSet {
aliases: Option<BTreeSet<ValueId>>,
aliases: Option<VecSet<[ValueId; 1]>>,
}

impl AliasSet {
Expand All @@ -19,20 +19,18 @@ impl AliasSet {
}

pub(super) fn known(value: ValueId) -> AliasSet {
let mut aliases = BTreeSet::new();
aliases.insert(value);
Self { aliases: Some(aliases) }
Self { aliases: Some(VecSet::single(value)) }
}

pub(super) fn known_multiple(values: BTreeSet<ValueId>) -> AliasSet {
pub(super) fn known_multiple(values: VecSet<[ValueId; 1]>) -> AliasSet {
Self { aliases: Some(values) }
}

/// In rare cases, such as when creating an empty array of references, the set of aliases for a
/// particular value will be known to be zero, which is distinct from being unknown and
/// possibly referring to any alias.
pub(super) fn known_empty() -> AliasSet {
Self { aliases: Some(BTreeSet::new()) }
Self { aliases: Some(VecSet::empty()) }
}

pub(super) fn is_unknown(&self) -> bool {
Expand All @@ -44,19 +42,33 @@ impl AliasSet {
pub(super) fn single_alias(&self) -> Option<ValueId> {
self.aliases
.as_ref()
.and_then(|aliases| (aliases.len() == 1).then(|| *aliases.first().unwrap()))
.and_then(|aliases| (aliases.len() == 1).then(|| *aliases.iter().next().unwrap()))
}

/// Unify this alias set with another. The result of this set is empty if either set is empty.
/// Otherwise, it is the union of both alias sets.
pub(super) fn unify(&mut self, other: &Self) {
if let (Some(self_aliases), Some(other_aliases)) = (&mut self.aliases, &other.aliases) {
self_aliases.extend(other_aliases);
self_aliases.extend(other_aliases.iter().cloned());
} else {
self.aliases = None;
}
}

/// Returns true if calling `unify` would change something in this alias set.
///
/// This is an optimization to avoid having to look up an entry ready to be modified in the [Block](crate::ssa::opt::mem2reg::block::Block),
/// because doing so would involve calling `Arc::make_mut` which clones the entry, ready for modification.
pub(super) fn should_unify(&self, other: &Self) -> bool {
if let (Some(self_aliases), Some(other_aliases)) = (&self.aliases, &other.aliases) {
// `unify` would extend `self_aliases` with `other_aliases`, so if `other_aliases` is a subset, then nothing would happen.
!other_aliases.is_subset(self_aliases)
} else {
// `unify` would set `aliases` to `None`, so if it's not `Some`, then nothing would happen.
self.aliases.is_some()
}
}

/// Inserts a new alias into this set if it is not unknown
pub(super) fn insert(&mut self, new_alias: ValueId) {
if let Some(aliases) = &mut self.aliases {
Expand All @@ -82,6 +94,6 @@ impl AliasSet {
/// The ordering is arbitrary (by lowest ValueId) so this method should only be
/// used when you need an arbitrary ValueId from the alias set.
pub(super) fn first(&self) -> Option<ValueId> {
self.aliases.as_ref().and_then(|aliases| aliases.first().copied())
self.aliases.as_ref().and_then(|aliases| aliases.iter().next().copied())
}
}
6 changes: 6 additions & 0 deletions compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,12 @@ impl Block {
}

for (expression, new_aliases) in &other.aliases {
// If nothing would change, then don't call `.entry(...).and_modify(...)` as it involves creating more `Arc`s.
if let Some(aliases) = self.aliases.get(expression) {
if !aliases.should_unify(new_aliases) {
continue;
}
}
let expression = expression.clone();

self.aliases
Expand Down
Loading

0 comments on commit 4283acf

Please sign in to comment.