-
Notifications
You must be signed in to change notification settings - Fork 387
feat(ssa): Remove all non side effectual instructions from unreachable blocks during DIE #9238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -245,7 +245,9 @@ impl Context { | |
| for instruction_id in block.instructions().iter().rev() { | ||
| let instruction = &function.dfg[*instruction_id]; | ||
|
|
||
| if self.is_unused(*instruction_id, function) { | ||
| if self.is_unused(*instruction_id, function) | ||
| || self.can_remove_with_unreachable_terminator(function, block, *instruction_id) | ||
| { | ||
| self.instructions_to_remove.insert(*instruction_id); | ||
| } else { | ||
| // We can't remove rc instructions if they're loaded from a reference | ||
|
|
@@ -290,6 +292,31 @@ impl Context { | |
| } | ||
| } | ||
|
|
||
| /// If we are inside a block that terminates with an [unreachable][TerminatorInstruction::Unreachable] | ||
| /// we can eliminate any instructions that: | ||
| /// - Have no side effects | ||
| /// - Are not the trap instruction which triggered the unreachable (we always expect the instruction preceding an unreachable to trigger a runtime failure) | ||
| /// - Does not have any results which are used in other instructions | ||
| fn can_remove_with_unreachable_terminator( | ||
| &mut self, | ||
| function: &Function, | ||
| block: &BasicBlock, | ||
| instruction_id: InstructionId, | ||
| ) -> bool { | ||
| let terminator = block.unwrap_terminator(); | ||
| let TerminatorInstruction::Unreachable { .. } = terminator else { | ||
| return false; | ||
| }; | ||
|
|
||
| let last_instruction = block.instructions().last().copied(); | ||
| let is_last = Some(instruction_id) == last_instruction; | ||
| if is_last { | ||
| return false; | ||
| } | ||
|
|
||
| self.is_unused(instruction_id, function) | ||
| } | ||
|
|
||
| /// Adds values referenced by the terminator to the set of used values. | ||
| fn mark_terminator_values_as_used(&mut self, function: &Function, block: &BasicBlock) { | ||
| let terminator = block.unwrap_terminator(); | ||
|
|
@@ -1179,4 +1206,60 @@ mod test { | |
| let ssa = ssa.dead_instruction_elimination(); | ||
| assert_normalized_ssa_equals(ssa, src); | ||
| } | ||
|
|
||
| #[test] | ||
| fn remove_all_instructions_in_unreachable_block_but_final_trap() { | ||
| let src = " | ||
| acir(inline) predicate_pure fn main f0 { | ||
| b0(): | ||
| v0 = allocate -> &mut u1 | ||
| v2 = make_array [u1 0, v0] : [(u1, &mut u1); 1] | ||
| v4 = array_get v2, index u32 2 -> u1 | ||
|
Comment on lines
+1215
to
+1217
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't the current DIE pass (without the changes in this PR) already remove these as unused?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes it would. I can update the test, but after #9232 none of these will be removed.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am going to branch off of #9232 as this is mostly relevant for that case, and I'm not sure if this is the best solution anymore. |
||
| unreachable | ||
| } | ||
| "; | ||
|
|
||
| let ssa = Ssa::from_str(src).unwrap(); | ||
| let ssa = ssa.dead_instruction_elimination(); | ||
| assert_ssa_snapshot!(ssa, @r" | ||
| acir(inline) predicate_pure fn main f0 { | ||
| b0(): | ||
| unreachable | ||
| } | ||
| "); | ||
| } | ||
|
|
||
| #[test] | ||
| fn do_not_remove_impure_calls_in_unreachable_block() { | ||
| let src = r#" | ||
| acir(inline) fn main f0 { | ||
| b0(): | ||
| v0 = allocate -> &mut Field | ||
| call f1(v0) | ||
| unreachable | ||
| } | ||
| acir(inline) fn impure_take_ref f1 { | ||
| b0(v0: &mut Field): | ||
| unreachable | ||
| } | ||
| "#; | ||
|
|
||
| let ssa = Ssa::from_str(src).unwrap(); | ||
| let ssa = ssa.purity_analysis(); | ||
| let (ssa, _) = ssa.dead_instruction_elimination_inner(false, false); | ||
|
|
||
| // We expect the program to be unchanged except that functions are labeled with purities now | ||
| assert_ssa_snapshot!(ssa, @r#" | ||
| acir(inline) impure fn main f0 { | ||
| b0(): | ||
| v0 = allocate -> &mut Field | ||
| call f1(v0) | ||
| unreachable | ||
| } | ||
| acir(inline) impure fn impure_take_ref f1 { | ||
| b0(v0: &mut Field): | ||
| unreachable | ||
| } | ||
| "#); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering if we can find a way to refactor this so that
self.is_unusedisn't called twice (self.is_unused(...) || self.can_remove_with_unreachable_terminator(...)where the latter calls the former again).I must be missing something, but when do we return from this construct anything but what
is_unsedwould have returned already? All we return isfalsefrom the early returns, and then end up in the same case as what the||already checked.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good point, I'll refactor this