diff --git a/cranelift/isle/isle/src/error.rs b/cranelift/isle/isle/src/error.rs index d919e36d1d..999821edde 100644 --- a/cranelift/isle/isle/src/error.rs +++ b/cranelift/isle/isle/src/error.rs @@ -24,6 +24,9 @@ impl std::fmt::Debug for Errors { Error::TypeError { msg, .. } => format!("type error: {}", msg), Error::UnreachableError { msg, .. } => format!("unreachable rule: {}", msg), Error::OverlapError { msg, .. } => format!("overlap error: {}", msg), + Error::ShadowedError { .. } => { + format!("more general higher-priority rule shadows other rules") + } }; let labels = match e { @@ -44,6 +47,16 @@ impl std::fmt::Debug for Errors { ); labels } + + Error::ShadowedError { shadowed, mask } => { + let mut labels = vec![Label::primary(mask.from.file, mask)]; + labels.extend( + shadowed + .iter() + .map(|span| Label::secondary(span.from.file, span)), + ); + labels + } }; let mut sources = Vec::new(); @@ -114,6 +127,15 @@ pub enum Error { /// wildcard case). rules: Vec, }, + + /// The rules can never match because another rule will always match first. + ShadowedError { + /// The locations of the unmatchable rules. + shadowed: Vec, + + /// The location of the rule that shadows them. + mask: Span, + }, } impl Errors { diff --git a/cranelift/isle/isle/src/lib.rs b/cranelift/isle/isle/src/lib.rs index 1248bcbf48..6d27f2c34f 100644 --- a/cranelift/isle/isle/src/lib.rs +++ b/cranelift/isle/isle/src/lib.rs @@ -197,6 +197,20 @@ impl DisjointSets { pub fn is_empty(&self) -> bool { self.parent.is_empty() } + + /// Returns the total number of elements in all sets. This method takes constant time. + /// + /// ``` + /// let mut sets = cranelift_isle::DisjointSets::default(); + /// sets.merge(1, 2); + /// assert_eq!(sets.len(), 2); + /// sets.merge(3, 4); + /// sets.merge(3, 5); + /// assert_eq!(sets.len(), 5); + /// ``` + pub fn len(&self) -> usize { + self.parent.len() + } } pub mod ast; diff --git a/cranelift/isle/isle/src/overlap.rs b/cranelift/isle/isle/src/overlap.rs index 4411de8c88..d127fc7f97 100644 --- a/cranelift/isle/isle/src/overlap.rs +++ b/cranelift/isle/isle/src/overlap.rs @@ -29,6 +29,9 @@ pub fn check(tyenv: &TypeEnv, termenv: &TermEnv) -> Result<(), error::Errors> { struct Errors { /// Edges between rules indicating overlap. nodes: HashMap>, + /// For each (mask, shadowed) pair, every rule in `shadowed` is unmatchable because `mask` will + /// always match first. + shadowed: HashMap>, } impl Errors { @@ -66,19 +69,37 @@ impl Errors { }); } + errors.extend( + self.shadowed + .into_iter() + .map(|(mask, shadowed)| Error::ShadowedError { + shadowed: shadowed.into_iter().map(Span::new_single).collect(), + mask: Span::new_single(mask), + }), + ); + errors.sort_by_key(|err| match err { - Error::OverlapError { rules, .. } => rules.first().unwrap().from, + Error::ShadowedError { mask, .. } => mask.from, + Error::OverlapError { rules, .. } => rules[0].from, _ => Pos::default(), }); errors } fn check_pair(&mut self, a: &trie_again::Rule, b: &trie_again::Rule) { - if let trie_again::Overlap::Yes { .. } = a.may_overlap(b) { + if let trie_again::Overlap::Yes { subset } = a.may_overlap(b) { if a.prio == b.prio { // edges are undirected self.nodes.entry(a.pos).or_default().insert(b.pos); self.nodes.entry(b.pos).or_default().insert(a.pos); + } else if subset { + // One rule's constraints are a subset of the other's, or they're equal. + // This is fine as long as the higher-priority rule has more constraints. + let (lo, hi) = if a.prio < b.prio { (a, b) } else { (b, a) }; + if hi.total_constraints() <= lo.total_constraints() { + // Otherwise, the lower-priority rule can never match. + self.shadowed.entry(hi.pos).or_default().push(lo.pos); + } } } } diff --git a/cranelift/isle/isle/src/trie_again.rs b/cranelift/isle/isle/src/trie_again.rs index ff046ece3a..4b02159bc2 100644 --- a/cranelift/isle/isle/src/trie_again.rs +++ b/cranelift/isle/isle/src/trie_again.rs @@ -151,8 +151,8 @@ pub enum Overlap { Yes { /// True if every input accepted by one rule is also accepted by the other. This does not /// indicate which rule is more general and in fact the rules could match exactly the same - /// set of inputs. You can work out which by comparing the number of constraints in both - /// rules: The more general rule has fewer constraints. + /// set of inputs. You can work out which by comparing `total_constraints()` in both rules: + /// The more general rule has fewer constraints. subset: bool, }, } @@ -213,7 +213,10 @@ impl Rule { // that they can cause rules to not overlap. However, because we don't have a concrete // pattern to compare, the analysis to prove that is complicated. For now, we approximate // the result. If either rule has nonlinear constraints, conservatively report that neither - // is a subset of the other. + // is a subset of the other. Note that this does not disagree with the doc comment for + // `Overlap::Yes { subset }` which says to use `total_constraints` to disambiguate, since if + // we return `subset: true` here, `equals` is empty for both rules, so `total_constraints()` + // equals `constraints.len()`. let mut subset = small.equals.is_empty() && big.equals.is_empty(); for (binding, a) in small.constraints.iter() { @@ -234,6 +237,14 @@ impl Rule { Overlap::Yes { subset } } + /// Returns the total number of binding sites which this rule constrains, with either a concrete + /// pattern or an equality constraint. + pub fn total_constraints(&self) -> usize { + // Because of `normalize_equivalence_classes`, these two sets don't overlap, so the size of + // the union is the sum of their sizes. + self.constraints.len() + self.equals.len() + } + /// Returns the constraint that the given binding site must satisfy for this rule to match, if /// there is one. pub fn get_constraint(&self, source: BindingId) -> Option {