diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index bf16a38459..789825fbf2 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -196,10 +196,8 @@ jobs: with: submodules: true - uses: ./.github/actions/install-rust - - name: Delete ISLE manifest files to force a re-build - run: rm cranelift/codegen/src/isa/aarch64/lower/isle/generated_code.manifest cranelift/codegen/src/isa/s390x/lower/isle/generated_code.manifest cranelift/codegen/src/isa/x64/lower/isle/generated_code.manifest - - name: Rebuild ISLE DSL files - run: cargo build -p cranelift-codegen --features "rebuild-isle" + - name: Force-rebuild ISLE DSL files + run: ./scripts/force-rebuild-isle.sh - name: Reformat run: cargo fmt -p cranelift-codegen - name: Check that the ISLE DSL files are up-to-date diff --git a/cranelift/codegen/build.rs b/cranelift/codegen/build.rs index 1b95bf02a8..29b3d03c56 100644 --- a/cranelift/codegen/build.rs +++ b/cranelift/codegen/build.rs @@ -232,6 +232,10 @@ fn get_isle_compilations(crate_dir: &std::path::Path) -> Result(ctx: &mut C, arg0: &Amode, arg1: Value) } } } - let pattern0_0 = arg0; let pattern1_0 = arg1; if let Some(pattern2_0) = C::def_inst(ctx, pattern1_0) { let pattern3_0 = C::inst_data(ctx, pattern2_0); @@ -1395,6 +1394,7 @@ pub fn constructor_amode_add(ctx: &mut C, arg0: &Amode, arg1: Value) } } } + let pattern0_0 = arg0; match pattern0_0 { &Amode::ImmReg { simm32: pattern1_0, diff --git a/cranelift/isle/isle/isle_examples/pass/prio_trie_bug.isle b/cranelift/isle/isle/isle_examples/pass/prio_trie_bug.isle new file mode 100644 index 0000000000..19fa9c78fa --- /dev/null +++ b/cranelift/isle/isle/isle_examples/pass/prio_trie_bug.isle @@ -0,0 +1,108 @@ +;; Minimized bug reproducer for earlier priority-range-merging trie +;; implementation in ISLE compiler. This example, when compiled with +;; old versions of islec, would result in the bottom-most rule (at +;; priority 0) being applied before the rule involving `iconst` at +;; priority 1 below. + +(type Unit (primitive Unit)) +(type u8 (primitive u8)) +(type u32 (primitive u32)) +(type Reg (primitive Reg)) +(type MemFlags (primitive MemFlags)) +(type MachLabel (primitive MachLabel)) +(type Value (primitive Value)) + +(decl iadd (Value Value) Value) +(extern extractor iadd iadd) +(decl ishl (Value Value) Value) +(extern extractor ishl ishl) +(decl uextend (Value) Value) +(extern extractor uextend uextend) +(decl sextend (Value) Value) +(extern extractor sextend sextend) +(decl iconst (u32) Value) +(extern extractor iconst iconst) + +(decl put_in_reg (Value) Reg) +(convert Value Reg put_in_reg) +(extern constructor put_in_reg put_in_reg) + +(decl invalid_reg () Reg) +(extern extractor invalid_reg invalid_reg) +(decl valid_reg () Reg) +(extern extractor valid_reg valid_reg) + +(decl pure u32_lteq (u32 u32) Unit) +(extern constructor u32_lteq u32_lteq) + +(decl pure s32_add_fallible (u32 u32) u32) +(extern constructor s32_add_fallible s32_add_fallible) + +(decl x64_add (Reg Reg) Reg) +(extern constructor x64_add x64_add) + +;; An `Amode` represents a possible addressing mode that can be used +;; in instructions. These denote a 64-bit value only. +(type Amode (enum + ;; Immediate sign-extended and a register + (ImmReg (simm32 u32) + (base Reg) + (flags MemFlags)) + + ;; Sign-extend-32-to-64(simm32) + base + (index << shift) + (ImmRegRegShift (simm32 u32) + (base Reg) + (index Reg) + (shift u32) + (flags MemFlags)) + + ;; Sign-extend-32-to-64(immediate) + RIP (instruction + ;; pointer). The appropriate relocation is emitted so + ;; that the resulting immediate makes this Amode refer to + ;; the given MachLabel. + (RipRelative (target MachLabel)))) + +;; One step in amode processing: take an existing amode and add +;; another value to it. +(decl amode_add (Amode Value) Amode) + +;; -- Top-level driver: pull apart the addends. +;; +;; Any amode can absorb an `iadd` by absorbing first the LHS of the +;; add, then the RHS. +;; +;; Priority 2 to take this above fallbacks and ensure we traverse the +;; `iadd` tree fully. +(rule 2 (amode_add amode (iadd x y)) + (let ((amode1 Amode (amode_add amode x)) + (amode2 Amode (amode_add amode1 y))) + amode2)) + +;; -- Case 1 (adding a register to the initial Amode with invalid_reg). +;; +;; An Amode.ImmReg with invalid_reg (initial state) can absorb a +;; register as the base register. +(rule (amode_add (Amode.ImmReg off (invalid_reg) flags) value) + (Amode.ImmReg off value flags)) + +;; -- Case 4 (absorbing constant offsets). +;; +;; An Amode can absorb a constant (i64, or extended i32) as long as +;; the sum still fits in the signed-32-bit offset. +;; +;; Priority 3 in order to take this option above the fallback +;; (immediate in register). Two rules, for imm+reg and +;; imm+reg+scale*reg cases. +(rule 1 (amode_add (Amode.ImmRegRegShift off base index shift flags) + (iconst c)) + (if-let sum (s32_add_fallible off c)) + (Amode.ImmRegRegShift sum base index shift flags)) + +;; -- Case 5 (fallback to add a new value to an imm+reg+scale*reg). +;; +;; An Amode.ImmRegRegShift can absorb any other value by creating a +;; new add instruction and replacing the base with +;; (base+value). +(rule (amode_add (Amode.ImmRegRegShift off base index shift flags) value) + (let ((sum Reg (x64_add base value))) + (Amode.ImmRegRegShift off sum index shift flags))) diff --git a/cranelift/isle/isle/src/codegen.rs b/cranelift/isle/isle/src/codegen.rs index c6ac0c98c5..b280d3a8f2 100644 --- a/cranelift/isle/isle/src/codegen.rs +++ b/cranelift/isle/isle/src/codegen.rs @@ -693,16 +693,11 @@ impl<'a> Codegen<'a> { &TrieNode::Decision { ref edges } => { let subindent = format!("{} ", indent); - // if this is a decision node, generate each match op - // in turn (in priority order). Sort the ops within - // each priority, and gather together adjacent - // MatchVariant ops with the same input and disjoint - // variants in order to create a `match` rather than a - // chain of if-lets. - let mut edges = edges.clone(); - edges.sort_by(|e1, e2| { - (-e1.range.min, &e1.symbol).cmp(&(-e2.range.min, &e2.symbol)) - }); + // If this is a decision node, generate each match op + // in turn (in priority order). Gather together + // adjacent MatchVariant ops with the same input and + // disjoint variants in order to create a `match` + // rather than a chain of if-lets. let mut i = 0; while i < edges.len() { @@ -712,8 +707,8 @@ impl<'a> Codegen<'a> { let mut adjacent_variants = BTreeSet::new(); let mut adjacent_variant_input = None; log::trace!( - "edge: range = {:?}, symbol = {:?}", - edges[i].range, + "edge: prio = {:?}, symbol = {:?}", + edges[i].prio, edges[i].symbol ); while last < edges.len() { diff --git a/cranelift/isle/isle/src/trie.rs b/cranelift/isle/isle/src/trie.rs index 5acb08b0ab..686d0afd98 100644 --- a/cranelift/isle/isle/src/trie.rs +++ b/cranelift/isle/isle/src/trie.rs @@ -100,67 +100,19 @@ pub fn build_tries(typeenv: &TypeEnv, termenv: &TermEnv) -> BTreeMap= P_lo and P <= P_hi, we "priority-split the edges" at priority -/// P. -/// -/// To priority-split the edges in a node at priority P: -/// -/// - For each out-edge with priority [P_lo, P_hi] s.g. P \in [P_lo, -/// P_hi], and token T: -/// - Trim the subnode at P, yielding children C_lo and C_hi. -/// - Both children must be non-empty (have at least one leaf) -/// because the original node must have had a leaf at P_lo -/// and a leaf at P_hi. -/// - Replace the one edge with two edges, one for each child, with -/// the original match op, and with ranges calculated according to -/// the trimmed children. -/// -/// To trim a node into range [P_lo, P_hi]: -/// -/// - For a decision node: -/// - If any edges have a range outside the bounds of the trimming -/// range, trim the bounds of the edge, and trim the subtree under the -/// edge into the trimmed edge's range. If the subtree is trimmed -/// to `None`, remove the edge. -/// - If all edges are removed, the decision node becomes `None`. -/// - For a leaf node: -/// - If the priority is outside the range, the node becomes `None`. -/// -/// As we descend a path to insert a leaf node, we (i) priority-split -/// if any edges' priority ranges overlap the insertion priority -/// range, and (ii) expand priority ranges on edges to include the new -/// leaf node's priority. -/// -/// As long as we do this, we ensure the two key priority-trie -/// invariants: -/// -/// 1. At a given node, no two edges exist with priority ranges R_1, -/// R_2 such that R_1 ∩ R_2 ≠ ∅, unless R_1 and R_2 are unit ranges -/// ([x, x]) and are on edges with different match-ops. -/// 2. Along the path from the root to any leaf node with priority P, -/// each edge has a priority range R such that P ∈ R. +/// logic, creating edges and subnodes where necessary. A new edge is +/// necessary whenever an edge does not exist for the (priority, +/// symbol) tuple. /// /// Note that this means that multiple edges with a single match-op /// may exist, with different priorities. @@ -187,78 +139,11 @@ impl TrieSymbol { /// A priority. pub type Prio = i64; -/// An inclusive range of priorities. -#[derive(Clone, Copy, Debug)] -pub struct PrioRange { - /// The minimum of this range. - pub min: Prio, - /// The maximum of this range. - pub max: Prio, -} - -impl PrioRange { - fn contains(&self, prio: Prio) -> bool { - prio >= self.min && prio <= self.max - } - - fn is_unit(&self) -> bool { - self.min == self.max - } - - fn overlaps(&self, other: PrioRange) -> bool { - // This can be derived via DeMorgan: !(self.begin > other.end - // OR other.begin > self.end). - self.min <= other.max && other.min <= self.max - } - - fn intersect(&self, other: PrioRange) -> PrioRange { - PrioRange { - min: std::cmp::max(self.min, other.min), - max: std::cmp::min(self.max, other.max), - } - } - - fn union(&self, other: PrioRange) -> PrioRange { - PrioRange { - min: std::cmp::min(self.min, other.min), - max: std::cmp::max(self.max, other.max), - } - } - - fn split_at(&self, prio: Prio) -> (PrioRange, PrioRange) { - assert!(self.contains(prio)); - assert!(!self.is_unit()); - if prio == self.min { - ( - PrioRange { - min: self.min, - max: self.min, - }, - PrioRange { - min: self.min + 1, - max: self.max, - }, - ) - } else { - ( - PrioRange { - min: self.min, - max: prio - 1, - }, - PrioRange { - min: prio, - max: self.max, - }, - ) - } - } -} - /// An edge in our term trie. #[derive(Clone, Debug)] pub struct TrieEdge { - /// The priority range for this edge's sub-trie. - pub range: PrioRange, + /// The priority for this edge's sub-trie. + pub prio: Prio, /// The match operation to perform for this edge. pub symbol: TrieSymbol, /// This edge's sub-trie. @@ -319,91 +204,19 @@ impl TrieNode { _ => panic!("insert on leaf node!"), }; - // Do we need to split? - let needs_split = edges - .iter() - .any(|edge| edge.range.contains(prio) && !edge.range.is_unit()); - - // If so, pass over all edges/subnodes and split each. - if needs_split { - let mut new_edges = vec![]; - for edge in std::mem::take(edges) { - if !edge.range.contains(prio) || edge.range.is_unit() { - new_edges.push(edge); - continue; - } - - let (lo_range, hi_range) = edge.range.split_at(prio); - let lo = edge.node.trim(lo_range); - let hi = edge.node.trim(hi_range); - if let Some((node, range)) = lo { - new_edges.push(TrieEdge { - range, - symbol: edge.symbol.clone(), - node, - }); - } - if let Some((node, range)) = hi { - new_edges.push(TrieEdge { - range, - symbol: edge.symbol, - node, - }); - } - } - *edges = new_edges; - } - // Now find or insert the appropriate edge. - let mut edge: Option = None; - let mut last_edge_with_op: Option = None; - let mut last_edge_with_op_prio: Option = None; - for i in 0..(edges.len() + 1) { - if i == edges.len() || prio > edges[i].range.max { - // We've passed all edges with overlapping priority - // ranges. Maybe the last edge we saw with the op - // we're inserting can have its range expanded, - // however. - if last_edge_with_op.is_some() { - // Move it to the end of the run of equal-unit-range ops. - edges.swap(last_edge_with_op.unwrap(), i - 1); - edge = Some(i - 1); - edges[i - 1].range.max = prio; - break; - } - edges.insert( - i, - TrieEdge { - range: PrioRange { - min: prio, - max: prio, - }, - symbol: op.clone(), - node: TrieNode::Empty, - }, - ); - edge = Some(i); - break; - } - if i == edges.len() { - break; - } - if edges[i].symbol == op { - last_edge_with_op = Some(i); - last_edge_with_op_prio = Some(edges[i].range.max); - } - if last_edge_with_op_prio.is_some() - && last_edge_with_op_prio.unwrap() < edges[i].range.max - { - last_edge_with_op = None; - last_edge_with_op_prio = None; - } - if edges[i].range.contains(prio) && edges[i].symbol == op { - edge = Some(i); - break; - } - } - let edge = edge.expect("Must have found an edge at least at last iter"); + let edge = edges + .iter() + .position(|edge| edge.symbol == op && edge.prio == prio) + .unwrap_or_else(|| { + edges.push(TrieEdge { + prio, + symbol: op, + node: TrieNode::Empty, + }); + edges.len() - 1 + }); + let edge = &mut edges[edge]; if is_last { @@ -420,57 +233,18 @@ impl TrieNode { } } - fn trim(&self, range: PrioRange) -> Option<(TrieNode, PrioRange)> { + /// Sort edges by priority. + pub fn sort(&mut self) { match self { - &TrieNode::Empty => None, - &TrieNode::Leaf { prio, ref output } => { - if range.contains(prio) { - Some(( - TrieNode::Leaf { - prio, - output: output.clone(), - }, - PrioRange { - min: prio, - max: prio, - }, - )) - } else { - None - } - } - &TrieNode::Decision { ref edges } => { - let edges = edges - .iter() - .filter_map(|edge| { - if !edge.range.overlaps(range) { - None - } else { - let range = range.intersect(edge.range); - if let Some((node, range)) = edge.node.trim(range) { - Some(TrieEdge { - range, - symbol: edge.symbol.clone(), - node, - }) - } else { - None - } - } - }) - .collect::>(); - - if edges.is_empty() { - None - } else { - let range = edges - .iter() - .map(|edge| edge.range) - .reduce(|a, b| a.union(b)) - .expect("reduce on non-empty vec must not return None"); - Some((TrieNode::Decision { edges }, range)) + TrieNode::Decision { edges } => { + // Sort by priority, highest integer value first; then + // by trie symbol. + edges.sort_by_cached_key(|edge| (-edge.prio, edge.symbol.clone())); + for child in edges { + child.node.sort(); } } + _ => {} } } @@ -490,8 +264,8 @@ impl TrieNode { for edge in edges { s.push_str(indent); s.push_str(&format!( - " edge: range = {:?}, symbol: {:?}\n", - edge.range, edge.symbol + " edge: prio = {:?}, symbol: {:?}\n", + edge.prio, edge.symbol )); pretty_rec(s, &edge.node, &new_indent); } @@ -535,6 +309,10 @@ impl TermFunctionBuilder { .chain(std::iter::once(TrieSymbol::EndOfMatch)); self.trie.insert(prio, symbols, expr_seq); } + + fn sort_trie(&mut self) { + self.trie.sort(); + } } #[derive(Debug)] @@ -574,6 +352,10 @@ impl<'a> TermFunctionsBuilder<'a> { .or_insert_with(|| TermFunctionBuilder::new()) .add_rule(prio, pattern.clone(), expr.clone()); } + + for builder in self.builders_by_term.values_mut() { + builder.sort_trie(); + } } fn finalize(self) -> BTreeMap { diff --git a/scripts/force-rebuild-isle.sh b/scripts/force-rebuild-isle.sh new file mode 100755 index 0000000000..1137341c34 --- /dev/null +++ b/scripts/force-rebuild-isle.sh @@ -0,0 +1,19 @@ +#!/bin/sh +# +# This script rebuilds all ISLE generated source that is checked in, even if +# the source has not changed relative to the manifests. +# +# This is useful when one is developing the ISLE compiler itself; otherwise, +# changing the compiler does not automatically change the generated code, even +# if the `rebuild-isle` feature is specified. + +set -e + +# Remove the manifests (which contain hashes of ISLE source) to force the build +# script to regenerate all backends. +rm -f cranelift/codegen/src/isa/*/lower/isle/generated_code.manifest + +# `cargo check` will both invoke the build script to rebuild the backends, and +# check that the output is valid Rust. We specify `all-arch` here to include +# all backends. +cargo check -p cranelift-codegen --features rebuild-isle,all-arch