Skip to content
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

Implement jump threading MIR opt #107009

Merged
merged 16 commits into from
Oct 23, 2023
Prev Previous commit
Next Next commit
Correct loop_headers logic.
  • Loading branch information
cjgillot committed Oct 21, 2023
commit 8fb99afb0219846f98a82059de2f0808e4c0f61f
4 changes: 2 additions & 2 deletions compiler/rustc_mir_transform/src/jump_threading.rs
Original file line number Diff line number Diff line change
Expand Up @@ -743,15 +743,15 @@ enum Update {
/// Compute the set of loop headers in the given body. We define a loop header as a block which has
/// at least a predecessor which it dominates. This definition is only correct for reducible CFGs.
/// But if the CFG is already irreducible, there is no point in trying much harder.
/// is already irreducibl
/// is already irreducible.
fn loop_headers(body: &Body<'_>) -> BitSet<BasicBlock> {
let mut loop_headers = BitSet::new_empty(body.basic_blocks.len());
let dominators = body.basic_blocks.dominators();
// Only visit reachable blocks.
for (bb, bbdata) in traversal::preorder(body) {
for succ in bbdata.terminator().successors() {
if dominators.dominates(succ, bb) {
loop_headers.insert(bb);
loop_headers.insert(succ);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was wrong. Added a correction commit.

Copy link
Member

@pnkfelix pnkfelix Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So... the effect of this change is two-fold (when succ dominates bb): 1. it avoids adding bb to the loop headers, and 2. it adds succ to the loop headers.

I see essentially only one effect on the tests though (where that one effect is duplicated in two twos) -- and I think the effect demonstrated by the tests is due to the addition of adding succ to the loop headers (since that cancels out a threading opportunity, which I believe is what is happening in the panic-abort and panic-unwind test)

So, does this imply that there is some missing test coverage, i.e. a test that demonstrates how now correctly removing bb from loop headers enables some jump threading to proceed?

(At least, I assume no other tests were affected by this change. But I have not confirmed that.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'm still going to r+ the PR as is, because the missing test, I think, only represents a failure to demonstrate a newly present optimization opportunity, which while nice to have in tests, is not as important as tests that are catching potential soundness issues in the transformation.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This correction is a change in the definition. The restriction on loop headers is purely heuristic, and has no impact on soundness. LLVM does not like irreducible loops, so we avoid creating them.
The DFA test means precisely to capture this, but was overlooked. With a845bac it should not break silently any more.

}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@
StorageLive(_1);
_1 = DFA::A;
StorageLive(_2);
- goto -> bb1;
+ goto -> bb7;
goto -> bb1;
}

bb1: {
Expand Down Expand Up @@ -64,11 +63,6 @@
_3 = const ();
StorageDead(_7);
goto -> bb1;
+ }
+
+ bb7: {
+ _4 = discriminant(_1);
+ goto -> bb4;
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@
StorageLive(_1);
_1 = DFA::A;
StorageLive(_2);
- goto -> bb1;
+ goto -> bb7;
goto -> bb1;
}

bb1: {
Expand Down Expand Up @@ -64,11 +63,6 @@
_3 = const ();
StorageDead(_7);
goto -> bb1;
+ }
+
+ bb7: {
+ _4 = discriminant(_1);
+ goto -> bb4;
}
}