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

@@ -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<Span>,
     },
+
+    /// The rules can never match because another rule will always match first.
+    ShadowedError {
+        /// The locations of the unmatchable rules.
+        shadowed: Vec<Span>,
+
+        /// The location of the rule that shadows them.
+        mask: Span,
+    },
 }
 
 impl Errors {

cranelift/isle/isle/src/lib.rs

@@ -197,6 +197,20 @@ impl<T: Copy + std::fmt::Debug + Eq + Hash> DisjointSets<T> {
     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;

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<Pos, HashSet<Pos>>,
+    /// For each (mask, shadowed) pair, every rule in `shadowed` is unmatchable because `mask` will
+    /// always match first.
+    shadowed: HashMap<Pos, Vec<Pos>>,
 }
 
 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);
+                }
             }
         }
     }

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<Constraint> {