-
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
Monomorphize in check mode to report (almost) all PMEs #107510
Conversation
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit b99639861e05bb61a3235429a82a8c232233c751 with merge f792b4425823c469a96cdaff35d25590fe0e5970... |
What's the "almost" about? |
codegen backends can still bail out if they don't support something (like some huge array size limits that are very target dependent) |
We do check for "types can't be larger than 2^48" somewhere on the MIR level already -- so this is a separate check? Anyway, would be good to have a concrete example and an issue tracking that, but doesn't sound high priority to fix. |
We have tests for
|
And const-eval errors, I presume? |
those all get emitted in |
☔ The latest upstream changes (presumably #100754) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment has been minimized.
This comment has been minimized.
I personally think whether optimised mir is used or for collection or not is orthogonal to whether instances are collected eagerly/lazily. Also, wouldn't this approach cause |
This comment has been minimized.
This comment has been minimized.
cc @tmandry |
Thanks for putting this up, I'm very curious to see the perf results! I think the use of PME here is slightly confusing – if I understand correctly we would still have errors that depend on which generic parameters are used, like the example in #104087 (comment). |
@oli-obk another implementation strategy that we should consider here is adding a new field to Part of the reason that I bring this up is because I knew the topic of this issue was a possibility, but thought it was a non-issue specifically because I was under the impression that the |
How is required_consts not already doing that?
The trouble is that we don't have required_fns so we might not transitively monomorphize functions even though they were mentioned by the user.
|
Well, what I didn't realize was that required_consts only contains consts that are unevaluated by the time we do borrowck. What I thought happened is that all consts are put into there, specifically to ensure that things like this don't happen Edit: Oh, I realize what the confusion might have been. I was implicitly assuming that if we put a FnDef-typed constant into the list of constants, that we'd then go and actually mono that function (at least enough to go set off any PMEs) |
Ah -- no, all we do with that list is evaluate constants, we don't monomorphize any further functions. |
If we want to make |
That's quite a big new feature that requires an RFC IMO. I don't think there is currently a plan to implement this. |
Yeah, agreed. That kind of feature would also definitely need explicit Mir support |
There's a lang team meeting that will touch on this PR next week (Feb 22). @oli-obk Do you think you could get it fixed up with a perf run by then? 🙏 |
b70f67f
to
5a2cab4
Compare
The |
I think we can avoid it. We only need to care about generic functions, and those have their MIR encoded no matter what |
The job Click to see the possible cause of the failure (guessed by this bot)
|
pub enum MonoItemCollectionMode { | ||
Eager, | ||
Eager { | ||
/// Whether to use optimized mir for collection or just analyis 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.
/// Whether to use optimized mir for collection or just analyis MIR. | |
/// Whether to use optimized mir for collection or just analysis MIR. |
☔ The latest upstream changes (presumably #108872) made this pull request unmergeable. Please resolve the merge conflicts. |
Closing this as it was an experiment |
This never got far enough to get a perf result, did it? Would have been nice to get some numbers, though I think they'd be devastating.^^ |
fixes #99682
see #104087 (comment) for discussion
r? @ghost