Implicit type conversions in ISLE (#3807)

Add support for implicit type conversions to ISLE.

This feature allows the DSL user to register to the compiler that a
particular term (used as a constructor or extractor) converts from one
type to another. The compiler will then *automatically* insert this term
whenever a type mismatch involving that specific pair of types occurs.

This significantly cleans up many uses of the ISLE DSL. For example,
when defining the compiler backends, we often have newtypes like `Gpr`
around `Reg` (signifying a particular type of register); we can define
a conversion from Gpr to Reg automatically.

Conversions can also have side-effects, as long as these side-effects
are idempotent. For example, `put_value_in_reg` in a compiler backend
has the effect of marking the value as used, causing codegen to produce
it, and assigns a register to the value; but multiple invocations of
this will return the same register for the same value. Thus it is safe
to use it as an implicit conversion that may be invoked multiple times.
This is documented in the ISLE-Cranelift integration document.

This PR also adds some testing infrastructure to the ISLE compiler,
checking that "pass" tests pass through the DSL compiler, "fail" tests
do not, and "link" tests are able to generate code and link that code
with corresponding Rust code.

Cargo.lock

@@ -614,6 +614,7 @@ version = "0.81.0"
 dependencies = [
  "log",
  "miette",
+ "tempfile",
  "thiserror",
 ]
 

cranelift/docs/isle-integration.md

@@ -233,3 +233,59 @@ instructions with flags-consuming instructions, ensuring that no errant
 instructions are ever inserted between our flags-using instructions, clobbering
 their flags. See `with_flags`, `ProducesFlags`, and `ConsumesFlags` inside
 `cranelift/codegen/src/prelude.isle` for details.
+
+## Implicit type conversions
+
+ISLE supports implicit type conversions, and we will make use of these
+where possible to simplify the lowering rules. For example, we have
+`Value` and `ValueRegs`; the former denotes SSA values in CLIF, and
+the latter denotes the register or registers that hold that value in
+lowered code. Prior to introduction of implicit type conversions, we
+had many occurrences of expressions like `(value_regs r1 r2)` or
+`(value_reg r)`. Given that we have already defined a term
+`value_reg`, we can define a conversion such as
+
+```lisp
+    (convert Reg ValueRegs value_reg)
+```
+
+and the ISLE compiler will automatically insert it where the types
+imply that it is necessary. When properly defined, converters allow us
+to write rules like
+
+```lisp
+    (rule (lower (has_type (fits_in_64 ty)
+                           (iadd x y)))
+          (add ty x y))
+```
+
+### Implicit type conversions and side-effects
+
+While implicit conversions are very convenient, we take care to manage
+how they introduce invisible side-effects.
+
+Particularly important here is the fact that implicit conversions can
+occur more than once. For example, if we define `(convert Value Reg
+put_value_in_reg)`, and we emit an instruction `(add a a)` (say, to
+double the value of `a`) where `a` is a `Value`, then we will invoke
+`put_value_in_reg` twice.
+
+If this were an external constructor that performed some unique action
+per invocation, for example allocating a fresh register, then this
+would not be desirable. So we follow a convention when defining
+implicit type conversions: the conversion must be *idempotent*, i.e.,
+must return the same value if invoked more than
+once. `put_value_in_reg` in particular already has this property, so
+we are safe to use it as an implicit converter.
+
+Note that this condition is *not* the same as saying that the
+converter cannot have side-effects. It is perfectly fine for this to
+be the case, and can add significant convenience, even. The small loss
+in efficiency from invoking the converter twice is tolerable, and we
+could optimize it later if we needed to do so.
+
+Even given this, there may be times where it is still clearer to be
+explicit. For example, sinking a load is an explicit and important
+detail of some lowering patterns; while we could define an implicit
+conversion from `SinkableLoad` to `Reg`, it is probably better for now
+to retain the `(sink_load ...)` form for clarity.

cranelift/isle/docs/language-reference.md

@@ -862,6 +862,66 @@ which will, for example, expand a pattern `(A (subterm ...) _)` into
 the arguments to `A` are substituted into the extractor body and then
 this body is inlined.
 
+#### Implicit Type Conversions
+
+For convenience, ISLE allows the program to associate terms with pairs
+of types, so that type mismatches are *automatically resolved* by
+inserting that term.
+
+For example, if one is writing a rule such as
+
+```lisp
+    (decl u_to_v (U) V)
+    (rule ...)
+    
+    (decl MyTerm (T) V)
+    (rule (MyTerm t)
+          (u_to_v t))
+```
+
+the `(u_to_v t)` term would not typecheck given the ISLE language
+functionality that we have seen so far, because it expects a `U` for
+its argument but `t` has type `T`. However, if we define
+
+
+```lisp
+    (convert T U t_to_u)
+    
+    ;; For the above to be valid, `t_to_u` should be declared with the
+    ;; signature:
+    (decl t_to_u (T) U)
+    (rule ...)
+```
+
+then the DSL compiler will implicitly understand the above `MyTerm` rule as:
+
+```lisp
+    (rule (MyTerm t)
+          (u_to_v (t_to_u t)))
+```
+
+This also works in the extractor position: for example, if one writes
+
+```lisp
+    (decl defining_instruction (Inst) Value)
+    (extern extractor definining_instruction ...)
+    
+    (decl iadd (Value Value) Inst)
+    
+    (rule (lower (iadd (iadd a b) c))
+          ...))
+          
+    (convert Inst Value defining_instruction)
+```
+
+then the `(iadd (iadd a b) c)` form will be implicitly handled like
+`(iadd (defining_instruction (iadd a b)) c)`. Note that the conversion
+insertion needs to have local type context in order to find the right
+converter: so, for example, it cannot infer a target type from a
+pattern where just a variable binding occurs, even if the variable is
+used in some typed context on the right-hand side. Instead, the
+"inner" and "outer" types have to come from explicitly typed terms.
+
 #### Summary: Terms, Constructors, and Extractors
 
 We start with a `term`, which is just a schema for data:

cranelift/isle/isle/Cargo.toml

@@ -12,3 +12,6 @@ version = "0.81.0"
 log = "0.4"
 miette = "3.0.0"
 thiserror = "1.0.29"
+
+[dev-dependencies]
+tempfile = "3"

cranelift/isle/isle/build.rs

@@ -0,0 +1,33 @@
+use std::fmt::Write;
+
+fn main() {
+    println!("cargo:rerun-if-changed=build.rs");
+
+    let out_dir = std::path::PathBuf::from(
+        std::env::var_os("OUT_DIR").expect("The OUT_DIR environment variable must be set"),
+    );
+
+    let mut out = String::new();
+
+    emit_tests(&mut out, "isle_examples/pass", "run_pass");
+    emit_tests(&mut out, "isle_examples/fail", "run_fail");
+    emit_tests(&mut out, "isle_examples/link", "run_link");
+
+    let output = out_dir.join("isle_tests.rs");
+    std::fs::write(output, out).unwrap();
+}
+
+fn emit_tests(out: &mut String, dir_name: &str, runner_func: &str) {
+    for test_file in std::fs::read_dir(dir_name).unwrap() {
+        let test_file = test_file.unwrap().file_name().into_string().unwrap();
+        if !test_file.ends_with(".isle") {
+            continue;
+        }
+        let test_file_base = test_file.replace(".isle", "");
+
+        writeln!(out, "#[test]").unwrap();
+        writeln!(out, "fn test_{}_{}() {{", runner_func, test_file_base).unwrap();
+        writeln!(out, "    {}(\"{}/{}\");", runner_func, dir_name, test_file).unwrap();
+        writeln!(out, "}}").unwrap();
+    }
+}

cranelift/isle/isle/isle_examples/fail/bad_converters.isle

@@ -0,0 +1,10 @@
+(type T (enum))
+(type U (enum))
+
+(decl t_to_u_1 (T) U)
+(decl t_to_u_2 (T) U)
+
+(convert T U t_to_u_1)
+(convert T U t_to_u_2)
+
+(convert T Undef undefined_term)

cranelift/isle/isle/isle_examples/fail/converter_extractor_constructor.isle

@@ -0,0 +1,29 @@
+(type T (enum))
+(type U (enum))
+(type V (enum))
+
+(convert T U t_to_u)
+(convert U V u_to_v)
+
+(decl t_to_u (T) U)
+(decl u_to_v (U) V)
+(decl t_to_v (T) V)
+(decl v_to_t (V) T)
+
+(extern constructor t_to_u t_to_u)
+(extern extractor u_to_v u_to_v)
+(extern constructor t_to_v t_to_v_ctor)
+(extern extractor t_to_v t_to_v_etor)
+(extern extractor v_to_t v_to_u_etor)
+
+;; We should fail to find a converter here. Given only the types, we
+;; might expect u_to_v to be implicitly inserted in the RHS, but
+;; u_to_v has only an extractor, not a constructor, associated.
+(decl Test1 (U) V)
+(rule (Test1 u) u)
+
+;; We should fail to find a converter here. Given only the types, we
+;; might expect t_to_u to be implicitly inserted in the LHS, but t_to_u
+;; has only a constructor, not an extractor, associated.
+(decl Test2 (U) V)
+(rule (Test2 (v_to_t v)) v)

cranelift/isle/isle_examples/test_main.rs → cranelift/isle/isle/isle_examples/link/test_main.rs

@@ -2,8 +2,8 @@ mod test;
 
 struct Context;
 impl test::Context for Context {
-    fn get_input(&mut self, x: u32) -> Option<(test::A,)> {
-        Some((test::A::A1 { x: x + 1 },))
+    fn get_input(&mut self, x: u32) -> Option<test::A> {
+        Some(test::A::A1 { x: x + 1 })
     }
 }
 

cranelift/isle/isle/isle_examples/pass/conversions.isle

@@ -0,0 +1,28 @@
+(type T (enum (A) (B)))
+(type U (enum (C) (D)))
+(type V (enum (E) (F)))
+(type u32 (primitive u32))
+
+(convert T U t_to_u)
+(convert U T u_to_t)
+(convert U V u_to_v)
+
+(decl t_to_u (T) U)
+(decl u_to_t (U) T)
+(decl u_to_v (U) V)
+
+(rule (t_to_u (T.A)) (U.C))
+(rule (t_to_u (T.B)) (U.D))
+
+(rule (u_to_t (U.C)) (T.A))
+(rule (u_to_t (U.D)) (T.B))
+
+(rule (u_to_v (U.C)) (V.E))
+(rule (u_to_v (U.D)) (V.F))
+
+(extern extractor u_to_t u_to_t)
+      
+
+(decl X (T U) V)
+(rule (X (U.C) (U.D))
+      (U.D))

cranelift/isle/isle/src/ast.rs

@@ -21,6 +21,7 @@ pub enum Def {
     Extractor(Extractor),
     Decl(Decl),
     Extern(Extern),
+    Converter(Converter),
 }
 
 /// An identifier -- a variable, term symbol, or type.
@@ -419,3 +420,23 @@ pub enum ArgPolarity {
     /// *from* the extractor op.
     Output,
 }
+
+/// An implicit converter: the given term, which must have type
+/// (inner_ty) -> outer_ty, is used either in extractor or constructor
+/// position as appropriate when a type mismatch with the given pair
+/// of types would otherwise occur.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub struct Converter {
+    /// The term name.
+    pub term: Ident,
+    /// The "inner type": the type to convert *from*, on the
+    /// right-hand side, or *to*, on the left-hand side. Must match
+    /// the singular argument type of the term.
+    pub inner_ty: Ident,
+    /// The "outer type": the type to convert *to*, on the right-hand
+    /// side, or *from*, on the left-hand side. Must match the ret_ty
+    /// of the term.
+    pub outer_ty: Ident,
+    /// The position of this converter decl.
+    pub pos: Pos,
+}

cranelift/isle/isle/src/parser.rs

@@ -140,6 +140,7 @@ impl<'a> Parser<'a> {
             "rule" => Def::Rule(self.parse_rule()?),
             "extractor" => Def::Extractor(self.parse_etor()?),
             "extern" => Def::Extern(self.parse_extern()?),
+            "convert" => Def::Converter(self.parse_converter()?),
             s => {
                 return Err(self.error(pos, format!("Unexpected identifier: {}", s)));
             }
@@ -515,4 +516,17 @@ impl<'a> Parser<'a> {
         self.rparen()?;
         Ok(LetDef { var, ty, val, pos })
     }
+
+    fn parse_converter(&mut self) -> Result<Converter> {
+        let pos = self.pos();
+        let inner_ty = self.parse_ident()?;
+        let outer_ty = self.parse_ident()?;
+        let term = self.parse_ident()?;
+        Ok(Converter {
+            term,
+            inner_ty,
+            outer_ty,
+            pos,
+        })
+    }
 }

cranelift/isle/isle/src/sema.rs

@@ -14,8 +14,10 @@
 //! the opposite).
 
 use crate::ast;
+use crate::ast::Ident;
 use crate::error::*;
 use crate::lexer::Pos;
+use std::collections::btree_map::Entry;
 use std::collections::BTreeMap;
 use std::collections::BTreeSet;
 use std::sync::Arc;
@@ -191,6 +193,11 @@ pub struct TermEnv {
     ///
     /// This is indexed by `RuleId`.
     pub rules: Vec<Rule>,
+
+    /// Map from (inner_ty, outer_ty) pairs to term IDs, giving the
+    /// defined implicit type-converter terms we can try to use to fit
+    /// types together.
+    pub converters: BTreeMap<(TypeId, TypeId), TermId>,
 }
 
 /// A term.
@@ -775,6 +782,7 @@ impl TermEnv {
             terms: vec![],
             term_map: BTreeMap::new(),
             rules: vec![],
+            converters: BTreeMap::new(),
         };
 
         env.collect_term_sigs(tyenv, defs);
@@ -783,6 +791,8 @@ impl TermEnv {
         env.collect_constructors(tyenv, defs);
         env.collect_extractor_templates(tyenv, defs);
         tyenv.return_errors()?;
+        env.collect_converters(tyenv, defs);
+        tyenv.return_errors()?;
         env.collect_rules(tyenv, defs);
         env.check_for_undefined_decls(tyenv, defs);
         env.check_for_expr_terms_without_constructors(tyenv, defs);
@@ -1088,6 +1098,72 @@ impl TermEnv {
         }
     }
 
+    fn collect_converters(&mut self, tyenv: &mut TypeEnv, defs: &ast::Defs) {
+        for def in &defs.defs {
+            match def {
+                &ast::Def::Converter(ast::Converter {
+                    ref term,
+                    ref inner_ty,
+                    ref outer_ty,
+                    pos,
+                }) => {
+                    let inner_ty_sym = tyenv.intern_mut(inner_ty);
+                    let inner_ty_id = match tyenv.type_map.get(&inner_ty_sym) {
+                        Some(ty) => *ty,
+                        None => {
+                            tyenv.report_error(
+                                inner_ty.1,
+                                format!("Unknown inner type for converter: '{}'", inner_ty.0),
+                            );
+                            continue;
+                        }
+                    };
+
+                    let outer_ty_sym = tyenv.intern_mut(outer_ty);
+                    let outer_ty_id = match tyenv.type_map.get(&outer_ty_sym) {
+                        Some(ty) => *ty,
+                        None => {
+                            tyenv.report_error(
+                                outer_ty.1,
+                                format!("Unknown outer type for converter: '{}'", outer_ty.0),
+                            );
+                            continue;
+                        }
+                    };
+
+                    let term_sym = tyenv.intern_mut(term);
+                    let term_id = match self.term_map.get(&term_sym) {
+                        Some(term_id) => *term_id,
+                        None => {
+                            tyenv.report_error(
+                                term.1,
+                                format!("Unknown term for converter: '{}'", term.0),
+                            );
+                            continue;
+                        }
+                    };
+
+                    match self.converters.entry((inner_ty_id, outer_ty_id)) {
+                        Entry::Vacant(v) => {
+                            v.insert(term_id);
+                        }
+                        Entry::Occupied(_) => {
+                            tyenv.report_error(
+                                pos,
+                                format!(
+                                    "Converter already exists for this type pair: '{}', '{}'",
+                                    inner_ty.0, outer_ty.0
+                                ),
+                            );
+                            continue;
+                        }
+                    }
+                }
+                _ => {}
+            }
+        }
+    }
+
     fn collect_rules(&mut self, tyenv: &mut TypeEnv, defs: &ast::Defs) {
         for def in &defs.defs {
             match def {
@@ -1327,6 +1403,36 @@ impl TermEnv {
         }
     }
 
+    fn maybe_implicit_convert_pattern(
+        &self,
+        tyenv: &mut TypeEnv,
+        pattern: &ast::Pattern,
+        inner_ty: TypeId,
+        outer_ty: TypeId,
+    ) -> Option<ast::Pattern> {
+        if let Some(converter_term) = self.converters.get(&(inner_ty, outer_ty)) {
+            if self.terms[converter_term.index()].has_extractor() {
+                // This is a little awkward: we have to
+                // convert back to an Ident, to be
+                // re-resolved. The pos doesn't matter
+                // as it shouldn't result in a lookup
+                // failure.
+                let converter_term_ident = Ident(
+                    tyenv.syms[self.terms[converter_term.index()].name.index()].clone(),
+                    pattern.pos(),
+                );
+                let expanded_pattern = ast::Pattern::Term {
+                    sym: converter_term_ident,
+                    pos: pattern.pos(),
+                    args: vec![ast::TermArgPattern::Pattern(pattern.clone())],
+                };
+
+                return Some(expanded_pattern);
+            }
+        }
+        None
+    }
+
     fn translate_pattern(
         &self,
         tyenv: &mut TypeEnv,
@@ -1485,12 +1591,31 @@ impl TermEnv {
 
                 // Get the return type and arg types. Verify the
                 // expected type of this pattern, if any, against the
-                // return type of the term.
+                // return type of the term. Insert an implicit
+                // converter if needed.
                 let ret_ty = self.terms[tid.index()].ret_ty;
                 let ty = match expected_ty {
                     None => ret_ty,
                     Some(expected_ty) if expected_ty == ret_ty => ret_ty,
                     Some(expected_ty) => {
+                        // Can we do an implicit type conversion? Look
+                        // up the converter term, if any. If one has
+                        // been registered, and the term has an
+                        // extractor, then build an expanded AST node
+                        // right here and recurse on it.
+                        if let Some(expanded_pattern) =
+                            self.maybe_implicit_convert_pattern(tyenv, pat, ret_ty, expected_ty)
+                        {
+                            return self.translate_pattern(
+                                tyenv,
+                                rule_term,
+                                &expanded_pattern,
+                                Some(expected_ty),
+                                bindings,
+                                /* is_root = */ false,
+                            );
+                        }
+
                         tyenv.report_error(
                             pos,
                             format!(
@@ -1685,6 +1810,30 @@ impl TermEnv {
         }
     }
 
+    fn maybe_implicit_convert_expr(
+        &self,
+        tyenv: &mut TypeEnv,
+        expr: &ast::Expr,
+        inner_ty: TypeId,
+        outer_ty: TypeId,
+    ) -> Option<ast::Expr> {
+        // Is there a converter for this type mismatch?
+        if let Some(converter_term) = self.converters.get(&(inner_ty, outer_ty)) {
+            if self.terms[converter_term.index()].has_constructor() {
+                let converter_ident = ast::Ident(
+                    tyenv.syms[self.terms[converter_term.index()].name.index()].clone(),
+                    expr.pos(),
+                );
+                return Some(ast::Expr::Term {
+                    sym: converter_ident,
+                    pos: expr.pos(),
+                    args: vec![expr.clone()],
+                });
+            }
+        }
+        None
+    }
+
     fn translate_expr(
         &self,
         tyenv: &mut TypeEnv,
@@ -1712,12 +1861,30 @@ impl TermEnv {
 
                 // Get the return type and arg types. Verify the
                 // expected type of this pattern, if any, against the
-                // return type of the term.
+                // return type of the term, and determine whether we
+                // are doing an implicit conversion. Report an error
+                // if types don't match and no conversion is possible.
                 let ret_ty = self.terms[tid.index()].ret_ty;
-                if ret_ty != ty {
-                    tyenv.report_error(pos, format!("Mismatched types: expression expects type '{}' but term has return type '{}'", tyenv.types[ty.index()].name(tyenv), tyenv.types[ret_ty.index()].name(tyenv)));
+                let ty = if ret_ty != ty {
+                    // Is there a converter for this type mismatch?
+                    if let Some(expanded_expr) =
+                        self.maybe_implicit_convert_expr(tyenv, expr, ret_ty, ty)
+                    {
+                        return self.translate_expr(tyenv, &expanded_expr, ty, bindings);
                     }
 
+                    tyenv.report_error(
+                        pos,
+                        format!("Mismatched types: expression expects type '{}' but term has return type '{}'",
+                                tyenv.types[ty.index()].name(tyenv),
+                                tyenv.types[ret_ty.index()].name(tyenv)));
+
+                    // Keep going, to discover more errors.
+                    ret_ty
+                } else {
+                    ty
+                };
+
                 // Check that we have the correct argument count.
                 if self.terms[tid.index()].arg_tys.len() != args.len() {
                     tyenv.report_error(
@@ -1754,8 +1921,15 @@ impl TermEnv {
                     Some(bv) => bv,
                 };
 
-                // Verify type.
+                // Verify type. Maybe do an implicit conversion.
                 if bv.ty != ty {
+                    // Is there a converter for this type mismatch?
+                    if let Some(expanded_expr) =
+                        self.maybe_implicit_convert_expr(tyenv, expr, bv.ty, ty)
+                    {
+                        return self.translate_expr(tyenv, &expanded_expr, ty, bindings);
+                    }
+
                     tyenv.report_error(
                         pos,
                         format!(

cranelift/isle/isle/tests/run_tests.rs

@@ -0,0 +1,54 @@
+//! Helper for autogenerated unit tests.
+
+use cranelift_isle::error::Result;
+use cranelift_isle::{compile, lexer, parser};
+
+fn build(filename: &str) -> Result<String> {
+    let lexer = lexer::Lexer::from_files(vec![filename])?;
+    let defs = parser::parse(lexer)?;
+    compile::compile(&defs)
+}
+
+pub fn run_pass(filename: &str) {
+    assert!(build(filename).is_ok());
+}
+
+pub fn run_fail(filename: &str) {
+    assert!(build(filename).is_err());
+}
+
+pub fn run_link(isle_filename: &str) {
+    let tempdir = tempfile::tempdir().unwrap();
+    let code = build(isle_filename).unwrap();
+
+    let isle_filename_base = std::path::Path::new(isle_filename)
+        .file_stem()
+        .unwrap()
+        .to_str()
+        .unwrap()
+        .to_string();
+    let isle_generated_code = tempdir
+        .path()
+        .to_path_buf()
+        .join(isle_filename_base.clone() + ".rs");
+    std::fs::write(isle_generated_code, code).unwrap();
+
+    let rust_filename = isle_filename.replace(".isle", "").to_string() + "_main.rs";
+    let rust_filename_base = std::path::Path::new(&rust_filename).file_name().unwrap();
+    let rust_driver = tempdir.path().to_path_buf().join(&rust_filename_base);
+    println!("copying {} to {:?}", rust_filename, rust_driver);
+    std::fs::copy(&rust_filename, &rust_driver).unwrap();
+
+    let output = tempdir.path().to_path_buf().join("out");
+
+    let mut rustc = std::process::Command::new("rustc")
+        .arg(&rust_driver)
+        .arg("-o")
+        .arg(output)
+        .spawn()
+        .unwrap();
+    assert!(rustc.wait().unwrap().success());
+}
+
+// Generated by build.rs.
+include!(concat!(env!("OUT_DIR"), "/isle_tests.rs"));

cranelift/isle/islec/src/main.rs

@@ -15,7 +15,7 @@ struct Opts {
     output: Option<PathBuf>,
 
     /// The input ISLE DSL source files.
-    #[structopt(parse(from_os_str))]
+    #[structopt(parse(from_os_str), required(true))]
     inputs: Vec<PathBuf>,
 }