-
Notifications
You must be signed in to change notification settings - Fork 13.3k
[MIR] Refine representation and translation of calls #30481
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
Conversation
match self.unreachable_block { | ||
Some(b) => b, | ||
None => { | ||
let bl = self.bcx(mir::START_BLOCK).fcx.new_block(false, "unreachable", None); |
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.
Note to self: maybe self
should contain a fcx
too?
0f066dc
to
fdaa56a
Compare
All in all, I think it is in a good enough spot now to begin reviewing it @nikomatsakis. |
Some(attrs), | ||
debugloc); | ||
if must_copy_dest { | ||
// FIXME: What do we do here? |
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.
AFAIK should just do the same as below: store_ty the result from Invoke to the destination.
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.
The problem here (is much harder than it looks at the first look!) is that invoke
is a basic block terminator and thus we cannot add anything else to the current block. We would have to add, or rather, prepend a store instruction to a success branch, but… that’s tricky, because:
- That’s hacky (duh);
- I can’t seem to see anyway to do it;
- It would make some already-generated blocks invalid (e.g. we would have to assert that success branch has higher basic-block number and has not been translated yet).
Basically, to me it seems like we should change the MIR some more and figure out if destination must be copied (or rather, avoid copying destination altogether as well as encoding the copy, if necessary) during the MIR build phase.
EDIT: we could also generate a temporary block with a store and branch to success block in it (still doesn’t avoid 3rd issue). Sounds like a most correct hack to do, but I’ll see if anybody has better ideas.
As I mentioned on IRC, I'm not sure if I think that splitting out "diverging call" into its own terminator is a good idea. My concerns are:
Basically, I'm not sure if the code for handling calls vs diverging calls will wind up being so very different in other passes. But it's a bit hard to know just now. A compromise might just be to introduce an enum into the call that is either In any case, let me read further before reaching final judgement. |
The last commit contains conversion to inner-enum, like you proposed on IRC. Feel free to compare. |
Comments on nagisa@0c7d57d are yet to be fixed. EDIT: changed/fixed in nagisa@1c2f0d8. @nikomatsakis the code still looks equally ugly to me, but not sure if it could be made any better. |
☔ The latest upstream changes (presumably #30651) made this pull request unmergeable. Please resolve the merge conflicts. |
In all those cases where I gave nits about exhaustive matches, we were just trying to handle converging cases -- might be that a helper function for extracting the converging cases would be good. OTOH, I guess this is why you split those cases into two to start with. |
OK, so, as you say, this is a big PR, and I feel like I could go on pointing out nits forever. But I'd rather we land it and just hack and try to clean things up. Some of these commits get pretty involved, but that may just be unavoidable if we want to generate less naive IR. Calls are just complicated. :) Anyway, so I'm leaning now towards landing this PR and then trying to keep improving things. One thing which would be nice are some more test cases for calls that generate |
☔ The latest upstream changes (presumably #30602) made this pull request unmergeable. Please resolve the merge conflicts. |
e482e81
to
d468ec9
Compare
@nikomatsakis rebased and comments addressed (I hope I didn’t miss any). I added a few more tests as well. |
2e9e015
to
9e7ffb6
Compare
r=me modulo final nits (in particular, the test with graphviz attribute should really be fixed) |
💔 Test failed - auto-win-msvc-64-opt |
☔ The latest upstream changes (presumably #30492) made this pull request unmergeable. Please resolve the merge conflicts. |
DivergingCall is different enough from the regular converging Call to warrant the split. This also inlines CallData struct and creates a new CallTargets enum in order to have a way to differentiate between calls that do not have an associated cleanup block. Note, that this patch still does not produce DivergingCall terminator anywhere. Look for that in the next patches.
This simplifies CFG greatly for some cases :)
Diverge should eventually go away
Unreachable terminator can be contained all within the trans.
It is useful for various cases where direct unreachable cannot be translated and a separate block is necessary.
* Implement landing pads; and * Implement DivergingCall translation; and * Modernise previous implementation of Call somewhat.
This merges two separate Call terminators and uses a separate CallKind sub-enum instead. A little bit unrelatedly, copying into destination value for a certain kind of invoke, is also implemented here. See the associated comment in code for various details that arise with this implementation.
Not any more beautiful.
This considerably simplifies code around calling functions and translation of Resume itself. This removes requirement that a block containing Resume terminator is always translated after something which creates a landing pad, thus allowing us to actually translate some valid MIRs we could not translate before. However, an assumption is added that translator is correct (in regards to landing pad generation) and code will never reach the Resume terminator without going through a landing pad first. Breaking these assumptions would pass an `undef` value into the personality functions.
@bors r=nikomatsakis |
📌 Commit 36b3951 has been approved by |
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
pub fn destination(&self) -> Option<Lvalue<'tcx>> { | ||
match *self { | ||
CallKind::Converging { ref destination, .. } | | ||
CallKind::ConvergingCleanup { ref destination, .. } => Some(destination.clone()), |
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.
Why clone here instead of returning Option<&Lvalue<'tcx>>
and letting the caller decide if it needs to clone?
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.
There’s two reasons, mostly, I had in mind:
- Cloning
Lvalue
is very cheap most of the time (i.e. whenLvalue
is not aProjection
); - There’s users who want
&mut lvalue
and there’s users who want&lvalue
. Returning a value allows to make either one easier when pattern matching (ref
vsref mut
).
On the hindsight, I think this might actually be a bug to return destination by value in some cases which I didn’t think about before, thus I’ll submit a patch in a moment making these return a mutable reference instead.
This just removes the `Some()` that appeared around terminators in MIR text output after rust-lang#30481 (cc @nagisa). The graphviz is already fixed. r? @eddyb
This just removes the `Some()` that appeared around terminators in MIR text output after rust-lang#30481 (cc @nagisa). The graphviz is already fixed. r? @eddyb
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