Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ISLE: Enable the overlap checker #5011

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 0 additions & 2 deletions cranelift/codegen/src/isa/aarch64/lower.isle
Original file line number Diff line number Diff line change
@@ -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)
Expand Down
2 changes: 0 additions & 2 deletions cranelift/codegen/src/isa/riscv64/lower.isle
Original file line number Diff line number Diff line change
@@ -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)
Expand Down
2 changes: 0 additions & 2 deletions cranelift/codegen/src/isa/s390x/lower.isle
Original file line number Diff line number Diff line change
@@ -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)
Expand Down
3 changes: 0 additions & 3 deletions cranelift/codegen/src/isa/x64/lower.isle
Original file line number Diff line number Diff line change
@@ -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)
Expand Down
6 changes: 3 additions & 3 deletions cranelift/isle/isle/isle_examples/link/iflets.isle
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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)
Original file line number Diff line number Diff line change
Expand Up @@ -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 _)
Expand Down
2 changes: 1 addition & 1 deletion cranelift/isle/isle/isle_examples/pass/test3.isle
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions cranelift/isle/isle/isle_examples/pass/tutorial.isle
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down

This file was deleted.

This file was deleted.

3 changes: 1 addition & 2 deletions cranelift/isle/isle/src/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,7 @@ pub struct Ident(pub String, pub Pos);
/// Pragmas parsed with the `(pragma <ident>)` 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.
Expand Down
22 changes: 8 additions & 14 deletions cranelift/isle/isle/src/overlap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
}
}

Expand Down
6 changes: 3 additions & 3 deletions cranelift/isle/isle/src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -200,9 +200,9 @@ impl<'a> Parser<'a> {

fn parse_pragma(&mut self) -> Result<Pragma> {
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... ah, and I see you have exactly that comment here :-) (Mentioning this in both places is still good, I think.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll copy this one over to the enum definition as well, thanks for catching that!

match ident.0.as_str() {
pragma => Err(self.error(ident.1, format!("Unknown pragma '{}'", pragma))),
}
}

Expand Down
14 changes: 3 additions & 11 deletions cranelift/isle/isle/src/sema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -789,7 +786,6 @@ impl TermEnv {
term_map: StableMap::new(),
rules: vec![],
converters: StableMap::new(),
overlap_errors: false,
};

env.collect_pragmas(defs);
Expand All @@ -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) {
Expand Down