diff --git a/cranelift/codegen/src/peepmatic.rs b/cranelift/codegen/src/peepmatic.rs index f78f5ccd3e..24dd0981eb 100644 --- a/cranelift/codegen/src/peepmatic.rs +++ b/cranelift/codegen/src/peepmatic.rs @@ -208,26 +208,6 @@ fn const_to_value<'a>(builder: impl InstBuilder<'a>, c: Constant, root: Inst) -> } } -fn part_to_inst(pos: &mut FuncCursor, root: Inst, part: Part) -> Option { - match part { - Part::Instruction(ValueOrInst::Inst(inst)) => Some(inst), - Part::Instruction(ValueOrInst::Value(v)) => { - let inst = pos.func.dfg.value_def(v).inst()?; - if pos.func.dfg.inst_results(inst).len() == 1 { - Some(inst) - } else { - None - } - } - Part::Constant(c) => { - let v = const_to_value(pos.ins(), c, root); - let inst = pos.func.dfg.value_def(v).unwrap_inst(); - Some(inst) - } - Part::ConditionCode(_) => None, - } -} - fn part_to_value(pos: &mut FuncCursor, root: Inst, part: Part) -> Option { match part { Part::Instruction(ValueOrInst::Inst(inst)) => { @@ -454,14 +434,30 @@ impl<'a, 'b> InstructionSet<'b> for &'a dyn TargetIsa { new: Part, ) -> ValueOrInst { log::trace!("replace {:?} with {:?}", old, new); - let old_inst = old.unwrap_inst(); + let old_inst = old.resolve_inst(&pos.func.dfg).unwrap(); // Try to convert `new` to an instruction, because we prefer replacing // an old instruction with a new one wholesale. However, if the // replacement cannot be converted to an instruction (e.g. the // right-hand side is a block/function parameter value) then we change // the old instruction's result to an alias of the new value. - match part_to_inst(pos, old_inst, new) { + let new_inst = match new { + Part::Instruction(ValueOrInst::Inst(inst)) => Some(inst), + Part::Instruction(ValueOrInst::Value(_)) => { + // Do not try and follow the value definition. If we transplant + // this value's instruction, and there are other uses of this + // value, then we could mess up ordering between instructions. + None + } + Part::Constant(c) => { + let v = const_to_value(pos.ins(), c, old_inst); + let inst = pos.func.dfg.value_def(v).unwrap_inst(); + Some(inst) + } + Part::ConditionCode(_) => None, + }; + + match new_inst { Some(new_inst) => { pos.func.transplant_inst(old_inst, new_inst); debug_assert_eq!(pos.current_inst(), Some(old_inst)); @@ -528,7 +524,7 @@ impl<'a, 'b> InstructionSet<'b> for &'a dyn TargetIsa { ) -> ValueOrInst { log::trace!("make_inst_1: {:?}({:?})", operator, a); - let root = root.unwrap_inst(); + let root = root.resolve_inst(&pos.func.dfg).unwrap(); match operator { Operator::AdjustSpDown => { let a = part_to_value(pos, root, a).unwrap(); @@ -541,12 +537,14 @@ impl<'a, 'b> InstructionSet<'b> for &'a dyn TargetIsa { } Operator::Bconst => { let c = a.unwrap_constant(); - const_to_value(pos.ins(), c, root).into() + let val = const_to_value(pos.ins(), c, root); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::Bint => { let a = part_to_value(pos, root, a).unwrap(); let ty = peepmatic_ty_to_ir_ty(r#type, &pos.func.dfg, root); - pos.ins().bint(ty, a).into() + let val = pos.ins().bint(ty, a); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::Brnz => { let a = part_to_value(pos, root, a).unwrap(); @@ -571,17 +569,20 @@ impl<'a, 'b> InstructionSet<'b> for &'a dyn TargetIsa { } Operator::Iconst => { let a = a.unwrap_constant(); - const_to_value(pos.ins(), a, root).into() + let val = const_to_value(pos.ins(), a, root); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::Ireduce => { let a = part_to_value(pos, root, a).unwrap(); let ty = peepmatic_ty_to_ir_ty(r#type, &pos.func.dfg, root); - pos.ins().ireduce(ty, a).into() + let val = pos.ins().ireduce(ty, a); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::Sextend => { let a = part_to_value(pos, root, a).unwrap(); let ty = peepmatic_ty_to_ir_ty(r#type, &pos.func.dfg, root); - pos.ins().sextend(ty, a).into() + let val = pos.ins().sextend(ty, a); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::Trapnz => { let a = part_to_value(pos, root, a).unwrap(); @@ -604,7 +605,8 @@ impl<'a, 'b> InstructionSet<'b> for &'a dyn TargetIsa { Operator::Uextend => { let a = part_to_value(pos, root, a).unwrap(); let ty = peepmatic_ty_to_ir_ty(r#type, &pos.func.dfg, root); - pos.ins().uextend(ty, a).into() + let val = pos.ins().uextend(ty, a); + pos.func.dfg.value_def(val).unwrap_inst().into() } _ => unreachable!(), } @@ -621,167 +623,199 @@ impl<'a, 'b> InstructionSet<'b> for &'a dyn TargetIsa { ) -> ValueOrInst { log::trace!("make_inst_2: {:?}({:?}, {:?})", operator, a, b); - let root = root.unwrap_inst(); + let root = root.resolve_inst(&pos.func.dfg).unwrap(); match operator { Operator::Band => { let a = part_to_value(pos, root, a).unwrap(); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().band(a, b).into() + let val = pos.ins().band(a, b); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::BandImm => { let a = part_to_imm64(pos, a); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().band_imm(b, a).into() + let val = pos.ins().band_imm(b, a); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::Bor => { let a = part_to_value(pos, root, a).unwrap(); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().bor(a, b).into() + let val = pos.ins().bor(a, b); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::BorImm => { let a = part_to_imm64(pos, a); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().bor_imm(b, a).into() + let val = pos.ins().bor_imm(b, a); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::Bxor => { let a = part_to_value(pos, root, a).unwrap(); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().bxor(a, b).into() + let val = pos.ins().bxor(a, b); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::BxorImm => { let a = part_to_imm64(pos, a); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().bxor_imm(b, a).into() + let val = pos.ins().bxor_imm(b, a); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::Iadd => { let a = part_to_value(pos, root, a).unwrap(); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().iadd(a, b).into() + let val = pos.ins().iadd(a, b); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::IaddImm => { let a = part_to_imm64(pos, a); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().iadd_imm(b, a).into() + let val = pos.ins().iadd_imm(b, a); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::Ifcmp => { let a = part_to_value(pos, root, a).unwrap(); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().ifcmp(a, b).into() + let val = pos.ins().ifcmp(a, b); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::IfcmpImm => { let a = part_to_imm64(pos, a); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().ifcmp_imm(b, a).into() + let val = pos.ins().ifcmp_imm(b, a); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::Imul => { let a = part_to_value(pos, root, a).unwrap(); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().imul(a, b).into() + let val = pos.ins().imul(a, b); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::ImulImm => { let a = part_to_imm64(pos, a); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().imul_imm(b, a).into() + let val = pos.ins().imul_imm(b, a); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::IrsubImm => { let a = part_to_imm64(pos, a); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().irsub_imm(b, a).into() + let val = pos.ins().irsub_imm(b, a); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::Ishl => { let a = part_to_value(pos, root, a).unwrap(); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().ishl(a, b).into() + let val = pos.ins().ishl(a, b); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::IshlImm => { let a = part_to_imm64(pos, a); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().ishl_imm(b, a).into() + let val = pos.ins().ishl_imm(b, a); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::Isub => { let a = part_to_value(pos, root, a).unwrap(); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().isub(a, b).into() + let val = pos.ins().isub(a, b); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::Rotl => { let a = part_to_value(pos, root, a).unwrap(); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().rotl(a, b).into() + let val = pos.ins().rotl(a, b); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::RotlImm => { let a = part_to_imm64(pos, a); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().rotl_imm(b, a).into() + let val = pos.ins().rotl_imm(b, a); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::Rotr => { let a = part_to_value(pos, root, a).unwrap(); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().rotr(a, b).into() + let val = pos.ins().rotr(a, b); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::RotrImm => { let a = part_to_imm64(pos, a); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().rotr_imm(b, a).into() + let val = pos.ins().rotr_imm(b, a); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::Sdiv => { let a = part_to_value(pos, root, a).unwrap(); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().sdiv(a, b).into() + let val = pos.ins().sdiv(a, b); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::SdivImm => { let a = part_to_imm64(pos, a); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().sdiv_imm(b, a).into() + let val = pos.ins().sdiv_imm(b, a); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::Srem => { let a = part_to_value(pos, root, a).unwrap(); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().srem(a, b).into() + let val = pos.ins().srem(a, b); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::SremImm => { let a = part_to_imm64(pos, a); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().srem_imm(b, a).into() + let val = pos.ins().srem_imm(b, a); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::Sshr => { let a = part_to_value(pos, root, a).unwrap(); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().sshr(a, b).into() + let val = pos.ins().sshr(a, b); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::SshrImm => { let a = part_to_imm64(pos, a); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().sshr_imm(b, a).into() + let val = pos.ins().sshr_imm(b, a); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::Udiv => { let a = part_to_value(pos, root, a).unwrap(); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().udiv(a, b).into() + let val = pos.ins().udiv(a, b); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::UdivImm => { let a = part_to_imm64(pos, a); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().udiv_imm(b, a).into() + let val = pos.ins().udiv_imm(b, a); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::Urem => { let a = part_to_value(pos, root, a).unwrap(); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().urem(a, b).into() + let val = pos.ins().urem(a, b); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::UremImm => { let a = part_to_imm64(pos, a); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().urem_imm(b, a).into() + let val = pos.ins().urem_imm(b, a); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::Ushr => { let a = part_to_value(pos, root, a).unwrap(); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().ushr(a, b).into() + let val = pos.ins().ushr(a, b); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::UshrImm => { let a = part_to_imm64(pos, a); let b = part_to_value(pos, root, b).unwrap(); - pos.ins().ushr_imm(b, a).into() + let val = pos.ins().ushr_imm(b, a); + pos.func.dfg.value_def(val).unwrap_inst().into() } _ => unreachable!(), } @@ -799,27 +833,30 @@ impl<'a, 'b> InstructionSet<'b> for &'a dyn TargetIsa { ) -> ValueOrInst { log::trace!("make_inst_3: {:?}({:?}, {:?}, {:?})", operator, a, b, c); - let root = root.unwrap_inst(); + let root = root.resolve_inst(&pos.func.dfg).unwrap(); match operator { Operator::Icmp => { let cond = a.unwrap_condition_code(); let cond = peepmatic_to_intcc(cond); let b = part_to_value(pos, root, b).unwrap(); let c = part_to_value(pos, root, c).unwrap(); - pos.ins().icmp(cond, b, c).into() + let val = pos.ins().icmp(cond, b, c); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::IcmpImm => { let cond = a.unwrap_condition_code(); let cond = peepmatic_to_intcc(cond); let imm = part_to_imm64(pos, b); let c = part_to_value(pos, root, c).unwrap(); - pos.ins().icmp_imm(cond, c, imm).into() + let val = pos.ins().icmp_imm(cond, c, imm); + pos.func.dfg.value_def(val).unwrap_inst().into() } Operator::Select => { let a = part_to_value(pos, root, a).unwrap(); let b = part_to_value(pos, root, b).unwrap(); let c = part_to_value(pos, root, c).unwrap(); - pos.ins().select(a, b, c).into() + let val = pos.ins().select(a, b, c); + pos.func.dfg.value_def(val).unwrap_inst().into() } _ => unreachable!(), } diff --git a/cranelift/filetests/filetests/simple_preopt/do_not_reorder_instructions_when_transplanting.clif b/cranelift/filetests/filetests/simple_preopt/do_not_reorder_instructions_when_transplanting.clif new file mode 100644 index 0000000000..a1e9f47f5a --- /dev/null +++ b/cranelift/filetests/filetests/simple_preopt/do_not_reorder_instructions_when_transplanting.clif @@ -0,0 +1,22 @@ +test simple_preopt +target x86_64 + +;; Test that although v5 can be replaced with v1, we don't transplant `load.i32 +;; v0` on top of `iadd v3, v4`, because that would move the load past other uses +;; of its result. + +function %foo(i64) -> i32 { +block0(v0: i64): + v1 = load.i32 v0 + v2 = iconst.i32 16 + v3 = iadd_imm v1, -16 + v4 = iconst.i32 16 + v5 = iadd v3, v4 + ; check: v1 = load.i32 v0 + ; nextln: v5 -> v1 + ; nextln: v2 = iconst.i32 16 + ; nextln: v3 = iadd_imm v1, -16 + ; nextln: v4 = iconst.i32 16 + ; nextln: nop + return v5 +}