From 0e65f87e379315f1ed419aeafddba68e800ad5f9 Mon Sep 17 00:00:00 2001 From: Jamey Sharp Date: Wed, 30 Nov 2022 15:06:00 -0800 Subject: [PATCH] cranelift-isle: Reject unreachable rules (#5322) Some of our ISLE rules can never fire because there's a higher-priority rule that will always fire instead. Sometimes the worst that can happen is we generate sub-optimal output. That's not so bad but we'd still like to know about it so we can fix it. In other cases there might be instructions which can't be lowered in isolation. If a general rule for lowering one of the instructions is higher-priority than the rule for lowering the combined sequence, then lowering the combined sequence will always fail. Either way, this is always a bug, so make it a fatal error if we can detect it. --- cranelift/isle/isle/src/error.rs | 22 ++++++++++++++++++++++ cranelift/isle/isle/src/lib.rs | 14 ++++++++++++++ cranelift/isle/isle/src/overlap.rs | 25 +++++++++++++++++++++++-- cranelift/isle/isle/src/trie_again.rs | 17 ++++++++++++++--- 4 files changed, 73 insertions(+), 5 deletions(-) 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 {