Skip to content
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

Add const_eval_select intrinsic #89247

Merged
merged 4 commits into from
Oct 14, 2021

Conversation

fee1-dead
Copy link
Member

Adds an intrinsic that calls a given function when evaluated at compiler time, but generates a call to another function when called at runtime.

See rust-lang/const-eval#7 for previous discussion.

r? @oli-obk.

@rust-highfive
Copy link
Collaborator

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 25, 2021
@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 25, 2021

cc @rust-lang/wg-const-eval

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see some tests that hit the invalid use cases that typeck is catching and thus report errors. While it is not important for intrinsics, it's still good to check that they aren't completely bonkers

compiler/rustc_codegen_ssa/src/mir/block.rs Outdated Show resolved Hide resolved
library/core/src/intrinsics.rs Outdated Show resolved Hide resolved
@nbdd0121
Copy link
Contributor

nbdd0121 commented Sep 27, 2021

Alternative design idea #89291: have a call_if_rt

pub fn call_if_rt<R, F: FnOnce() -> R + Copy>(f: F) -> Option<R>;

that returns Some(f()) in runtime and None in compile time.

Pro:

  • No changes required to typeck, codegen
  • Allows everything to be written in the same function:
const fn some_const_fn() {
    if let Some(ret) = call_if_rt(|| {
		// runtime part
	}) {
		return ret;
	}
	// const part
}

Con:

  • Can't handle move.

@nbdd0121
Copy link
Contributor

Came up with a new idea while writing the above comment:

How about have two functions

pub fn is_const_eval() -> bool;

and

pub fn call_at_rt<R, F: FnOnce() -> R>(f: F) -> R;

which will just call f() in runtime, and abort if called in compile time.

This should have the same pro as call_if_rt that it allows no typeck, codegen changes and allows everything to be written in the same function:

const fn some_const_fn() {
    if !is_const_eval() {
		return call_at_rt(|| {
			// runtime part
		});
	}
	// const part
}

while it could still handle move.

@RalfJung
Copy link
Member

Seems a bit annoying to have two intrinsics. Also, const-check special handling will still be required to not subject the closure called by call_at_rt to const-check rules.

Why does this PR require such large typeck changes in the first place?

@nbdd0121
Copy link
Contributor

nbdd0121 commented Sep 27, 2021

Seems a bit annoying to have two intrinsics. Also, const-check special handling will still be required to not subject the closure called by call_at_rt to const-check rules.

Closure isn't considered const, so no changes are needed for that regard: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=2085fb30e0a126f84b16b1744901a0f7

@fee1-dead
Copy link
Member Author

Both cannot handle move, which can be a limitation, and I think that we shouldn't allow closures to be used in const contexts like this. There could be hidden problems. Therefore this might be the best and the most straightforward solution we have.

I wouldn't consider being able to write them in one function a pro, IMO I think we should make this intrinsic a little bit intentionally complex and hard to use.

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 28, 2021

Both cannot handle move, which can be a limitation, and I think that we shouldn't allow closures to be used in const contexts like this. There could be hidden problems. Therefore this might be the best and the most straightforward solution we have.

Could we pick the API of this PR, but still use the lang item solution? So

#[lang = "const_eval_select"]
pub fn const_eval_select<R, F: FnOnce() -> R, G: const FnOnce() -> R>(f: F) -> R {
    f()
}

Then the typeck change could be just checking that F and G are function items, while leaving all the rest (signature, constness, ..) to the regular typeck logic?

This would avoid touching codegen backends entirely.

@nbdd0121
Copy link
Contributor

Could we pick the API of this PR, but still use the lang item solution? So

#88963 would be needed first.

#[lang = "const_eval_select"]
pub fn const_eval_select<R, F: FnOnce() -> R, G: const FnOnce() -> R>(f: F) -> R {
    f()
}

Then the typeck change could be just checking that F and G are function items, while leaving all the rest (signature, constness, ..) to the regular typeck logic?

I don't think function item check would be necessary then.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 28, 2021

🙈 I thought I r+ed that already...

Only allowing function items, in order to make it very explicit what is going on still stands, but it could "simply" be circumvented via const_eval_select(call_first, const_fn_call, (|x| x.stuff(), real_arg)) where fn call_first<T>((f, arg): (impl FnOnce(T), T)) {f(arg)}, so I guess you're right, we can just allow closures.

@oli-obk oli-obk added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 6, 2021
@fee1-dead fee1-dead force-pushed the const-eval-select branch 2 times, most recently from 843b469 to d9163ae Compare October 12, 2021 05:07
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@fee1-dead fee1-dead added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 12, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Oct 14, 2021

r=me with CI passing

@fee1-dead
Copy link
Member Author

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Oct 14, 2021

📌 Commit 11fac09 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 14, 2021
@bors
Copy link
Contributor

bors commented Oct 14, 2021

⌛ Testing commit 11fac09 with merge c34ac87...

@bors
Copy link
Contributor

bors commented Oct 14, 2021

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing c34ac87 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 14, 2021
@bors bors merged commit c34ac87 into rust-lang:master Oct 14, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 14, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c34ac87): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@fee1-dead fee1-dead deleted the const-eval-select branch October 15, 2021 13:36
jfrimmel added a commit to jfrimmel/rust that referenced this pull request Nov 9, 2021
This commit re-enables the debug checks for valid usages of the two
functions `copy()` and `copy_nonoverlapping()`. Those checks were com-
mented out in rust-lang#79684 in order to make the functions const. All that's
been left was a FIXME, that could not be resolved until there is was way
to only do the checks at runtime.
Since rust-lang#89247 there is such a way: `const_eval_select()`. This commit
uses that new intrinsic in order to either do nothing (at compile time)
or to do the old checks (at runtime).

The change itself is rather small: in order to make the checks usable
with `const_eval_select`, they are moved into a local function (one for
`copy` and one for `copy_nonoverlapping` to keep symmetry).

The change does not break referential transparency, as there is nothing
you can do at compile time, which you cannot do on runtime without get-
ting undefined behavior. The CTFE-engine won't allow missuses. The other
way round is also fine.
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 13, 2021
…acrum

Re-enable `copy[_nonoverlapping]()` debug-checks

This commit re-enables the debug checks for valid usages of the two functions `copy()` and `copy_nonoverlapping()`. Those checks were commented out in rust-lang#79684 in order to make the functions const. All that's been left was a FIXME, that could not be resolved until there is was way to only do the checks at runtime.
Since rust-lang#89247 there is such a way: `const_eval_select()`. This commit uses that new intrinsic in order to either do nothing (at compile time) or to do the old checks (at runtime).

The change itself is rather small: in order to make the checks usable with `const_eval_select`, they are moved into a local function (one for `copy` and one for `copy_nonoverlapping` to keep symmetry).

The change does not break referential transparency, as there is nothing you can do at compile time, which you cannot do on runtime without getting undefined behavior. The CTFE-engine won't allow missuses. The other way round is also fine.

I've refactored the code to use `#[cfg(debug_assertions)]` on the new items. If that is not desired, the second commit can be dropped.
I haven't added any checks, as I currently don't know, how to test this properly.

Closes rust-lang#90012.

cc `@rust-lang/lang,` `@rust-lang/libs` and `@rust-lang/wg-const-eval` (as those teams are linked in the issue above).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants