-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Allow reifying intrinsics to fn pointers. (rebase of #86699) #126595
base: master
Are you sure you want to change the base?
Conversation
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
This comment has been minimized.
This comment has been minimized.
I'll review this soon |
@@ -154,6 +154,7 @@ fn make_shim<'tcx>(tcx: TyCtxt<'tcx>, instance: ty::InstanceDef<'tcx>) -> Body<' | |||
&add_moves_for_packed_drops::AddMovesForPackedDrops, | |||
&deref_separator::Derefer, | |||
&remove_noop_landing_pads::RemoveNoopLandingPads, | |||
&lower_intrinsics::LowerIntrinsics, |
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 was this added?
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.
Actually, I know why. I wonder if it's better to place this in build_call_shim
, or even make our own function like build_reified_intrinsic
.
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.
Generally looks fine.
I think this needs a T-types FCP since AFAICT the only way to hit this is on nightly code. Is that last part correct?
Regardless, do you mind writing up a short blurb motivating why we want to do this and explaining the general approach?
First this needs a rebase and also please fix the CI issues @rustbot author |
572fbdc
to
85abfd0
Compare
Done
Sorry, I've tried, but couldn't figure out what the problem was and why it didn't pick up the mir test. I would appreciate if somebody could help me here. |
This comment has been minimized.
This comment has been minimized.
@rustbot label +E-help-wanted |
☔ The latest upstream changes (presumably #127831) made this pull request unmergeable. Please resolve the merge conflicts. |
|
||
// Check that the MIR shims used for reifying intrinsics to `fn` pointers, | ||
// also go through the lowering pass. | ||
pub fn reify_intrinsics() -> impl Copy { |
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.
Maybe
// EMIT_MIR lower_intrinsics.reify_intrinsics.LowerIntrinsics.mir
?
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.
Well, I think this should be a .diff
to preserve the original intentions. Plus I was not able to make it generate the same code as in #86699.
85abfd0
to
6ae6dd5
Compare
I've rebased and found a place where this ICEs. I am not sure whether the ICE was there originally or it got introduced after one of the rebases. |
The job Click to see the possible cause of the failure (guessed by this bot)
|
I've checked and this ICE has been present since the first version of this PR. I was not able to check whether it existed in the original #86699, because I couldn't get that old version of the compiler to build. |
The original PR (#86699) died after merge conflicts due to inactivity. This PR is just a rebase of the original with some minor changes to make it fit into the now-nearly-3-years-older rustc. Such as using
//@
instead of//
for test directives, the removal ofAbi::PlatformIntrinsic
in #121516, etc.I am not very familiar with the code surrounding the changes, so review this with caution. There might have been some new problems that popped up during those 3 years that I have no idea about.
Might allow to re-revert #81238.