[meta] Simplify and comment instruction building a bit;

cranelift/codegen/meta/src/cdsl/formats.rs

@@ -189,7 +189,9 @@ impl FormatRegistry {
             if operand.is_value() {
                 num_values += 1;
             }
-            has_varargs = has_varargs || operand.is_varargs();
+            if !has_varargs {
+                has_varargs = operand.is_varargs();
+            }
             if let Some(imm_key) = operand.kind.imm_key() {
                 imm_keys.push(imm_key);
             }

cranelift/codegen/meta/src/cdsl/instructions.rs

@@ -113,9 +113,12 @@ pub struct InstructionContent {
     /// polymorphic, set otherwise.
     pub polymorphic_info: Option<PolymorphicInfo>,
 
+    /// Indices in operands_in of input operands that are values.
     pub value_opnums: Vec<usize>,
-    pub value_results: Vec<usize>,
+    /// Indices in operands_in of input operands that are immediates or entities.
     pub imm_opnums: Vec<usize>,
+    /// Indices in operands_out of output operands that are values.
+    pub value_results: Vec<usize>,
 
     /// True for instructions that terminate the EBB.
     pub is_terminator: bool,
@@ -332,8 +335,6 @@ impl InstructionBuilder {
         let operands_in = self.operands_in.unwrap_or_else(Vec::new);
         let operands_out = self.operands_out.unwrap_or_else(Vec::new);
 
-        let format_index = format_registry.lookup(&operands_in);
-
         let mut value_opnums = Vec::new();
         let mut imm_opnums = Vec::new();
         for (i, op) in operands_in.iter().enumerate() {
@@ -346,13 +347,13 @@ impl InstructionBuilder {
             }
         }
 
-        let mut value_results = Vec::new();
+        let value_results = operands_out
-        for (i, op) in operands_out.iter().enumerate() {
+            .iter()
-            if op.is_value() {
+            .enumerate()
-                value_results.push(i);
+            .filter_map(|(i, op)| if op.is_value() { Some(i) } else { None })
-            }
+            .collect();
-        }
 
+        let format_index = format_registry.lookup(&operands_in);
         let format = format_registry.get(format_index);
         let polymorphic_info =
             verify_polymorphic(&operands_in, &operands_out, &format, &value_opnums);
@@ -461,12 +462,8 @@ fn verify_polymorphic(
     }
 
     // Verify the use of type variables.
-    let mut use_typevar_operand = false;
-    let mut ctrl_typevar = None;
-    let mut other_typevars = None;
-    let mut maybe_error_message = None;
-
     let tv_op = format.typevar_operand;
+    let mut maybe_error_message = None;
     if let Some(tv_op) = tv_op {
         if tv_op < value_opnums.len() {
             let op_num = value_opnums[tv_op];
@@ -475,11 +472,13 @@ fn verify_polymorphic(
             if (free_typevar.is_some() && tv == &free_typevar.unwrap())
                 || tv.singleton_type().is_some()
             {
-                match verify_ctrl_typevar(tv, &value_opnums, &operands_in, &operands_out) {
+                match is_ctrl_typevar_candidate(tv, &operands_in, &operands_out) {
-                    Ok(typevars) => {
+                    Ok(other_typevars) => {
-                        other_typevars = Some(typevars);
+                        return Some(PolymorphicInfo {
-                        ctrl_typevar = Some(tv.clone());
+                            use_typevar_operand: true,
-                        use_typevar_operand = true;
+                            ctrl_typevar: tv.clone(),
+                            other_typevars,
+                        });
                     }
                     Err(error_message) => {
                         maybe_error_message = Some(error_message);
@@ -489,33 +488,32 @@ fn verify_polymorphic(
         }
     };
 
-    if !use_typevar_operand {
+    // If we reached here, it means the type variable indicated as the typevar operand couldn't
-        if operands_out.len() == 0 {
+    // control every other input and output type variable. We need to look at the result type
-            match maybe_error_message {
+    // variables.
-                Some(msg) => panic!(msg),
+    if operands_out.len() == 0 {
-                None => panic!("typevar_operand must be a free type variable"),
+        // No result means no other possible type variable, so it's a type inference failure.
-            }
+        match maybe_error_message {
+            Some(msg) => panic!(msg),
+            None => panic!("typevar_operand must be a free type variable"),
         }
-
-        let tv = operands_out[0].type_var().unwrap();
-        let free_typevar = tv.free_typevar();
-        if free_typevar.is_some() && tv != &free_typevar.unwrap() {
-            panic!("first result must be a free type variable");
-        }
-
-        other_typevars =
-            Some(verify_ctrl_typevar(tv, &value_opnums, &operands_in, &operands_out).unwrap());
-        ctrl_typevar = Some(tv.clone());
     }
 
-    // rustc is not capable to determine this statically, so enforce it with options.
+    // Otherwise, try to infer the controlling type variable by looking at the first result.
-    assert!(ctrl_typevar.is_some());
+    let tv = operands_out[0].type_var().unwrap();
-    assert!(other_typevars.is_some());
+    let free_typevar = tv.free_typevar();
+    if free_typevar.is_some() && tv != &free_typevar.unwrap() {
+        panic!("first result must be a free type variable");
+    }
+
+    // At this point, if the next unwrap() fails, it means the output type couldn't be used as a
+    // controlling type variable either; panicking is the right behavior.
+    let other_typevars = is_ctrl_typevar_candidate(tv, &operands_in, &operands_out).unwrap();
 
     Some(PolymorphicInfo {
-        use_typevar_operand,
+        use_typevar_operand: false,
-        ctrl_typevar: ctrl_typevar.unwrap(),
+        ctrl_typevar: tv.clone(),
-        other_typevars: other_typevars.unwrap(),
+        other_typevars,
     })
 }
 
@@ -527,57 +525,51 @@ fn verify_polymorphic(
 ///
 /// All polymorphic results must be derived from `ctrl_typevar`.
 ///
-/// Return a vector of other type variables used, or panics.
+/// Return a vector of other type variables used, or a string explaining what went wrong.
-fn verify_ctrl_typevar(
+fn is_ctrl_typevar_candidate(
     ctrl_typevar: &TypeVar,
-    value_opnums: &Vec<usize>,
     operands_in: &Vec<Operand>,
     operands_out: &Vec<Operand>,
 ) -> Result<Vec<TypeVar>, String> {
     let mut other_typevars = Vec::new();
 
     // Check value inputs.
-    for &op_num in value_opnums {
+    for input in operands_in {
-        let typ = operands_in[op_num].type_var();
+        if !input.is_value() {
+            continue;
+        }
 
-        let tv = if let Some(typ) = typ {
+        let typ = input.type_var().unwrap();
-            typ.free_typevar()
+        let free_typevar = typ.free_typevar();
-        } else {
-            None
-        };
 
         // Non-polymorphic or derived from ctrl_typevar is OK.
-        let tv = match tv {
+        if free_typevar.is_none() {
-            Some(tv) => {
+            continue;
-                if &tv == ctrl_typevar {
+        }
-                    continue;
+        let free_typevar = free_typevar.unwrap();
-                }
+        if &free_typevar == ctrl_typevar {
-                tv
+            continue;
-            }
+        }
-            None => continue,
-        };
 
         // No other derived typevars allowed.
-        if typ.is_some() && typ.unwrap() != &tv {
+        if typ != &free_typevar {
             return Err(format!(
-                "{:?}: type variable {} must be derived from {:?}",
+                "{:?}: type variable {} must be derived from {:?} while it is derived from {:?}",
-                operands_in[op_num],
+                input, typ.name, ctrl_typevar, free_typevar
-                typ.unwrap().name,
-                ctrl_typevar
             ));
         }
 
         // Other free type variables can only be used once each.
         for other_tv in &other_typevars {
-            if &tv == other_tv {
+            if &free_typevar == other_tv {
                 return Err(format!(
-                    "type variable {} can't be used more than once",
+                    "non-controlling type variable {} can't be used more than once",
-                    tv.name
+                    free_typevar.name
                 ));
             }
         }
 
-        other_typevars.push(tv);
+        other_typevars.push(free_typevar);
     }
 
     // Check outputs.
@@ -587,10 +579,10 @@ fn verify_ctrl_typevar(
         }
 
         let typ = result.type_var().unwrap();
-        let tv = typ.free_typevar();
+        let free_typevar = typ.free_typevar();
 
-        // Non-polymorphic or derived form ctrl_typevar is OK.
+        // Non-polymorphic or derived from ctrl_typevar is OK.
-        if tv.is_none() || &tv.unwrap() == ctrl_typevar {
+        if free_typevar.is_none() || &free_typevar.unwrap() == ctrl_typevar {
             continue;
         }