-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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. |
This comment has been minimized.
This comment has been minimized.
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. |
@GrigorenkoPV any updates on this? thanks |
6ae6dd5
to
e8ef4ef
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
I've rebased to resolve the merge conflicts. As for the remaining issues (outlined them in the OP), I am underqualified to resolve those on my own. I would either need some help with this, or someone to take over the PR entirely. Alternatively, I could try to just brute-force my way through, gaining the qualification in process, but that would take more time than I can currently dedicate to this, sadly. |
The original PR (#86699) died after merge conflicts due to inactivity.
Might allow to re-revert #81238.
I was able to do a rebase (mostly), but, sadly, I am underqualified to deal with the remaining issues. So,
E-help-wanted
label.Current problems:
tests/mir-opt/lower_intrinsics.rs
do not actually get tested (this is even reported bytidy
). The directives are missing. In the original PR they were in the middle of the function and without any indentation: src. I could not get this to work with the current testing infrastructure. Also see these review comments.