From a209cb63f5b7ccf9cbb3e69e022a286b1770def9 Mon Sep 17 00:00:00 2001 From: Trevor Elliott Date: Tue, 4 Oct 2022 14:56:49 -0700 Subject: [PATCH] ISLE: Enable the overlap checker (#5011) This PR turns the overlap checker on by default, requiring the use of priorities to resolve overlap between rules. --- cranelift/codegen/src/isa/aarch64/lower.isle | 2 -- cranelift/codegen/src/isa/riscv64/lower.isle | 2 -- cranelift/codegen/src/isa/s390x/lower.isle | 2 -- cranelift/codegen/src/isa/x64/lower.isle | 3 --- .../isle/isle/isle_examples/link/iflets.isle | 6 ++--- .../isle_examples/link/multi_extractor.isle | 2 +- .../isle/isle/isle_examples/pass/test3.isle | 2 +- .../isle/isle_examples/pass/tutorial.isle | 6 ++--- .../run/extractor_ordering_bug.isle | 19 --------------- .../run/extractor_ordering_bug_main.rs | 23 ------------------- cranelift/isle/isle/src/ast.rs | 3 +-- cranelift/isle/isle/src/overlap.rs | 22 +++++++----------- cranelift/isle/isle/src/parser.rs | 6 ++--- cranelift/isle/isle/src/sema.rs | 14 +++-------- 14 files changed, 23 insertions(+), 89 deletions(-) delete mode 100644 cranelift/isle/isle/isle_examples/run/extractor_ordering_bug.isle delete mode 100644 cranelift/isle/isle/isle_examples/run/extractor_ordering_bug_main.rs diff --git a/cranelift/codegen/src/isa/aarch64/lower.isle b/cranelift/codegen/src/isa/aarch64/lower.isle index 7114a0929f..dbe56d7a0e 100644 --- a/cranelift/codegen/src/isa/aarch64/lower.isle +++ b/cranelift/codegen/src/isa/aarch64/lower.isle @@ -1,7 +1,5 @@ ;; aarch64 instruction selection and CLIF-to-MachInst lowering. -(pragma overlap_errors) - ;; The main lowering constructor term: takes a clif `Inst` and returns the ;; register(s) within which the lowered instruction's result values live. (decl lower (Inst) InstOutput) diff --git a/cranelift/codegen/src/isa/riscv64/lower.isle b/cranelift/codegen/src/isa/riscv64/lower.isle index ce29982de0..5b5f6ba6ad 100644 --- a/cranelift/codegen/src/isa/riscv64/lower.isle +++ b/cranelift/codegen/src/isa/riscv64/lower.isle @@ -1,7 +1,5 @@ ;; riscv64 instruction selection and CLIF-to-MachInst lowering. -(pragma overlap_errors) - ;; The main lowering constructor term: takes a clif `Inst` and returns the ;; register(s) within which the lowered instruction's result values live. (decl lower (Inst) InstOutput) diff --git a/cranelift/codegen/src/isa/s390x/lower.isle b/cranelift/codegen/src/isa/s390x/lower.isle index 44323bd4cc..4c05690651 100644 --- a/cranelift/codegen/src/isa/s390x/lower.isle +++ b/cranelift/codegen/src/isa/s390x/lower.isle @@ -1,7 +1,5 @@ ;; s390x instruction selection and CLIF-to-MachInst lowering. -(pragma overlap_errors) - ;; The main lowering constructor term: takes a clif `Inst` and returns the ;; register(s) within which the lowered instruction's result values live. (decl lower (Inst) InstOutput) diff --git a/cranelift/codegen/src/isa/x64/lower.isle b/cranelift/codegen/src/isa/x64/lower.isle index b1ec08e48f..c03239b8de 100644 --- a/cranelift/codegen/src/isa/x64/lower.isle +++ b/cranelift/codegen/src/isa/x64/lower.isle @@ -1,8 +1,5 @@ ;; x86-64 instruction selection and CLIF-to-MachInst lowering. -;; Enable overlap checking for the x64 backend -(pragma overlap_errors) - ;; The main lowering constructor term: takes a clif `Inst` and returns the ;; register(s) within which the lowered instruction's result values live. (decl lower (Inst) InstOutput) diff --git a/cranelift/isle/isle/isle_examples/link/iflets.isle b/cranelift/isle/isle/isle_examples/link/iflets.isle index 5071022a97..8bc6b65439 100644 --- a/cranelift/isle/isle/isle_examples/link/iflets.isle +++ b/cranelift/isle/isle/isle_examples/link/iflets.isle @@ -11,7 +11,7 @@ (decl pure predicate () u32) (rule (predicate) 1) -(rule (C a b c (B d e)) +(rule 2 (C a b c (B d e)) (if-let (B f g) d) (if-let h (A a b)) (A h a)) @@ -20,10 +20,10 @@ (if (predicate)) 42) -(rule (C a b a b) +(rule 1 (C a b a b) (if-let x (D a b)) x) (decl pure D (u32 u32) u32) (rule (D x 0) x) -(rule (D 0 x) x) +(rule 1 (D 0 x) x) diff --git a/cranelift/isle/isle/isle_examples/link/multi_extractor.isle b/cranelift/isle/isle/isle_examples/link/multi_extractor.isle index 84fd15c00b..947dc96446 100644 --- a/cranelift/isle/isle/isle_examples/link/multi_extractor.isle +++ b/cranelift/isle/isle/isle_examples/link/multi_extractor.isle @@ -7,7 +7,7 @@ (decl Rule (u32) u32) -(rule (Rule (E1 a idx)) +(rule 1 (Rule (E1 a idx)) (if-let (A.B) a) idx) (rule (Rule _) diff --git a/cranelift/isle/isle/isle_examples/pass/test3.isle b/cranelift/isle/isle/isle_examples/pass/test3.isle index b2265ce4ba..82d37624d5 100644 --- a/cranelift/isle/isle/isle_examples/pass/test3.isle +++ b/cranelift/isle/isle/isle_examples/pass/test3.isle @@ -56,7 +56,7 @@ (rule (Lower (Iadd ra rb)) (MachInst.Add (UseInput ra) (UseInput rb))) -(rule +(rule 1 (Lower (Iadd (Producer (Iadd ra rb)) rc)) (MachInst.Add3 (UseInput ra) (UseInput rb) (UseInput rc))) (rule diff --git a/cranelift/isle/isle/isle_examples/pass/tutorial.isle b/cranelift/isle/isle/isle_examples/pass/tutorial.isle index d040ceeedb..f072fee74f 100644 --- a/cranelift/isle/isle/isle_examples/pass/tutorial.isle +++ b/cranelift/isle/isle/isle_examples/pass/tutorial.isle @@ -51,12 +51,12 @@ (extern constructor put_in_reg put_in_reg) ;; Simple rule for lowering adds. -(rule (lower (HighLevelInst.Add a b)) +(rule -1 (lower (HighLevelInst.Add a b)) (LowLevelInst.Add (AddrMode.RegReg (put_in_reg a) (put_in_reg b)))) ;; Simple rule for lowering loads. -(rule (lower (HighLevelInst.Load addr)) +(rule -1 (lower (HighLevelInst.Load addr)) (LowLevelInst.Load 0 (put_in_reg addr))) ;; Declare an external extractor for extracting the instruction that defined a @@ -72,7 +72,7 @@ 0))) ;; Rule to sink a load of a base address with a static offset into a single add. -(rule (lower (HighLevelInst.Add +(rule 1 (lower (HighLevelInst.Add a (inst_result (HighLevelInst.Load (inst_result (HighLevelInst.Add diff --git a/cranelift/isle/isle/isle_examples/run/extractor_ordering_bug.isle b/cranelift/isle/isle/isle_examples/run/extractor_ordering_bug.isle deleted file mode 100644 index d0a383920e..0000000000 --- a/cranelift/isle/isle/isle_examples/run/extractor_ordering_bug.isle +++ /dev/null @@ -1,19 +0,0 @@ - -(type u32 (primitive u32)) - -(decl identity (u32) u32) -(extern extractor infallible identity identity) - -(decl is_zero (u32) u32) -(extern extractor is_zero is_zero) - -(decl test (u32) u32) - -;; This exposes a bug where infallible extractors were running before fallible -;; ones, as the derived ordering for the `Extract` type was ordering them ahead -;; of the fallible ones. The result is that the fallible `is_zero` extractor -;; never runs, as the `identity` extractor will always succeed before it's -;; called. -(rule (test (identity x)) x) - -(rule (test (is_zero x)) 2) diff --git a/cranelift/isle/isle/isle_examples/run/extractor_ordering_bug_main.rs b/cranelift/isle/isle/isle_examples/run/extractor_ordering_bug_main.rs deleted file mode 100644 index 70bb544bee..0000000000 --- a/cranelift/isle/isle/isle_examples/run/extractor_ordering_bug_main.rs +++ /dev/null @@ -1,23 +0,0 @@ -mod extractor_ordering_bug; - -struct Context; -impl extractor_ordering_bug::Context for Context { - fn is_zero(&mut self, val: u32) -> Option { - if val == 0 { - Some(val) - } else { - None - } - } - - fn identity(&mut self, val: u32) -> u32 { - val - } -} - -fn main() { - let mut ctx = Context; - - assert_eq!(extractor_ordering_bug::constructor_test(&mut ctx, 0), Some(2)); - assert_eq!(extractor_ordering_bug::constructor_test(&mut ctx, 1), Some(1)); -} diff --git a/cranelift/isle/isle/src/ast.rs b/cranelift/isle/isle/src/ast.rs index 6337ff5b04..1850b5af98 100644 --- a/cranelift/isle/isle/src/ast.rs +++ b/cranelift/isle/isle/src/ast.rs @@ -33,8 +33,7 @@ pub struct Ident(pub String, pub Pos); /// Pragmas parsed with the `(pragma )` syntax. #[derive(Clone, PartialEq, Eq, Debug)] pub enum Pragma { - /// Enable overlap errors in the source. - OverlapErrors, + // currently, no pragmas are defined, but the infrastructure is useful to keep around } /// A declaration of a type. diff --git a/cranelift/isle/isle/src/overlap.rs b/cranelift/isle/isle/src/overlap.rs index 5861b8b5fe..c7b1302346 100644 --- a/cranelift/isle/isle/src/overlap.rs +++ b/cranelift/isle/isle/src/overlap.rs @@ -11,20 +11,14 @@ use crate::sema::{self, Rule, RuleId, Sym, TermEnv, TermId, TermKind, TypeEnv, V /// Check for overlap. pub fn check(tyenv: &TypeEnv, termenv: &TermEnv) -> Result<()> { let mut errors = check_overlaps(termenv).report(tyenv, termenv); - if termenv.overlap_errors { - errors.sort_by_key(|err| match err { - Error::OverlapError { rules, .. } => rules.first().unwrap().1.from, - _ => Pos::default(), - }); - match errors.len() { - 0 => Ok(()), - 1 => Err(errors.pop().unwrap()), - _ => Err(Error::Errors(errors)), - } - } else { - use crate::log; - log!("found {} overlap errors", errors.len()); - Ok(()) + errors.sort_by_key(|err| match err { + Error::OverlapError { rules, .. } => rules.first().unwrap().1.from, + _ => Pos::default(), + }); + match errors.len() { + 0 => Ok(()), + 1 => Err(errors.pop().unwrap()), + _ => Err(Error::Errors(errors)), } } diff --git a/cranelift/isle/isle/src/parser.rs b/cranelift/isle/isle/src/parser.rs index cfe0444a91..347f27f3bd 100644 --- a/cranelift/isle/isle/src/parser.rs +++ b/cranelift/isle/isle/src/parser.rs @@ -200,9 +200,9 @@ impl<'a> Parser<'a> { fn parse_pragma(&mut self) -> Result { let ident = self.parse_ident()?; - match ident.0.as_ref() { - "overlap_errors" => Ok(Pragma::OverlapErrors), - _ => Err(self.error(ident.1, format!("Unknown pragma '{}'", ident.0))), + // currently, no pragmas are defined, but the infrastructure is useful to keep around + match ident.0.as_str() { + pragma => Err(self.error(ident.1, format!("Unknown pragma '{}'", pragma))), } } diff --git a/cranelift/isle/isle/src/sema.rs b/cranelift/isle/isle/src/sema.rs index 2613d4bf14..9340729d71 100644 --- a/cranelift/isle/isle/src/sema.rs +++ b/cranelift/isle/isle/src/sema.rs @@ -200,9 +200,6 @@ pub struct TermEnv { /// defined implicit type-converter terms we can try to use to fit /// types together. pub converters: StableMap<(TypeId, TypeId), TermId>, - - /// A flag indicating whether or not overlap between rules should be considered an error. - pub overlap_errors: bool, } /// A term. @@ -789,7 +786,6 @@ impl TermEnv { term_map: StableMap::new(), rules: vec![], converters: StableMap::new(), - overlap_errors: false, }; env.collect_pragmas(defs); @@ -811,13 +807,9 @@ impl TermEnv { Ok(env) } - fn collect_pragmas(&mut self, defs: &ast::Defs) { - for def in &defs.defs { - match def { - ast::Def::Pragma(ast::Pragma::OverlapErrors) => self.overlap_errors = true, - _ => (), - } - } + fn collect_pragmas(&mut self, _: &ast::Defs) { + // currently, no pragmas are defined, but the infrastructure is useful to keep around + return; } fn collect_term_sigs(&mut self, tyenv: &mut TypeEnv, defs: &ast::Defs) {