add const_allocate intrinsic by vn-ki · Pull Request #79594 · rust-lang/rust

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

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

cc @rust-lang/wg-const-eval

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

+#![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

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

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

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

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

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

Comment thread compiler/rustc_mir/src/interpret/intrinsics.rs Outdated

RalfJung reviewed Dec 1, 2020

View reviewed changes

Comment thread compiler/rustc_mir/src/interpret/memory.rs Outdated

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

Comment thread compiler/rustc_mir/src/const_eval/machine.rs

add comment and bless some tests

1b7fe09

oli-obk reviewed Dec 2, 2020

View reviewed changes

Comment thread src/test/ui/infinite/infinite-recursion-const-fn.rs Outdated

oli-obk reviewed Dec 2, 2020

View reviewed changes

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

oli-obk reviewed Dec 2, 2020

View reviewed changes

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

rename MemoryKind::Heap to ConstHeap; bless test

899a59e

RalfJung reviewed Dec 2, 2020

View reviewed changes

Comment thread compiler/rustc_mir/src/interpret/intrinsics.rs Outdated

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, covers all const contexts (static, const fn, ...) A-const-fn 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

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

Uh oh!

Conversation

vn-ki commented Dec 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oli-obk commented Dec 1, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oli-obk Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Dec 3, 2020

Uh oh!

bors commented Dec 3, 2020

Uh oh!

bors commented Dec 3, 2020

Uh oh!

bors commented Dec 3, 2020

Uh oh!

bjorn3 commented Dec 4, 2020

Uh oh!

RalfJung commented Dec 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vn-ki commented Dec 1, 2020 •

edited

Loading

oli-obk Dec 2, 2020 •

edited

Loading