Add `intrinsics::const_deallocate` by woppopo · Pull Request #92274 · rust-lang/...
source link: https://github.com/rust-lang/rust/pull/92274
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
New issue
Add intrinsics::const_deallocate
#92274
Conversation
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...?).
(rust-highfive has picked a reviewer for you, use r? to override)
r? @RalfJung
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.
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).
We already have const_eval_select for that. The intrinsic should imo not be implemented in backends
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.
That seems strange, the const function shouldn't even be codegen'd... right?
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.
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 const_allocate too is the intrinsic is possible to cause ICE easily. Currently, just including it in a runtime function will cause an ICE.)
- Use
const_eval_selectand#[inline(always)]to wrap the intrinsics:
const_(de)allocate isn't implemented in codegens, so we can't define a function which uses the intrinsic and wrap it with const_eval_select. (The compiler seems to generate a code for almost all functions found in a library crate) However, we may be able to avoid this problem by avoiding code generation with #[inline(always)]. If it always do inlining...
- Modify codegen to support calling the intrinsics at runtime.
This may be little bit troublesome since it needs to be implemented in all codegen backends. Just modify rustc_codegen_ssa and it will work. At runtime, const_allocate just returns a null pointer, and const_deallocate does nothing. I think this is fine.
Please let me know if there's a better solution.
The inline(always) solution seems like the right one, but the PR for that is a bit in the future.
Until then, we could add something to the pre codegen cleanup that replaces just const_eval_select with its runtime argument
Until then, we could add something to the pre codegen cleanup that replaces just const_eval_select with its runtime argument
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)
Or is there a pass to remove a dead code?
I think it's troublesome but reimplementing const_eval_select with builtin macro may be another candidate.
We have separate MIR for CTFE and for runtime. As long as this opt is in the optimized_mir optimizations, it will never end up in CTFE
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 #[inline(always)].
I have added the commit for your reference.
This comment has been hidden.
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 core with stage1 compiler.
Backtrace:
The following is mir-dump result of transforming this with ReplaceConstEvalSelect.
Before:
After:
Oh, I noticed that (perhaps) I made a mistake... I'll check it immediately.
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() {
maybe...
if Some(*def_id) == tcx.lang_items().const_eval_select()
if intrinsic_name == sym::const_eval_select
I misunderstood... These are equivalent...
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.
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
Ok... so... the collector will pick up the generics somewhere in the visitor in
impl<'a, 'tcx> MirVisitor<'tcx> for MirNeighborCollector<'a, 'tcx> {
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.
I think we cannot make it abort, it should just be a NOP at runtime.
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
force-pushed the const_deallocate
branch
3 times, most recently
from
94c8d78
to
29932db
22 days ago
@bors r+
thanks for bearing with me during this back and forth!
Commit 7a7144f has been approved by oli-obk
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.
This should mention that it does nothing when the allocation was created by another constant than the one that is currently being evaluated.
Documented.
@bors r-
@bors r+
Commit 9728cc4 has been approved by oli-obk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK