Do not deduce parameter attributes during CTFE

[eggyal]

Ever since #103172 (cc @pcwalton), fn_abi_of_instance might 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 today ArgAttribute::ReadOnly and ArgAttribute::CapturesNone). However, this can lead to cycles when performed during CTFE, despite such attributes being irrelevant to 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.

This is the correct fix that #151784 was (incorrectly) attempting to part-address.

Fixes #151748
r? jdonszelmann (as requested in the previous PR)

[rustbot]

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

[eggyal]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[RalfJung]

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?

[eggyal]

Agreed—I'll rename to fn_abi_of_instance_no_deduced_attrs.

The only differences are in the attributes of indirectly-passed parameters, which will include ArgAttribute::ReadOnly and/or ArgAttribute::CapturesNone in optimised builds when their applicability can be inferred from the function body.

[RalfJung]

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.

[eggyal]

I've updated the PR description—hopefully it's now clearer?

Re documentation, the "unadjusted" query (to be renamed fn_abi_of_instance_no_deduced_attrs) is currently documented as follows:

Compute a FnAbi suitable for declaring/defining an fn instance, and for direct calls to an fn. This ABI does not have deduced parameter attributes set, and therefore may produce less optimal codegen than fn_abi_of_instance (which should be preferred instead). However, CTFE may wish to avoid such parameter attribute deduction, as it can otherwise result in loops during typeck.

Happy to clarify it further if required.

[eggyal]

I've updated the documentation to:

Compute a FnAbi suitable for declaring/defining an fn instance, and for direct calls to an fn. Indirectly-passed parameters in the returned ABI might not include all possible codegen optimization attributes (such as ReadOnly or CapturesNone), as deducing these requires inspection of function bodies that can lead to cycles when perfomed during typeck. Post typeck, you should prefer the optimized ABI returned by fn_abi_of_instance.

NB: the ABI returned by this query must not differ from that returned by fn_abi_of_instance in any other way.

@rustbot ready

[RalfJung]

That name and PR description are much better, thank you!

[RalfJung]

@bors try
@rust-timer queue

[RalfJung]

Suggested change

	/// to an `fn`. Indirectly-passed parameters in the returned ABI might not include all possible
	/// codegen optimization attributes (such as `ReadOnly` or `CapturesNone`), as deducing these
	/// requires inspection of function bodies that can lead to cycles when performed during typeck.
	/// Post typeck, you should prefer the optimized ABI returned by `fn_abi_of_instance`.
	/// to an `fn`. This query does *not* look at the function body to deduce further
	/// attributes for function arguments as doing so can lead to cycles during typeck.
	/// Post typeck, you should prefer the optimized ABI returned by `fn_abi_of_instance`.

Let's not list what exactly gets put into "deduced attrs" as that will inevitably expand in the future.

[RalfJung]

Suggested change

	/// Compute a `FnAbi` suitable for declaring/defining an `fn` instance, and for direct calls*
	/// to an `fn`. Indirectly-passed parameters in the returned ABI will include applicable
	/// codegen optimization attributes, including `ReadOnly` and `CapturesNone` -- deduction of
	/// which requires inspection of function bodies that can lead to cycles when performed during
	/// typeck. During typeck, you should therefore use instead the unoptimized ABI returned by
	/// `fn_abi_of_instance_no_deduced_attrs`.
	/// Compute a `FnAbi` suitable for declaring/defining an `fn` instance, and for direct calls*
	/// to an `fn`. This query looks at the function body to deduce further attributes, which
	/// can lead to cycles when performed during typeck. During typeck, you should therefore
	/// instead use `fn_abi_of_instance_no_deduced_attrs`.

[RalfJung]

I gave some feedback on the comments. I have not looked at the actual code changes at all.

[rust-bors]

☀️ Try build successful (CI)
Build commit: cbab83a (cbab83a2924e26be5f80b36c1432a52a618b6c07, parent: e823167aa6f7f03aea8e91208ce9bace8ad9ebf2)

[rust-timer]

Finished benchmarking commit (cbab83a): comparison URL.

Overall result: ❌ regressions - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.2%, 1.0%]	36
Regressions ❌ (secondary)	0.7%	[0.4%, 1.3%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.2%, 1.0%]	36

Max RSS (memory usage)

Results (primary 1.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.1%	[0.7%, 1.5%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.1%	[0.7%, 1.5%]	4

Cycles

Results (secondary 2.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.5%	[2.2%, 2.9%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 474.712s -> 472.121s (-0.55%)
Artifact size: 397.79 MiB -> 397.81 MiB (0.00%)

[jdonszelmann]

@rustbot reroll, not quite comfy with reviewing this one. @RalfJung maybe you're the right reviewer?

[rustbot]

Error: Parsing assign command in comment failed: ...' reroll' | error: expected end of command at >| ', not quit'...

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip.

[eggyal]

Re the performance regression, it's worth noting that these cycles currently lead to us rejecting valid programs such as can be seen in the linked issue, the simplest minimisation of which is included as a new UI test with this PR:

rust/tests/ui/coroutine/avoid-query-cycle-in-ctfe.rs

Lines 10 to 18 in 132a633

	fn main() {
	let _ = async || {
	let COMPLEX_CONSTANT = ();
	};
	}
	const fn do_nothing() {}
	const COMPLEX_CONSTANT: () = do_nothing();

Accordingly I think a 0.4% mean instruction count regression in the primary suite might be acceptable in order not to continue rejecting such programs? But of course if there's a more performant solution, that would be preferable.

@rustbot label: +perf-regression-triaged

[theemathas]

Since the previous command failed:

@rustbot reroll

[RalfJung]

This is a pretty broad regression, ISTM we should explore other ways of fixing this / ways to improve the performance of this before deciding to go with this approach.

Cc @rust-lang/wg-compiler-performance

[BoxyUwU]

r? RalfJung

[rustbot]

RalfJung is not on the review rotation at the moment.
They may take a while to respond.

[rust-bors]

☔ The latest upstream changes (presumably #152089) made this pull request unmergeable. Please resolve the merge conflicts.

[eggyal]

@rustbot author