From 6e135b3aea242c24d7acdaf573b0d660164703a8 Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Thu, 7 May 2020 17:13:03 -0700 Subject: [PATCH] peepmatic: Fix a failed assertion due to extra iterations after fixed point After replacing an instruction with an alias to an earlier value, trying to further optimize that value is unnecessary, since we've already processed it, and also was triggering an assertion. --- cranelift/codegen/src/simple_preopt.rs | 22 +++++++++++++++++-- ...zations_after_replacing_with_an_alias.clif | 14 ++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) create mode 100644 cranelift/filetests/filetests/peepmatic/do_not_keep_applying_optimizations_after_replacing_with_an_alias.clif diff --git a/cranelift/codegen/src/simple_preopt.rs b/cranelift/codegen/src/simple_preopt.rs index a03c8219d2..a77c8cb19a 100644 --- a/cranelift/codegen/src/simple_preopt.rs +++ b/cranelift/codegen/src/simple_preopt.rs @@ -611,8 +611,26 @@ pub fn do_preopt<'func, 'isa>( while let Some(block) = pos.next_block() { while let Some(inst) = pos.next_inst() { - preopt.apply_all(&mut pos, ValueOrInst::Inst(inst)); - let inst = pos.current_inst().unwrap(); + // After we apply one optimization, that might make another + // optimization applicable. Keep running the peephole optimizer + // until either: + // + // * No optimization applied, and therefore it doesn't make sense to + // try again, because no optimization will apply again. + // + // * Or when we replaced an instruction with an alias to an existing + // value, because we already ran the peephole optimizer over the + // aliased value's instruction in an early part of the traversal + // over the function. + while let Some(ValueOrInst::Inst(new_inst)) = + preopt.apply_one(&mut pos, ValueOrInst::Inst(inst)) + { + // We transplanted a new instruction into the current + // instruction, so the "new" instruction is actually the same + // one, just with different data. + debug_assert_eq!(new_inst, inst); + } + debug_assert_eq!(pos.current_inst(), Some(inst)); // Try to transform divide-by-constant into simpler operations. if let Some(divrem_info) = get_div_info(inst, &pos.func.dfg) { diff --git a/cranelift/filetests/filetests/peepmatic/do_not_keep_applying_optimizations_after_replacing_with_an_alias.clif b/cranelift/filetests/filetests/peepmatic/do_not_keep_applying_optimizations_after_replacing_with_an_alias.clif new file mode 100644 index 0000000000..ceefd5bd1c --- /dev/null +++ b/cranelift/filetests/filetests/peepmatic/do_not_keep_applying_optimizations_after_replacing_with_an_alias.clif @@ -0,0 +1,14 @@ +test simple_preopt +target x86_64 + +;; This file used to trigger assertions where we would keep trying to +;; unnecessarily apply optimizations after replacing an instruction with an +;; alias of another value that we had already optimized. + +function %foo() { +block0: + v0 = iconst.i32 3 + v1 = srem_imm v0, 2 + v2 = sdiv_imm v1, 1 + trap unreachable +}