Do not deduce parameter attributes during CTFE#151842
Do not deduce parameter attributes during CTFE#151842rust-bors[bot] merged 2 commits intorust-lang:mainfrom
Conversation
5650820 to
ea095f5
Compare
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
|
Reminder, once the PR becomes ready for a review, use |
ea095f5 to
de839fc
Compare
|
Note that ABI "adjustments" typically do more than just deducing parameter attributes. So if "unadjusted" just refers to the attributes, that's confusing naming. Also, having the full signature might be relevant for the UB checking that const-eval and Miri are doing. So it's at least slightly concerning that we shouldn't be able to access some of that info any more. What exactly is the difference between the signatures we obtain this way? |
This comment was marked as outdated.
This comment was marked as outdated.
|
Looking at #151748, it seems like the underlying issue is that fn_sig_of_instance looks at the body of the function. That is indeed somewhat sus -- at least all the UB-relevant information should be deducible without the body. Please extend the PR description to explain this; it currently takes a bit of digging to fiure out what problem you're actually solving. If we end up resolving this by splitting the query, it is very important that the queries have good documentation explaining their relationship. We want to avoid someone changing this code in the future in a way that removes too much information from the "unadjusted" signature. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
de839fc to
6a5ec89
Compare
This comment has been minimized.
This comment has been minimized.
6a5ec89 to
132a633
Compare
|
That name and PR description are much better, thank you! |
|
@bors try |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…tfe, r=<try> Do not deduce parameter attributes during CTFE
|
I gave some feedback on the comments. I have not looked at the actual code changes at all. |
FWIW this is something we already do quite a bit; we usually call these queries |
|
@rustbot ready |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
All good points—I've incorporated, and rebased. @rustbot ready |
|
One last comment nit. :) r=me with that resolved. Thanks a lot for working through so many rounds of review with me! |
`fn_abi_of_instance` can look at function bodies to deduce codegen optimization attributes (namely `ArgAttribute::ReadOnly` and `ArgAttribute::CapturesNone`) of indirectly-passed parameters. This can lead to cycles when performed during typeck, when such attributes are irrelevant. This patch breaks a subquery out from `fn_abi_of_instance`, `fn_abi_of_instance_no_deduced_attrs`, which returns the ABI before such parameter attributes are deduced; and that new subquery is used in CTFE instead.
As a performance optimization, we altogether avoid ever invoking the query that computes an Instance fn's fully optimized ABI in cases where the build will not actually deduce parameter attributes.
|
I updated the comment to reflect your suggestion, but with a minor difference in wording. Hope that's okay? @bors r=RalfJung
Au contraire - thank you for putting up with my constant changes, and providing such excellent feedback. This PR is so much better as a result! |
This comment has been minimized.
This comment has been minimized.
|
💔 Test for 8f8f174 failed: CI. Failed job:
|
😆 sounds like GitHub's having a good day ! |
|
Lmao lets try again then |
This comment has been minimized.
This comment has been minimized.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@bors retry The runner has received a shutdown signal |
|
❗ You can only retry pull requests that are approved and have a previously failed auto build. Hint: There is currently a pending auto build on this PR. To cancel it, run |
|
Oh that was probably just a slow log analyzer? |
|
See #t-infra > free disk space step takes 8-10 minutes @ 💬 for discussion around disk space failures |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing b90dc1e (parent) -> b55e20a (this PR) Test differencesShow 82 test diffsStage 1
Stage 2
Additionally, 80 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard b55e20ad905a336395ca80863488d0827712d067 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (b55e20a): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -0.2%, secondary -1.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 479.576s -> 478.218s (-0.28%) |
View all comments
Ever since #103172,
fn_abi_of_instancemight look at a function's body to deduce certain codegen optimization attributes for its indirectly-passed parameters beyond what can be determined purely from its signature (namely todayArgAttribute::ReadOnlyandArgAttribute::CapturesNone). Since #130201, looking at a function's body in this way entails generating, for any coroutine-closures, additional by-move MIR bodies (which aren't represented in the HIR)—but this requires knowing the types of their context and consequently cycles can ensue if such bodies are generated before typeck is complete (such as during CTFE).Since they have no bearing on the evaluation result, this patch breaks a subquery out from
fn_abi_of_instance,fn_abi_of_instance_no_deduced_attrs, which returns the ABI before such parameter attributes are deduced; and that new subquery is used in CTFE instead (however, since parameter attributes are only deduced in optimized builds, as a performance optimization we avoid calling the original query unless we are performing such a build).Fixes #151748
Fixes #152497