Add intrinsics::const_deallocate
#92274
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
the
T-compiler
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @RalfJung |
fbstj
reviewed
Dec 25, 2021
src/test/ui/consts/const-eval/heap/dealloc_intrinsic_incorrect_layout.rs
Outdated
Show resolved
Hide resolved
|
Cc @rust-lang/wg-const-eval |
RalfJung
reviewed
Dec 30, 2021
RalfJung
reviewed
Dec 30, 2021
library/core/src/intrinsics.rs
Outdated
| @@ -1918,6 +1918,12 @@ extern "rust-intrinsic" { | |||
| #[rustc_const_unstable(feature = "const_heap", issue = "79597")] | |||
| pub fn const_allocate(size: usize, align: usize) -> *mut u8; | |||
|
|
|||
| /// Deallocate a memory which allocated by `intrinsics::const_allocate` at compile time. | |||
| /// Should not be called at runtime. | |||
There was a problem hiding this comment.
IMO it would make more sense to just make this a NOP at runtime (since it looks like it will also be a NOP for some compile-time cases, so this is more consistent).
There was a problem hiding this comment.
We already have const_eval_select for that. The intrinsic should imo not be implemented in backends
There was a problem hiding this comment.
We already have const_eval_select for that.
I had thought wrapping const_(de)allocate with const_eval_select works well, but even just defining an function which calls the intrinsic causes a codegen error. So we may need to add nop to codegens anyway.
There was a problem hiding this comment.
That seems strange, the const function shouldn't even be codegen'd... right?
There was a problem hiding this comment.
It's in the generic args of the const_eval_select call
There was a problem hiding this comment.
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=7c6e75dd1cc1d2f14878ee24a3fe43cb
It occurs when compiling as a library crate.
EDIT: It seems that we can avoid a codegen error by using #[inline(always)] but I'm worried about whether it's okay to do or not.
RalfJung
reviewed
Jan 1, 2022
yvt
added a commit
to r3-os/r3
that referenced
this pull request
Jan 20, 2022
We need a suballocator for a compile-time heap because the `const_deallocate` intrinsic isn't implemented yet (there's an open PR [1]), but we want to reuse the deallocated memory regions. We are not adding the vendored `rlsf` as a separate crate because, as explained earlier, this is a temporary measure, and we don't want to pollute crates.io's namespace. We can't make the necessary changes to the upstream `rlsf` because maintaining the compatibility with a stable compiler is very likely to pose a considerable challenge and unacceptable maintenance burdens. [1]: rust-lang/rust#92274
yvt
added a commit
to r3-os/r3
that referenced
this pull request
Jan 20, 2022
We need a suballocator for a compile-time heap because the `const_deallocate` intrinsic isn't implemented yet (there's an open PR [1]), but we want to reuse deallocated memory regions. We are not adding the vendored `rlsf` as a separate crate because, as explained earlier, this is a temporary measure, and we don't want to pollute crates.io's namespace. We can't make the necessary changes to the upstream `rlsf` because maintaining the compatibility with a stable compiler is very likely to pose a considerable challenge and unacceptable maintenance burdens. [1]: rust-lang/rust#92274
yvt
added a commit
to r3-os/r3
that referenced
this pull request
Jan 20, 2022
We need a suballocator for a compile-time heap because the `const_deallocate` intrinsic isn't implemented yet (there's an open PR [1]), but we don't want deallocated memory regions to go to waste. We are not adding the vendored `rlsf` as a separate crate because, as explained earlier, this is a temporary measure, and we don't want to pollute crates.io's namespace. We can't make the necessary changes to the upstream `rlsf` because maintaining the compatibility with a stable compiler is very likely to pose a considerable challenge and unacceptable maintenance burdens. [1]: rust-lang/rust#92274
|
As now, as far as I can think of, there is two choices to implement the intrinsics. (Just to add, the reason why I try to change the behavior of
Please let me know if there's a better solution. |
|
The Until then, we could add something to the pre codegen cleanup that replaces just |
Assuming that we do it with rustc_mir_transform, is there any guarantee that the compile-time argument will not be codegen'd after the replacement? (if the argument isn't used by others) I think it's troublesome but reimplementing |
|
We have separate MIR for CTFE and for runtime. As long as this opt is in the |
|
Wait I misunderstood. That is not an issue I think, with this change, the monomorphization collector should be able to do everything correctly |
|
I added mirpass which replaces, but I still get a codegen error... It looks like we still need |
This comment has been minimized.
This comment has been minimized.
oli-obk
reviewed
Jan 25, 2022
oli-obk
reviewed
Jan 25, 2022
|
Do you have a backtrace available? Is it encountering a call to that intrinsic or is it just trying to generate code so people can call the intrinsic in downstream crates? |
|
The codegen error occurs during building Backtrace:The following is mir-dump result of transforming this with ReplaceConstEvalSelect. Before:After: |
|
|
lilasta
commented
Jan 25, 2022
| let terminator = block.terminator.as_mut().unwrap(); | ||
| if let TerminatorKind::Call { func, args, .. } = &mut terminator.kind { | ||
| if let ty::FnDef(def_id, _) = func.ty(local_decls, tcx).kind() { | ||
| if Some(*def_id) == tcx.lang_items().const_eval_select() { |
There was a problem hiding this comment.
maybe...
❌ if Some(*def_id) == tcx.lang_items().const_eval_select()
⭕ if intrinsic_name == sym::const_eval_select
I misunderstood... These are equivalent...
There was a problem hiding this comment.
hmm... it would be weird if that fixes it. while comparing the name is ok, going via the def id takes less work and I don't see why it shouldn't work.
There was a problem hiding this comment.
So... one thing that could in theory happen (but doesn't happen here) is that we create a function pointer to const_eval_select calling something, and then it would get monomorphized.
From the backtrace it looks like we're codegenning the at_compiletime helper function. So... maybe this is happening because of the monomorphization collector. I was afraid of that ^^ let me grab you a link
There was a problem hiding this comment.
Ok... so... the collector will pick up the generics somewhere in the visitor in
at this point... I think maybe we should give up XD I'm sorry for sending you on a wild goose chase. Let's just codegen the intrinsics to abort or sth and be done with it. Unfortunate, but it seems to fragile to continue here.
There was a problem hiding this comment.
I think we cannot make it abort, it should just be a NOP at runtime.
There was a problem hiding this comment.
I think both intrinsics could just abort, as we can use const eval select for the runtime behavior. Implementing const_alloc cannot just be a nop, only dealloc can. So we'd need some complex logic anyway
…onst. Does nothing at runtime. `const_allocate`: Returns a null pointer at runtime.
oli-obk
reviewed
Jan 26, 2022
|
@bors r+ thanks for bearing with me during this back and forth! |
|
📌 Commit 7a7144f has been approved by |
bors
added
S-waiting-on-bors
RalfJung
reviewed
Jan 28, 2022
library/core/src/intrinsics.rs
Outdated
| #[rustc_const_unstable(feature = "const_heap", issue = "79597")] | ||
| pub fn const_allocate(size: usize, align: usize) -> *mut u8; | ||
|
|
||
| /// Deallocate a memory which allocated by `intrinsics::const_allocate` at compile time. |
There was a problem hiding this comment.
This should mention that it does nothing when the allocation was created by another constant than the one that is currently being evaluated.
|
@bors r- |
bors
added
S-waiting-on-author
|
@bors r+ |
|
📌 Commit 9728cc4 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 29, 2022
…askrgr Rollup of 8 pull requests Successful merges: - rust-lang#88205 (Add Explanation For Error E0772) - rust-lang#92274 (Add `intrinsics::const_deallocate`) - rust-lang#93236 (Make `NonNull::new` `const`) - rust-lang#93299 (Fix dot separator when there is no source link) - rust-lang#93410 (kmc-solid: Implement `net::FileDesc::duplicate`) - rust-lang#93424 (fix nit) - rust-lang#93431 (remove unused `jemallocator` crate) - rust-lang#93453 (Add GUI theme change test) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
yvt
added a commit
to r3-os/r3
that referenced
this pull request
Feb 3, 2022
… a null pointer The change to `const_allocate`'s behavior was introduced by [rust-lang/rust#92274][1]. This means two things: For one, we no longer need `const_eval_select` to keep the compiler from panicking. For two, it can now return a null pointer, for which we must be prepared. [1]: rust-lang/rust#92274
yvt
added a commit
to r3-os/r3
that referenced
this pull request
Feb 3, 2022
…ing with `rlsf` Since CTFE-heap deallocation is now [supported natively][1], we don't need our own sub-allocator anymore. This also speeds up the compilation. [1]: rust-lang/rust#92274
yvt
added a commit
to r3-os/r3
that referenced
this pull request
Feb 3, 2022
…ting with `rlsf` Since CTFE-heap deallocation is now [supported natively][1], we don't need our own sub-allocator anymore. This also speeds up the compilation. [1]: rust-lang/rust#92274
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tracking issue: #79597
Related: #91884
This allows deallocation of a memory allocated by
intrinsics::const_allocate. At the moment, this can be only used to reduce memory usage, but in the future this may be useful to detect memory leaks (If an allocated memory remains after evaluation, raise an error...?).