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_allocate intrinsic #79594

Merged
merged 6 commits into from
Dec 3, 2020
Merged

Conversation

vn-ki
Copy link
Contributor

@vn-ki vn-ki commented Dec 1, 2020

r? @oli-obk

fixes #75390

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 1, 2020
@vn-ki vn-ki changed the title add const_allocate intrisic add const_allocate intrinsic Dec 1, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Dec 1, 2020

cc @rust-lang/wg-const-eval

compiler/rustc_mir/src/interpret/memory.rs Outdated Show resolved Hide resolved
library/core/src/intrinsics.rs Outdated Show resolved Hide resolved
#![feature(const_mut_refs)]
use std::intrinsics;

const FOO: *const i32 = foo();
Copy link
Contributor

Choose a reason for hiding this comment

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

It's super curious that this doesn't error with the same error that src/test/ui/consts/const-eval/heap/alloc_intrinsic_untyped.rs emits: "untyped pointer".

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh... I think I know why... foo is a function without any arguments, and is thus not actually called but treated as a constant itself and its body is thus evaluated directly:

// For the moment we only do this for functions which take no arguments
// (or all arguments are ZSTs) so that we don't memoize too much.
if args.iter().any(|a| !a.layout.is_zst()) {
return Ok(false);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we'll need to disable that optimization for function items now

Copy link
Member

Choose a reason for hiding this comment

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

I guess we'll need to disable that optimization for function items now

Why that?

Copy link
Contributor

Choose a reason for hiding this comment

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

This test memoizes function alls to foo(), even though every call should be producing a new heap allocation. Otherwise all calls to Box::new_uninit() will end up with the same heap allocation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Basically if you used

const fn foo() -> &'static i32 {
    const fn bar() -> *mut i32 {
        unsafe { intrinsics::const_allocate(4, 4) as *mut i32 }
    }
    unsafe {
        let i = bar();
        *i = 20;
        ...
    }
}

the *i = 20 will fail to interpret because you'd be modifying interned memory

Copy link
Member

Choose a reason for hiding this comment

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

Oh, lol... yeah, const fn without inputs are no longer pure with this change, That's kind of a big deal actually. We certainly need T-lang approval before stabilizing this.

@oli-obk oli-obk mentioned this pull request Dec 1, 2020
3 tasks
@@ -104,7 +104,7 @@ fn intern_shallow<'rt, 'mir, 'tcx, M: CompileTimeMachine<'mir, 'tcx>>(
// This match is just a canary for future changes to `MemoryKind`, which most likely need
// changes in this function.
match kind {
MemoryKind::Stack | MemoryKind::Vtable | MemoryKind::CallerLocation => {}
MemoryKind::Stack | MemoryKind::Heap | MemoryKind::Vtable | MemoryKind::CallerLocation => {}
Copy link
Member

Choose a reason for hiding this comment

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

Should Miri also use this Heap variant? Or should we rename it to ConstHeap?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, good point. I think miri will need its own heap variant to prevent us from accidentally mixing the two somewhere.

Copy link
Member

Choose a reason for hiding this comment

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

@oli-obk so what do you think about using a non-! MemoryKind for the CTFE machine?

Copy link
Contributor

@oli-obk oli-obk Dec 2, 2020

Choose a reason for hiding this comment

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

Ah, apologies, forgot to post after discussing with @vn-ki . @vn-ki experimented with that, but it's a nontrivial change, because ConstProp reuses a lot of the same machinery. I am still in favor of doing that change, but in a follow up PR that does just that single change

Copy link
Contributor

Choose a reason for hiding this comment

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

if you are worried about miri breakage, we can also do that change to an enum MemoryKind {} in a PR and then rebase this PR on top of it.

Copy link
Member

Choose a reason for hiding this comment

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

No I think this is fine (but please leave a FIXME in the enum definition).

Comment on lines 235 to 236
match instance.def {
InstanceDef::Intrinsic(_) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

ah sorry, I meant to run this check within try_eval_const_fn_call and return Ok(false) from it if it's not an intrinsic

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this new code. Don't we have to just entirely remove memoization?

Copy link
Contributor

Choose a reason for hiding this comment

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

we still memoize calls to size_of and align_of I think

Copy link
Member

Choose a reason for hiding this comment

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

Is that worth it all the code complexity here? Those functions are trivial to evaluate...

| inside `a` at $DIR/infinite-recursion-const-fn.rs:4:5
| inside `a` at $DIR/infinite-recursion-const-fn.rs:4:5
| inside `a` at $DIR/infinite-recursion-const-fn.rs:4:5
| inside `a` at $DIR/infinite-recursion-const-fn.rs:4:5
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this test gives worse errors now. Not sure how disabling memoization caused this.

Copy link
Member

Choose a reason for hiding this comment

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

In a sense this is actually a better error, since the full stack trace is printed now (#75390).

@oli-obk
Copy link
Contributor

oli-obk commented Dec 3, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Dec 3, 2020

📌 Commit bc6eb6f 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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 3, 2020
@oli-obk oli-obk added A-const-eval Area: Constant evaluation (MIR interpretation) A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. labels Dec 3, 2020
@bors
Copy link
Contributor

bors commented Dec 3, 2020

⌛ Testing commit bc6eb6f with merge d015f0d...

@bors
Copy link
Contributor

bors commented Dec 3, 2020

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 3, 2020
@bors bors merged commit d015f0d into rust-lang:master Dec 3, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 3, 2020
@bjorn3
Copy link
Member

bjorn3 commented Dec 4, 2020

This had a positive effect on the performance, especially for ctfe-stress for which it had an up to 2% improvement. https://perf.rust-lang.org/compare.html?start=c7cff213e937c1bb301be807ce04fcf6092b9163&end=d015f0d92144f0e72735a918aee8510b0fe2cff5&stat=instructions%3Au

@RalfJung
Copy link
Member

RalfJung commented Dec 4, 2020

That is probably caused by memoization being mostly disabled. Funny that memoization actually made things slower in the stress test, probably the memoized cache was rarely/never hit so maintaining that cache was just an unnecessary cost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. 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.

CTFE: nullary function memoization munges backtraces
7 participants