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

Mir: Refactor calls to permit calls that cannot panic and to better accommodate calls that always panic #29767

Closed
nikomatsakis opened this issue Nov 11, 2015 · 4 comments · Fixed by #30481
Labels
A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html I-needs-decision Issue: In need of a decision. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

Today, calls are always basic block terminators. This is unnecessary for calls that cannot panic. We might consider adding a call rvalue. The plus side is that it permits fewer basic blocks and in particular permits avoiding the unwinding code. The minus side is that it means more than one way for a call to happen.

Another thing is that calls today always have two successors. But for fns of type -> !, they never return normally, so they should only have a panic successor.

It might be better to leave calls as always being terminators, but just have the number of successors be either 1 or 2. If the number of successors is 1, then the fn is either diverging (-> !), in which case this is a panic successor, or else it means the fn is known not to panic somehow.

cc @Aatch you were mentioning this

@nikomatsakis nikomatsakis added I-needs-decision Issue: In need of a decision. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html labels Nov 11, 2015
@Aatch
Copy link
Contributor

Aatch commented Nov 11, 2015

One advantage of leaving calls as terminators is that it makes inlining at the MIR level easier as you don't need to (potentially) split an existing basic block.

@nikomatsakis
Copy link
Contributor Author

@Aatch interesting point. I think I am persuaded that the right thing is for them to stay as terminators but to have more flexibility about number of successors.

@RalfJung
Copy link
Member

RalfJung commented Dec 2, 2015

From a theoretical perspective, relating the MIR to a CPS version of Rust, I also agree that calls should always terminate a basic block. This implies that every basic block in MIR corresponds exactly to a lambda-expression in CPS, and (proper Rust) functions never return; they always call a return continuation. If functions in MIR could live in the middle of a basic block, that would correspond to a function returning back to its caller in the CPS, which should just never be the case.

Whether functions take a return continuation, a panic continuation, or both - that's of course completely flexible.

@nagisa
Copy link
Member

nagisa commented Dec 3, 2015

I do not see much problem with call being a terminator either. When it comes to non-panicking or diverging functions LLVM’s call can always be followed by unconditional br to its only viable successor. While that does introduce some extra basic blocks, majority of calls will be invoked anyway.

bors added a commit that referenced this issue Jan 6, 2016
r? @nikomatsakis

This is a pretty big PR conflating changes to a few different block terminators (Call, DivergingCall, Panic, Resume, Diverge), because they are somewhat closely related.

Each commit has a pretty good description on what is being changed in each commit. The end result is greatly simplified CFG and translation for calls (no success branch if the function is diverging, no cleanup branch if there’s nothing to cleanup etc).

Fixes #30480
Fixes #29767
Partialy solves #29575
Fixes #29573
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html I-needs-decision Issue: In need of a decision. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants