From 660d0b8ed0ceee4542081f026362c0da23584bf1 Mon Sep 17 00:00:00 2001 From: Sean Billig Date: Sun, 30 Jun 2024 13:49:38 -0700 Subject: [PATCH 1/4] Rework parser Value handling; fix test type errors --- .../fixtures/adce/all_dests_remove.sntn | 8 +- .../fixtures/adce/whole_loop_removed.sntn | 2 +- .../fixtures/gvn/fold_predicted_value.sntn | 6 +- .../filecheck/fixtures/insn_simplify/neg.sntn | 4 +- crates/interpreter/src/state.rs | 12 +- crates/ir/src/types.rs | 4 + crates/parser/Cargo.toml | 1 + crates/parser/src/error.rs | 13 ++ crates/parser/src/lib.rs | 160 +++++++++++------- crates/parser/test_files/errors/type_err.snap | 16 ++ crates/parser/test_files/errors/type_err.sntn | 13 ++ .../parser/test_files/errors/undefined.snap | 20 ++- 12 files changed, 175 insertions(+), 84 deletions(-) create mode 100644 crates/parser/test_files/errors/type_err.snap create mode 100644 crates/parser/test_files/errors/type_err.sntn diff --git a/crates/filecheck/fixtures/adce/all_dests_remove.sntn b/crates/filecheck/fixtures/adce/all_dests_remove.sntn index 42dfedf4..fc7ebcd6 100644 --- a/crates/filecheck/fixtures/adce/all_dests_remove.sntn +++ b/crates/filecheck/fixtures/adce/all_dests_remove.sntn @@ -29,15 +29,15 @@ func public %all_dests_removed() -> i8 { } # check: block0: -# nextln: v1.i32 = add v0 v0; +# nextln: v1.i8 = add v0 v0; # nextln: jump block3; # nextln: # nextln: block3: # nextln: return v1; -func public %all_dests_removed2(v0.i32) -> i8 { +func public %all_dests_removed2(v0.i8) -> i8 { block0: - v1.i32 = add v0 v0; - br_table v0 (v1 block1) (2.i32 block2); + v1.i8 = add v0 v0; + br_table v0 (v1 block1) (2.i8 block2); block1: v3.i8 = add v0 -10.i8; diff --git a/crates/filecheck/fixtures/adce/whole_loop_removed.sntn b/crates/filecheck/fixtures/adce/whole_loop_removed.sntn index 2467af5e..f51d49d5 100644 --- a/crates/filecheck/fixtures/adce/whole_loop_removed.sntn +++ b/crates/filecheck/fixtures/adce/whole_loop_removed.sntn @@ -5,7 +5,7 @@ target = "evm-ethereum-london" # nextln: return 1.i8; func public %whole_loop_removed() -> i8 { block0: - v0.i1 = or 1.i8 0.i8; + v0.i8 = or 1.i8 0.i8; v1.i8 = sext v0; jump block2; diff --git a/crates/filecheck/fixtures/gvn/fold_predicted_value.sntn b/crates/filecheck/fixtures/gvn/fold_predicted_value.sntn index e06f0501..750255bf 100644 --- a/crates/filecheck/fixtures/gvn/fold_predicted_value.sntn +++ b/crates/filecheck/fixtures/gvn/fold_predicted_value.sntn @@ -4,15 +4,15 @@ target = "evm-ethereum-london" # nextln: return 1.i1; # check: block2: # nextln: return 0.i1; -func public %fold_with_predicted_value(v0.i1) -> i8 { +func public %fold_with_predicted_value(v0.i1) -> i1 { block0: br v0 block1 block2; block1: - v1.i8 = or v0 v0; + v1.i1 = or v0 v0; return v1; block2: - v2.i8 = or v0 v0; + v2.i1 = or v0 v0; return v2; } diff --git a/crates/filecheck/fixtures/insn_simplify/neg.sntn b/crates/filecheck/fixtures/insn_simplify/neg.sntn index 19c6b97b..7d642d2b 100644 --- a/crates/filecheck/fixtures/insn_simplify/neg.sntn +++ b/crates/filecheck/fixtures/insn_simplify/neg.sntn @@ -13,7 +13,7 @@ func public %neg0(v0.i8) -> i8 { # check: v2.i16 = add v0 1.i16; func public %neg1(v0.i16) -> i16 { block0: - v1.i8 = not v0; - v2.i8 = neg v1; + v1.i16 = not v0; + v2.i16 = neg v1; return v2; } diff --git a/crates/interpreter/src/state.rs b/crates/interpreter/src/state.rs index 1a662665..f8322f41 100644 --- a/crates/interpreter/src/state.rs +++ b/crates/interpreter/src/state.rs @@ -321,7 +321,7 @@ mod test { v0.i16 = add 3.i16 4.i16; v1.i16 = sub v0 1.i16; v2.i16 = udiv v1 2.i16; - v3.i8 = sdiv v2 65535.i16; + v3.i16 = sdiv v2 65535.i16; return v3; } "; @@ -380,7 +380,7 @@ mod test { block0: v0.*i32 = alloca i32; store @memory v0 1.i32; - v1.*i32 = load @memory v0; + v1.i32 = load @memory v0; return v1; } "; @@ -559,10 +559,10 @@ mod test { let input = " target = \"evm-ethereum-london\" - func private %test() -> *i1 { + func private %test() -> **i32 { block0: v0.*[*i32; 3] = alloca [*i32; 3]; - v1.*i32 = gep v0 2.i8; + v1.**i32 = gep v0 2.i8; return v1; } "; @@ -581,10 +581,10 @@ mod test { type %s1 = {i32, [i16; 3], [i8; 2]}; - func private %test() -> *i1 { + func private %test() -> *i8 { block0: v0.*%s1 = alloca %s1; - v1.*i1 = gep v0 2.i8 1.i8; + v1.*i8 = gep v0 2.i8 1.i8; return v1; } "; diff --git a/crates/ir/src/types.rs b/crates/ir/src/types.rs index 5dae308b..389bbfd1 100644 --- a/crates/ir/src/types.rs +++ b/crates/ir/src/types.rs @@ -232,6 +232,10 @@ impl Type { Self::I1 | Self::I8 | Self::I16 | Self::I32 | Self::I64 | Self::I128 | Self::I256 ) } + + pub fn to_string(&self, dfg: &DataFlowGraph) -> String { + DisplayType { ty: *self, dfg }.to_string() + } } impl cmp::PartialOrd for Type { diff --git a/crates/parser/Cargo.toml b/crates/parser/Cargo.toml index 9444e354..9104c8f4 100644 --- a/crates/parser/Cargo.toml +++ b/crates/parser/Cargo.toml @@ -24,6 +24,7 @@ annotate-snippets = "0.11.4" rustc-hash = "2.0.0" bimap = "0.6.3" derive_more = { version = "=1.0.0-beta.6", default-features = false, features = ["debug"] } +smallvec = "1.13.2" [dev-dependencies] dir-test = "0.3" diff --git a/crates/parser/src/error.rs b/crates/parser/src/error.rs index 0d81a124..b18f7eda 100644 --- a/crates/parser/src/error.rs +++ b/crates/parser/src/error.rs @@ -13,6 +13,11 @@ pub enum Error { SyntaxError(pest::error::Error), Undefined(UndefinedKind, Span), DuplicateValueName(SmolStr, Span), + TypeMismatch { + specified: SmolStr, + inferred: SmolStr, + span: Span, + }, } #[derive(Debug)] @@ -35,6 +40,7 @@ impl Error { pest::error::InputLocation::Pos(p) => Span(p as u32, p as u32), pest::error::InputLocation::Span((s, e)) => Span(s as u32, e as u32), }, + Error::TypeMismatch { span, .. } => *span, } } @@ -56,6 +62,13 @@ impl Error { UndefinedKind::Value(name) => format!("undefined value: `{name}`"), }, Error::DuplicateValueName(name, _) => format!("value name `{name}` is already defined"), + Error::TypeMismatch { + specified, + inferred, + .. + } => format!( + "type mismatch: value declared as `{specified}`, but inferred type is `{inferred}`", + ), }; let snippet = Level::Error.title("parse error").snippet( Snippet::source(content) diff --git a/crates/parser/src/lib.rs b/crates/parser/src/lib.rs index 2ca93875..7629b066 100644 --- a/crates/parser/src/lib.rs +++ b/crates/parser/src/lib.rs @@ -1,4 +1,4 @@ -use ast::ValueDeclaration; +use ast::{StmtKind, ValueDeclaration}; use cranelift_entity::SecondaryMap; use ir::{ self, @@ -7,9 +7,10 @@ use ir::{ ir_writer::DebugProvider, isa::IsaBuilder, module::{FuncRef, ModuleCtx}, - Module, Signature, + InsnData, Module, Signature, }; use rustc_hash::{FxHashMap, FxHashSet, FxHasher}; +use smallvec::SmallVec; use smol_str::SmolStr; use std::hash::BuildHasherDefault; use syntax::Spanned; @@ -119,7 +120,6 @@ impl DebugProvider for DebugInfo { struct BuildCtx { errors: Vec, blocks: FxHashSet, - undefined: FxHashMap, value_names: FxHashMap>, func_value_names: Bimap, } @@ -132,11 +132,17 @@ impl BuildCtx { func: &ast::Func, ) -> ModuleBuilder { self.blocks.clear(); - self.undefined.clear(); for (i, ValueDeclaration(name, _ty)) in func.signature.params.iter().enumerate() { let value = fb.func.arg_values[i]; - self.name_value(&mut fb.func, value, name); + self.name_value(value, name); + } + + for stmt in func.blocks.iter().flat_map(|b| b.stmts.iter()) { + if let StmtKind::Define(ValueDeclaration(name, ty), _) = &stmt.kind { + let ty = self.type_(&mut fb.module_builder, ty); + self.declare_value(&mut fb.func, name, ty); + } } // collect all defined block ids @@ -155,63 +161,88 @@ impl BuildCtx { for stmt in &block.stmts { match &stmt.kind { - ast::StmtKind::Define(ValueDeclaration(val, ty), expr) => { - let ty = self.type_(&mut fb.module_builder, ty); + ast::StmtKind::Define(ValueDeclaration(name, type_), expr) => { + let ty = self.type_(&mut fb.module_builder, type_); + let err_count = self.errors.len(); - let result_val = match expr { + let insn_data = match expr { ast::Expr::Binary(op, lhs, rhs) => { let lhs = self.value(&mut fb, lhs); let rhs = self.value(&mut fb, rhs); - fb.binary_op(*op, lhs, rhs) + InsnData::Binary { + code: *op, + args: [lhs, rhs], + } } ast::Expr::Unary(op, val) => { let val = self.value(&mut fb, val); - fb.unary_op(*op, val) + InsnData::Unary { + code: *op, + args: [val], + } } ast::Expr::Cast(op, val) => { let val = self.value(&mut fb, val); - fb.cast_op(*op, val, ty) + InsnData::Cast { + code: *op, + args: [val], + ty, + } } ast::Expr::Load(location, addr) => { let addr = self.value(&mut fb, addr); - match location { - ir::DataLocationKind::Memory => fb.memory_load(addr), - ir::DataLocationKind::Storage => fb.storage_load(addr), + InsnData::Load { + args: [addr], + loc: *location, } } ast::Expr::Alloca(ty) => { let ty = self.type_(&mut fb.module_builder, ty); - fb.alloca(ty) + InsnData::Alloca { ty } } ast::Expr::Call(ast::Call(name, args)) => { let func = self.func_ref(&mut fb.module_builder, name); - let args = args - .iter() - .map(|val| self.value(&mut fb, val)) - .collect::>(); - fb.call(func, &args).unwrap() + let args: smallvec::SmallVec<[ir::Value; 8]> = + args.iter().map(|val| self.value(&mut fb, val)).collect(); + + let sig = fb.module_builder.get_sig(func).clone(); + let ret_ty = sig.ret_ty(); + fb.func.callees.insert(func, sig); + + InsnData::Call { func, args, ret_ty } } ast::Expr::Gep(vals) => { - let vals = vals - .iter() - .map(|val| self.value(&mut fb, val)) - .collect::>(); - fb.gep(&vals).unwrap() + let args: SmallVec<[ir::Value; 8]> = + vals.iter().map(|val| self.value(&mut fb, val)).collect(); + InsnData::Gep { args } } - ast::Expr::Phi(vals) => { - let args = vals + ast::Expr::Phi(vals) => InsnData::Phi { + values: vals .iter() - .map(|(val, block)| { - let b = self.block(block); - let v = self.value(&mut fb, val); - (v, b) - }) - .collect::>(); - fb.phi(ty, &args) - } + .map(|(val, _)| self.value(&mut fb, val)) + .collect(), + blocks: vals.iter().map(|(_, block)| self.block(block)).collect(), + ty, + }, }; - self.name_value(&mut fb.func, result_val, val) + + // Report declared type mismatch if no error has been reported for this stmt + let inferred_ty = insn_data.result_type(&fb.func.dfg).unwrap(); + if self.errors.len() == err_count && ty != inferred_ty { + self.errors.push(Error::TypeMismatch { + specified: ty.to_string(&fb.func.dfg).into(), + inferred: inferred_ty.to_string(&fb.func.dfg).into(), + span: type_.span, + }); + } + + // xxx cleanup + let value = *self.func_value_names.get_by_right(&name.string).unwrap(); + let insn = fb.cursor.insert_insn_data(&mut fb.func, insn_data); + fb.func.dfg.values[value] = ir::ValueData::Insn { insn, ty }; + fb.cursor.attach_result(&mut fb.func, insn, value); + fb.cursor.set_location(CursorLocation::At(insn)); } ast::StmtKind::Store(loc, addr, val) => { let addr = self.value(&mut fb, addr); @@ -262,11 +293,6 @@ impl BuildCtx { } } - for (val, span) in self.undefined.drain() { - let name = self.func_value_names.get_by_left(&val).unwrap(); - self.errors - .push(Error::Undefined(UndefinedKind::Value(name.clone()), span)); - } let names = std::mem::take(&mut self.func_value_names); self.value_names.insert(func_ref, names); fb.seal_all(); @@ -292,37 +318,45 @@ impl BuildCtx { block } - pub fn name_value(&mut self, func: &mut ir::Function, value: ir::Value, name: &ast::ValueName) { - if let Some(v) = self.func_value_names.get_by_right(&name.string) { - if self.undefined.remove(v).is_some() { - func.dfg.change_to_alias(*v, value); - } else { - self.errors - .push(Error::DuplicateValueName(name.string.clone(), name.span)); - } + fn declare_value(&mut self, func: &mut ir::Function, name: &ast::ValueName, ty: ir::Type) { + // Abusing Immediate here; we just need a dummy value with a given type. + // The ValueData will be replaced when create the Insn that defines the value. + let value = func.dfg.make_value(ir::ValueData::Immediate { + imm: ir::Immediate::I128(424242), + ty, + }); + if self + .func_value_names + .insert_no_overwrite(value, name.string.clone()) + .is_err() + { + self.errors + .push(Error::DuplicateValueName(name.string.clone(), name.span)); } - self.func_value_names.insert(value, name.string.clone()); } - pub fn get_named_value(&mut self, func: &mut ir::Function, name: &ast::ValueName) -> ir::Value { - if let Some(v) = self.func_value_names.get_by_right(&name.string).copied() { - v - } else { - let v = func.dfg.make_value(ir::ValueData::Immediate { - imm: ir::Immediate::I128(424242), - ty: ir::Type::I128, - }); - - self.undefined.insert(v, name.span); - self.name_value(func, v, name); - v + fn name_value(&mut self, value: ir::Value, name: &ast::ValueName) { + if self.func_value_names.contains_right(&name.string) { + self.errors + .push(Error::DuplicateValueName(name.string.clone(), name.span)); } + self.func_value_names.insert(value, name.string.clone()); } fn value(&mut self, fb: &mut FunctionBuilder, val: &ast::Value) -> ir::Value { match &val.kind { ast::ValueKind::Immediate(imm) => fb.make_imm_value(*imm), - ast::ValueKind::Named(v) => self.get_named_value(&mut fb.func, v), + ast::ValueKind::Named(name) => self + .func_value_names + .get_by_right(&name.string) + .copied() + .unwrap_or_else(|| { + self.errors.push(Error::Undefined( + UndefinedKind::Value(name.string.clone()), + name.span, + )); + ir::Value(0) + }), ast::ValueKind::Error => unreachable!(), } } diff --git a/crates/parser/test_files/errors/type_err.snap b/crates/parser/test_files/errors/type_err.snap new file mode 100644 index 00000000..f1c1fc43 --- /dev/null +++ b/crates/parser/test_files/errors/type_err.snap @@ -0,0 +1,16 @@ +--- +source: crates/parser/tests/errors.rs +expression: s +input_file: crates/parser/test_files/errors/type_err.sntn +--- +error: parse error + --> type_err.sntn:4:12 + | +4 | v0.i16 = add 2.i8 3.i8; + | ^^^ type mismatch: value declared as `i16`, but inferred type is `i8` + |error: parse error + --> type_err.sntn:5:12 + | +5 | v1.i8 = call %foo; + | ^^ type mismatch: value declared as `i8`, but inferred type is `i16` + | diff --git a/crates/parser/test_files/errors/type_err.sntn b/crates/parser/test_files/errors/type_err.sntn new file mode 100644 index 00000000..bc60c213 --- /dev/null +++ b/crates/parser/test_files/errors/type_err.sntn @@ -0,0 +1,13 @@ +target = "evm-ethereum-london" + +func public %main() -> i8 { + block0: + v0.i16 = add 2.i8 3.i8; + v1.i8 = call %foo; + return 1.i8; +} + +func %foo() -> i16 { + block0: + return 100.i16; +} diff --git a/crates/parser/test_files/errors/undefined.snap b/crates/parser/test_files/errors/undefined.snap index 343d20f9..1e318052 100644 --- a/crates/parser/test_files/errors/undefined.snap +++ b/crates/parser/test_files/errors/undefined.snap @@ -14,13 +14,23 @@ error: parse error 6 | v0.i8 = call %foo 100.i8; | ^^^^ undefined function: `%foo` |error: parse error - --> undefined.sntn:9:14 - | -9 | jump block2; - | ^^^^^^ undefined block: `block2` - |error: parse error --> undefined.sntn:7:24 | 7 | v2.i8 = add v0 v1; | ^^ undefined value: `v1` + |error: parse error + --> undefined.sntn:8:21 + | +8 | v3.i8 = add v1 v1; + | ^^ undefined value: `v1` + |error: parse error + --> undefined.sntn:8:24 + | +8 | v3.i8 = add v1 v1; + | ^^ undefined value: `v1` + |error: parse error + --> undefined.sntn:9:14 + | +9 | jump block2; + | ^^^^^^ undefined block: `block2` | From 45a065f0097c0951376de8e845a11302f9747873 Mon Sep 17 00:00:00 2001 From: Sean Billig Date: Sun, 30 Jun 2024 14:38:41 -0700 Subject: [PATCH 2/4] Remove ValueData::Alias --- crates/codegen/src/optim/gvn.rs | 2 +- crates/codegen/src/optim/sccp.rs | 2 -- crates/codegen/src/optim/simplify_impl.rs | 10 +++---- crates/ir/src/dfg.rs | 32 +++++------------------ crates/ir/src/ir_writer.rs | 2 +- crates/ir/src/value.rs | 2 -- 6 files changed, 14 insertions(+), 36 deletions(-) diff --git a/crates/codegen/src/optim/gvn.rs b/crates/codegen/src/optim/gvn.rs index de476f81..790c79eb 100644 --- a/crates/codegen/src/optim/gvn.rs +++ b/crates/codegen/src/optim/gvn.rs @@ -504,7 +504,7 @@ impl GvnSolver { value: Value, edge: Edge, ) -> Value { - let mut rep_value = self.leader(func.dfg.resolve_alias(value)); + let mut rep_value = self.leader(value); if let Some(inferred_value) = self.infer_value_impl(edge, rep_value) { rep_value = inferred_value; diff --git a/crates/codegen/src/optim/sccp.rs b/crates/codegen/src/optim/sccp.rs index f6c47161..84472ee0 100644 --- a/crates/codegen/src/optim/sccp.rs +++ b/crates/codegen/src/optim/sccp.rs @@ -140,8 +140,6 @@ impl SccpSolver { for (i, from) in func.dfg.phi_blocks(insn).iter().enumerate() { if self.is_reachable(func, *from, block) { let phi_arg = func.dfg.insn_arg(insn, i); - let phi_arg = func.dfg.resolve_alias(phi_arg); - let v_cell = self.lattice[phi_arg]; eval_result = eval_result.join(v_cell); } diff --git a/crates/codegen/src/optim/simplify_impl.rs b/crates/codegen/src/optim/simplify_impl.rs index a5e776d8..3447c5d4 100644 --- a/crates/codegen/src/optim/simplify_impl.rs +++ b/crates/codegen/src/optim/simplify_impl.rs @@ -17,7 +17,7 @@ use generated_code::{Context, SimplifyRawResult}; pub fn simplify_insn(dfg: &mut DataFlowGraph, insn: Insn) -> Option { if dfg.is_phi(insn) { - return simplify_phi(dfg, dfg.insn_data(insn)); + return simplify_phi(dfg.insn_data(insn)); } let mut ctx = SimplifyContext::new(dfg); @@ -27,7 +27,7 @@ pub fn simplify_insn(dfg: &mut DataFlowGraph, insn: Insn) -> Option Option { if matches!(data, InsnData::Phi { .. }) { - return simplify_phi(dfg, &data); + return simplify_phi(&data); } let mut ctx = SimplifyContext::new(dfg); @@ -40,12 +40,12 @@ pub enum SimplifyResult { Insn(InsnData), } -fn simplify_phi(dfg: &DataFlowGraph, insn_data: &InsnData) -> Option { +fn simplify_phi(insn_data: &InsnData) -> Option { match insn_data { InsnData::Phi { values, .. } => { let mut values = values.iter().copied(); let first_value = values.next().unwrap(); - if values.all(|value| dfg.is_same_value(first_value, value)) { + if values.all(|value| value == first_value) { Some(SimplifyResult::Value(first_value)) } else { None @@ -499,7 +499,7 @@ impl<'a> generated_code::Context for SimplifyContext<'a> { fn is_eq(&mut self, arg0: ExprValue, arg1: ExprValue) -> Option<()> { match (arg0.as_value(), arg1.as_value()) { - (Some(val1), Some(val2)) => self.dfg.is_same_value(val1, val2), + (Some(val1), Some(val2)) => val1 == val2, _ => arg0 == arg1, } .then_some(()) diff --git a/crates/ir/src/dfg.rs b/crates/ir/src/dfg.rs index 05fbd8b3..55c41f71 100644 --- a/crates/ir/src/dfg.rs +++ b/crates/ir/src/dfg.rs @@ -82,23 +82,15 @@ impl DataFlowGraph { } pub fn change_to_alias(&mut self, value: Value, alias: Value) { - self.values[value] = ValueData::Alias { - alias: self.resolve_alias(alias), - } - } - - pub fn resolve_alias(&self, mut value: Value) -> Value { - for _ in 0..self.values.len() { - match self.values[value] { - ValueData::Insn { .. } - | ValueData::Arg { .. } - | ValueData::Immediate { .. } - | ValueData::Global { .. } => return value, - ValueData::Alias { alias } => value = alias, + let mut users = std::mem::take(&mut self.users[value]); + for insn in &users { + for arg in self.insns[*insn].args_mut() { + if *arg == value { + *arg = alias; + } } } - - panic!("alias loop detected"); + self.users[alias].append(&mut users); } pub fn make_result(&mut self, insn: Insn) -> Option { @@ -159,7 +151,6 @@ impl DataFlowGraph { } pub fn value_insn(&self, value: Value) -> Option { - let value = self.resolve_alias(value); match self.value_data(value) { ValueData::Insn { insn, .. } => Some(*insn), _ => None, @@ -167,13 +158,11 @@ impl DataFlowGraph { } pub fn value_ty(&self, value: Value) -> Type { - let value = self.resolve_alias(value); match &self.values[value] { ValueData::Insn { ty, .. } | ValueData::Arg { ty, .. } | ValueData::Immediate { ty, .. } | ValueData::Global { ty, .. } => *ty, - ValueData::Alias { .. } => unreachable!(), } } @@ -182,7 +171,6 @@ impl DataFlowGraph { } pub fn value_imm(&self, value: Value) -> Option { - let value = self.resolve_alias(value); match self.value_data(value) { ValueData::Immediate { imm, .. } => Some(*imm), ValueData::Global { gv, .. } => self.ctx.with_gv_store(|s| { @@ -199,7 +187,6 @@ impl DataFlowGraph { } pub fn value_gv(&self, value: Value) -> Option { - let value = self.resolve_alias(value); match self.value_data(value) { ValueData::Global { gv, .. } => Some(*gv), _ => None, @@ -281,10 +268,6 @@ impl DataFlowGraph { self.insns[insn].is_branch() } - pub fn is_same_value(&self, v0: Value, v1: Value) -> bool { - self.resolve_alias(v0) == self.resolve_alias(v1) - } - /// Returns `true` if `value` is an immediate. pub fn is_imm(&self, value: Value) -> bool { self.value_imm(value).is_some() @@ -292,7 +275,6 @@ impl DataFlowGraph { /// Returns `true` if `value` is a function argument. pub fn is_arg(&self, value: Value) -> bool { - let value = self.resolve_alias(value); matches!(self.value_data(value), ValueData::Arg { .. }) } } diff --git a/crates/ir/src/ir_writer.rs b/crates/ir/src/ir_writer.rs index 663e2e7f..fb79d80a 100644 --- a/crates/ir/src/ir_writer.rs +++ b/crates/ir/src/ir_writer.rs @@ -205,7 +205,7 @@ trait IrWrite { impl IrWrite for Value { fn write(&self, writer: &mut FuncWriter, w: &mut impl io::Write) -> io::Result<()> { - let value = writer.func.dfg.resolve_alias(*self); + let value = *self; if let Some(imm) = writer.func.dfg.value_imm(value) { write!(w, "{}.", imm)?; let ty = writer.func.dfg.value_ty(value); diff --git a/crates/ir/src/value.rs b/crates/ir/src/value.rs index 36004235..78017f84 100644 --- a/crates/ir/src/value.rs +++ b/crates/ir/src/value.rs @@ -80,8 +80,6 @@ pub enum ValueData { /// The value is a function argument. Arg { ty: Type, idx: usize }, - /// The value is alias to another value. - Alias { alias: Value }, /// The value is immediate value. Immediate { imm: Immediate, ty: Type }, From 4a6a1bdfa15ccc986d8273db2ff290487ac3eb1e Mon Sep 17 00:00:00 2001 From: Sean Billig Date: Sat, 13 Jul 2024 21:10:06 -0700 Subject: [PATCH 3/4] Fix gvn alias bug --- crates/codegen/src/optim/gvn.rs | 25 +++++++++++++++++++--- crates/ir/src/dfg.rs | 1 + crates/ir/src/insn.rs | 37 ++++++++------------------------- 3 files changed, 32 insertions(+), 31 deletions(-) diff --git a/crates/codegen/src/optim/gvn.rs b/crates/codegen/src/optim/gvn.rs index 790c79eb..d461ccee 100644 --- a/crates/codegen/src/optim/gvn.rs +++ b/crates/codegen/src/optim/gvn.rs @@ -1334,6 +1334,8 @@ struct RedundantCodeRemover<'a> { /// Record resolved value phis. resolved_value_phis: FxHashMap, + + renames: FxHashMap, } impl<'a> RedundantCodeRemover<'a> { @@ -1342,7 +1344,23 @@ impl<'a> RedundantCodeRemover<'a> { solver, avail_set: SecondaryMap::default(), resolved_value_phis: FxHashMap::default(), + renames: FxHashMap::default(), + } + } + + fn change_to_alias(&mut self, func: &mut Function, value: Value, target: Value) { + func.dfg.change_to_alias(value, target); + self.renames.insert(value, target); + } + + fn resolve_alias(&self, mut value: Value) -> Value { + for _ in 0..1 + self.renames.len() { + match self.renames.get(&value) { + Some(v) => value = *v, + None => return value, + } } + panic!("alias loop detected"); } /// The entry function of redundant code removal. @@ -1421,7 +1439,7 @@ impl<'a> RedundantCodeRemover<'a> { // Use representative value if the class is in avail set. if let Some(value) = avails.get(&class) { - func.dfg.change_to_alias(insn_result, *value); + self.change_to_alias(func, insn_result, *value); inserter.remove_insn(func); continue; } @@ -1469,7 +1487,8 @@ impl<'a> RedundantCodeRemover<'a> { ty, block, ); - func.dfg.change_to_alias(insn_result, value); + + self.change_to_alias(func, insn_result, value); inserter.remove_insn(func); continue; } @@ -1537,7 +1556,7 @@ impl<'a> RedundantCodeRemover<'a> { for (value_phi, phi_block) in &phi_insn.args { let resolved = self.resolve_value_phi(func, inserter, value_phi, ty, *phi_block); - phi.append_phi_arg(resolved, *phi_block); + phi.append_phi_arg(self.resolve_alias(resolved), *phi_block); } // Insert new phi insn to top of the phi_insn block. diff --git a/crates/ir/src/dfg.rs b/crates/ir/src/dfg.rs index 55c41f71..1fcfc88d 100644 --- a/crates/ir/src/dfg.rs +++ b/crates/ir/src/dfg.rs @@ -249,6 +249,7 @@ impl DataFlowGraph { } pub fn remove_branch_dest(&mut self, insn: Insn, dest: Block) { + // xxx remove user self.insns[insn].remove_branch_dest(dest) } diff --git a/crates/ir/src/insn.rs b/crates/ir/src/insn.rs index d6872567..a11beadf 100644 --- a/crates/ir/src/insn.rs +++ b/crates/ir/src/insn.rs @@ -48,16 +48,10 @@ impl<'a> fmt::Display for DisplayInsn<'a> { #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum InsnData { /// Unary instructions. - Unary { - code: UnaryOp, - args: [Value; 1], - }, + Unary { code: UnaryOp, args: [Value; 1] }, /// Binary instructions. - Binary { - code: BinaryOp, - args: [Value; 2], - }, + Binary { code: BinaryOp, args: [Value; 2] }, /// Cast operations. Cast { @@ -86,15 +80,10 @@ pub enum InsnData { }, /// Unconditional jump instruction. - Jump { - dests: [Block; 1], - }, + Jump { dests: [Block; 1] }, /// Conditional jump instruction. - Branch { - args: [Value; 1], - dests: [Block; 2], - }, + Branch { args: [Value; 1], dests: [Block; 2] }, /// Indirect jump instruction. BrTable { @@ -104,18 +93,13 @@ pub enum InsnData { }, /// Allocate a memory on the stack frame for the given type. - Alloca { - ty: Type, - }, + Alloca { ty: Type }, /// Return. - Return { - args: Option, - }, + Return { args: Option }, - Gep { - args: SmallVec<[Value; 8]>, - }, + /// Get element pointer. + Gep { args: SmallVec<[Value; 8]> }, /// Phi function. Phi { @@ -239,6 +223,7 @@ impl InsnData { if Some(dest) == *default { *default = None; } else { + // xxx remove user table.retain(|block| dest != *block); } @@ -374,10 +359,6 @@ impl InsnData { } } - pub fn replace_arg(&mut self, new_arg: Value, idx: usize) { - self.args_mut()[idx] = new_arg; - } - pub fn is_phi(&self) -> bool { matches!(self, InsnData::Phi { .. }) } From 78c997ea2e4a7a658f44eaf7c9b67f45823c5e4d Mon Sep 17 00:00:00 2001 From: Sean Billig Date: Sun, 28 Jul 2024 17:32:01 -0700 Subject: [PATCH 4/4] Update dfg.users in remove_branch_dest --- crates/codegen/src/optim/licm.rs | 2 +- crates/ir/src/dfg.rs | 54 ++++++++++++++++++++++++++++++-- crates/ir/src/insn.rs | 35 +-------------------- 3 files changed, 54 insertions(+), 37 deletions(-) diff --git a/crates/codegen/src/optim/licm.rs b/crates/codegen/src/optim/licm.rs index d69723a7..9a80166a 100644 --- a/crates/codegen/src/optim/licm.rs +++ b/crates/codegen/src/optim/licm.rs @@ -87,7 +87,7 @@ impl LicmSolver { /// Returns preheader of the loop. /// 1. If there is natural preheader for the loop, then returns it without any modification of - /// function. + /// function. /// 2. If no natural preheader for the loop, then create the preheader and modify function /// layout, `cfg`, and `lpt`. fn create_preheader( diff --git a/crates/ir/src/dfg.rs b/crates/ir/src/dfg.rs index 1fcfc88d..51309379 100644 --- a/crates/ir/src/dfg.rs +++ b/crates/ir/src/dfg.rs @@ -3,6 +3,7 @@ use std::collections::BTreeSet; use cranelift_entity::{entity_impl, packed_option::PackedOption, PrimaryMap, SecondaryMap}; use rustc_hash::FxHashMap; +use smallvec::SmallVec; use crate::{global_variable::ConstantValue, module::ModuleCtx, GlobalVariable}; @@ -249,8 +250,57 @@ impl DataFlowGraph { } pub fn remove_branch_dest(&mut self, insn: Insn, dest: Block) { - // xxx remove user - self.insns[insn].remove_branch_dest(dest) + let this = &mut self.insns[insn]; + match this { + InsnData::Jump { .. } => panic!("can't remove destination from `Jump` insn"), + + InsnData::Branch { dests, args } => { + let remain = if dests[0] == dest { + dests[1] + } else if dests[1] == dest { + dests[0] + } else { + panic!("no dests found in the branch destination") + }; + self.users[args[0]].remove(&insn); + *this = InsnData::jump(remain); + } + + InsnData::BrTable { + default, + table, + args, + } => { + if Some(dest) == *default { + *default = None; + } else if let Some((lhs, rest)) = args.split_first() { + type V = SmallVec<[T; 8]>; + let (keep, drop): (V<_>, V<_>) = table + .iter() + .copied() + .zip(rest.iter().copied()) + .partition(|(b, _)| *b != dest); + let (b, mut a): (V<_>, V<_>) = keep.into_iter().unzip(); + a.insert(0, *lhs); + *args = a; + *table = b; + + for (_, val) in drop { + self.users[val].remove(&insn); + } + } + + let branch_info = this.analyze_branch(); + if branch_info.dests_num() == 1 { + for val in this.args() { + self.users[*val].remove(&insn); + } + *this = InsnData::jump(branch_info.iter_dests().next().unwrap()); + } + } + + _ => panic!("not a branch"), + } } pub fn rewrite_branch_dest(&mut self, insn: Insn, from: Block, to: Block) { diff --git a/crates/ir/src/insn.rs b/crates/ir/src/insn.rs index a11beadf..b195dd85 100644 --- a/crates/ir/src/insn.rs +++ b/crates/ir/src/insn.rs @@ -183,7 +183,7 @@ impl InsnData { pub fn analyze_branch(&self) -> BranchInfo { match self { - Self::Jump { dests, .. } => BranchInfo::Jump { dest: dests[0] }, + Self::Jump { dests } => BranchInfo::Jump { dest: dests[0] }, Self::Branch { args, dests } => BranchInfo::Br { cond: args[0], @@ -204,39 +204,6 @@ impl InsnData { } } - pub fn remove_branch_dest(&mut self, dest: Block) { - match self { - Self::Jump { .. } => panic!("can't remove destination from `Jump` insn"), - - Self::Branch { dests, .. } => { - let remain = if dests[0] == dest { - dests[1] - } else if dests[1] == dest { - dests[0] - } else { - panic!("no dests found in the branch destination") - }; - *self = Self::jump(remain); - } - - Self::BrTable { default, table, .. } => { - if Some(dest) == *default { - *default = None; - } else { - // xxx remove user - table.retain(|block| dest != *block); - } - - let branch_info = self.analyze_branch(); - if branch_info.dests_num() == 1 { - *self = Self::jump(branch_info.iter_dests().next().unwrap()); - } - } - - _ => panic!("not a branch"), - } - } - pub fn rewrite_branch_dest(&mut self, from: Block, to: Block) { match self { Self::Jump { dests, .. } => {