add const_allocate intrinsic #79594

vn-ki · 2020-12-01T10:11:53Z

r? @oli-obk

fixes #75390

oli-obk · 2020-12-01T10:14:12Z

cc @rust-lang/wg-const-eval

compiler/rustc_mir/src/interpret/memory.rs

library/core/src/intrinsics.rs

oli-obk · 2020-12-01T10:18:13Z

src/test/ui/consts/const-eval/heap/alloc_intrinsic_nontransient.rs

+#![feature(const_mut_refs)]
+use std::intrinsics;
+
+const FOO: *const i32 = foo();


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".

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:

rust/compiler/rustc_mir/src/const_eval/machine.rs

Lines 40 to 44 in 75f1db1

// 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);

}

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

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

Why that?

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.

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

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.

RalfJung · 2020-12-01T13:00:45Z

compiler/rustc_mir/src/interpret/intern.rs

@@ -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 => {}


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

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

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

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

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.

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

compiler/rustc_mir/src/interpret/intrinsics.rs

compiler/rustc_mir/src/interpret/memory.rs

oli-obk · 2020-12-01T14:25:32Z

compiler/rustc_mir/src/const_eval/machine.rs

+                match instance.def {
+                    InstanceDef::Intrinsic(_) => {


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

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

we still memoize calls to size_of and align_of I think

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

compiler/rustc_mir/src/const_eval/machine.rs

src/test/ui/infinite/infinite-recursion-const-fn.rs

src/test/ui/consts/uninhabited-const-issue-61744.rs

src/test/ui/consts/const-eval/heap/alloc_intrinsic_nontransient_fail.rs

compiler/rustc_mir/src/interpret/intrinsics.rs

vn-ki · 2020-12-03T07:28:31Z

src/test/ui/infinite/infinite-recursion-const-fn.stderr

+   |     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


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

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

oli-obk · 2020-12-03T09:23:31Z

@bors r+

bors · 2020-12-03T09:23:32Z

📌 Commit bc6eb6f has been approved by oli-obk

bors · 2020-12-03T09:44:14Z

⌛ Testing commit bc6eb6f with merge d015f0d...

bors · 2020-12-03T12:14:21Z

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

bjorn3 · 2020-12-04T09:00:27Z

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 · 2020-12-04T10:11:59Z

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.

add const_allocate intrisic

528355c

rust-highfive assigned oli-obk Dec 1, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 1, 2020

vn-ki changed the title ~~add const_allocate intrisic~~ add const_allocate intrinsic Dec 1, 2020

oli-obk reviewed Dec 1, 2020

View reviewed changes

oli-obk mentioned this pull request Dec 1, 2020

Tracking Issue for const_heap #79597

Open

3 tasks

RalfJung reviewed Dec 1, 2020

View reviewed changes

compiler/rustc_mir/src/interpret/intrinsics.rs Outdated Show resolved Hide resolved

RalfJung reviewed Dec 1, 2020

View reviewed changes

compiler/rustc_mir/src/interpret/memory.rs Outdated Show resolved Hide resolved

review comments

b5b811a

oli-obk reviewed Dec 1, 2020

View reviewed changes

review comment and one more test

a6c4cbd

RalfJung reviewed Dec 1, 2020

View reviewed changes

compiler/rustc_mir/src/const_eval/machine.rs Show resolved Hide resolved

add comment and bless some tests

1b7fe09

oli-obk reviewed Dec 2, 2020

View reviewed changes

src/test/ui/infinite/infinite-recursion-const-fn.rs Outdated Show resolved Hide resolved

oli-obk reviewed Dec 2, 2020

View reviewed changes

src/test/ui/consts/uninhabited-const-issue-61744.rs Outdated Show resolved Hide resolved

oli-obk reviewed Dec 2, 2020

View reviewed changes

src/test/ui/consts/const-eval/heap/alloc_intrinsic_nontransient_fail.rs Outdated Show resolved Hide resolved

rename MemoryKind::Heap to ConstHeap; bless test

899a59e

RalfJung reviewed Dec 2, 2020

View reviewed changes

compiler/rustc_mir/src/interpret/intrinsics.rs Outdated Show resolved Hide resolved

move intrinsic to CTFE, add FIXME

bc6eb6f

vn-ki commented Dec 3, 2020

View reviewed changes

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 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 added the merged-by-bors This PR was explicitly merged by bors. label Dec 3, 2020

bors merged commit d015f0d into rust-lang:master Dec 3, 2020

rustbot added this to the 1.50.0 milestone Dec 3, 2020

RalfJung mentioned this pull request Dec 3, 2020

CTFE: remove memoization leftovers #79667

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add const_allocate intrinsic #79594

add const_allocate intrinsic #79594

vn-ki commented Dec 1, 2020 •

edited

Loading

oli-obk commented Dec 1, 2020

oli-obk Dec 1, 2020

oli-obk Dec 1, 2020

oli-obk Dec 1, 2020

RalfJung Dec 1, 2020

oli-obk Dec 1, 2020

oli-obk Dec 1, 2020

RalfJung Dec 1, 2020

RalfJung Dec 1, 2020

oli-obk Dec 1, 2020

RalfJung Dec 2, 2020

oli-obk Dec 2, 2020 •

edited

Loading

oli-obk Dec 2, 2020

RalfJung Dec 2, 2020

oli-obk Dec 1, 2020

RalfJung Dec 1, 2020

oli-obk Dec 1, 2020

RalfJung Dec 1, 2020

vn-ki Dec 3, 2020

RalfJung Dec 3, 2020

oli-obk commented Dec 3, 2020

bors commented Dec 3, 2020

bors commented Dec 3, 2020

bors commented Dec 3, 2020

bjorn3 commented Dec 4, 2020

RalfJung commented Dec 4, 2020

	// 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);
	}

add const_allocate intrinsic #79594

add const_allocate intrinsic #79594

Conversation

vn-ki commented Dec 1, 2020 • edited Loading

oli-obk commented Dec 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Dec 3, 2020

bors commented Dec 3, 2020

bors commented Dec 3, 2020

bors commented Dec 3, 2020

bjorn3 commented Dec 4, 2020

RalfJung commented Dec 4, 2020

vn-ki commented Dec 1, 2020 •

edited

Loading

oli-obk Dec 2, 2020 •

edited

Loading