Put Local, Static and Promoted as one Base variant of Place #58631

spastorino · 2019-02-22T04:29:03Z

Related to #52708

The Place 2.0 representation use a Base variant for Local, Static and Promoted so we start making this change in the current Place to make the following steps simpler.

r? @oli-obk

oli-obk

lgtm

Note that this will cause a max-rss regression, so we'll merge it after the beta cutoff, so we have 6 weeks to fix it (which should happen with the enum Place -> struct Place refactoring discussed in https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/place.202.2E0/near/159082516

oli-obk · 2019-02-22T09:01:49Z

src/librustc_mir/borrow_check/error_reporting.rs

@@ -1813,7 +1815,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
    fn classify_drop_access_kind(&self, place: &Place<'tcx>) -> StorageDeadOrDrop<'tcx> {
        let tcx = self.infcx.tcx;
        match place {
-            Place::Local(_) | Place::Static(_) | Place::Promoted(_) => {
+            Place::Base(PlaceBase::Local(_)) |
+                Place::Base(PlaceBase::Static(_)) |


the indent is off here

Yeah, vim does that :). Fixing it.

oli-obk · 2019-02-22T09:02:46Z

src/librustc_mir/borrow_check/error_reporting.rs

@@ -1910,7 +1915,10 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                    "annotate_argument_and_return_for_borrow: target={:?} stmt={:?}",
                    target, stmt
                );
-                if let StatementKind::Assign(Place::Local(assigned_to), box rvalue) = &stmt.kind
+                if let StatementKind::Assign(
+                            Place::Base(PlaceBase::Local(assigned_to)),


odd indentation here. Maybe write

if let StatementKind::Assign( Place::Base(PlaceBase::Local(assigned_to)), box rvalue, ) = &stmt.kind {

oli-obk · 2019-02-22T09:04:03Z

src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs

@@ -384,7 +384,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                        // Just point to the function, to reduce the chance of overlapping spans.
                        let function_span = match func {
                            Operand::Constant(c) => c.span,
-                            Operand::Copy(Place::Local(l)) | Operand::Move(Place::Local(l)) => {
+                            Operand::Copy(Place::Base(PlaceBase::Local(l))) |
+                                Operand::Move(Place::Base(PlaceBase::Local(l))) => {


use the same indent as the Operand::Copy

oli-obk · 2019-02-22T09:04:11Z

src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs

-                            Operand::Copy(Place::Local(from)) |
-                            Operand::Move(Place::Local(from)) if *from == target => {
+                            Operand::Copy(Place::Base(PlaceBase::Local(from))) |
+                                Operand::Move(Place::Base(PlaceBase::Local(from)))


oli-obk · 2019-02-22T09:04:18Z

src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs

-                            Operand::Copy(Place::Local(from)) |
-                            Operand::Move(Place::Local(from)) if *from == target => {
+                            Operand::Copy(Place::Base(PlaceBase::Local(from))) |
+                                Operand::Move(Place::Base(PlaceBase::Local(from)))


oli-obk · 2019-02-22T09:05:58Z

src/librustc_mir/borrow_check/prefixes.rs

-                Place::Static(_) => {
+                Place::Base(PlaceBase::Promoted(_)) |
+                Place::Base(PlaceBase::Local(_)) | // search yielded this leaf
+                Place::Base(PlaceBase::Static(_)) => {


same here, just use the Place::Base(_) pattern

Same as above.

oli-obk · 2019-02-22T09:07:03Z

src/librustc_mir/build/expr/as_rvalue.rs


                // initialize the box contents:
-                unpack!(block = this.into(&Place::Local(result).deref(), block, value));


pull Place::Local(result) into a variable before this statement and use that variable here and in the next statement

deref takes ownership so that's not possible

oli-obk · 2019-02-22T09:09:01Z

src/librustc_mir/build/mod.rs

@@ -956,7 +956,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
        }

        let body = self.hir.mirror(ast_body);
-        self.into(&Place::Local(RETURN_PLACE), block, body)
+        self.into(&Place::Base(PlaceBase::Local(RETURN_PLACE)), block, body)


Maybe add a Place::RETURN_PLACE associated constant to Place to make all of these Place::Base(PlaceBase::Local(RETURN_PLACE)) shorter.

oli-obk · 2019-02-22T09:10:49Z

src/librustc_mir/transform/add_retag.rs

-        Local { .. } |
-        Promoted { .. } |
-        Static { .. } =>
+        Base(PlaceBase::Local { .. }) |


Just use Base(_)

oli-obk · 2019-02-22T09:13:59Z

src/librustc_mir/util/elaborate_drops.rs

        let length_or_end = if ptr_based {
-            Place::Local(self.new_temp(iter_ty))
+            Place::Base(PlaceBase::Local(self.new_temp(iter_ty)))


Add a FIXME to new_temp to check if we want to make it return a Place directly if all use sites want a Place::Base anyway.

bors · 2019-02-22T09:47:27Z

☔ The latest upstream changes (presumably #56113) made this pull request unmergeable. Please resolve the merge conflicts.

spastorino · 2019-02-23T04:17:12Z

@oli-obk fixed, rebased and force pushed :).

oli-obk · 2019-02-23T08:22:25Z

@bors r+

bors · 2019-02-23T08:22:26Z

📌 Commit 4b9d48f0a4f006480f64bb16b56714fef9f66db3 has been approved by oli-obk

Centril · 2019-02-23T08:23:56Z

@bors p=1

matthewjasper · 2019-02-23T17:42:44Z

so we'll merge it after the beta cutoff

@oli-obk What happened to this?

bors · 2019-02-24T11:17:35Z

⌛ Testing commit 4b9d48f0a4f006480f64bb16b56714fef9f66db3 with merge 8601df9996e847f042bc7a4c9d97fad4b6b0448a...

bors · 2019-02-24T12:56:06Z

💔 Test failed - checks-travis

rust-highfive · 2019-02-24T12:56:07Z

The job x86_64-gnu-tools of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[01:19:36]    Compiling clippy_lints v0.0.212 (/checkout/src/tools/clippy/clippy_lints)
[01:19:39] error[E0531]: cannot find tuple struct/variant `Local` in this scope
[01:19:39]    --> src/tools/clippy/clippy_lints/src/redundant_clone.rs:298:13
[01:19:39]     |
[01:19:39] 298 |             Local(local) => return Some((*local, deref || field)),
[01:19:39] help: possible candidates are found in other modules, you can import them into scope
[01:19:39]     |
[01:19:39] 1   | use rustc::hir::Node::Local;
[01:19:39]     |
---
[01:19:39] 
[01:19:40] error[E0531]: cannot find tuple struct/variant `Local` in this scope
[01:19:40]    --> src/tools/clippy/clippy_lints/src/redundant_clone.rs:298:13
[01:19:40]     |
[01:19:40] 298 |             Local(local) => return Some((*local, deref || field)),
[01:19:40] help: possible candidates are found in other modules, you can import them into scope
[01:19:40]     |
[01:19:40] 1   | use rustc::hir::Node::Local;
[01:19:40]     |
---
[01:19:40] 
[01:19:43] error[E0599]: no variant named `Local` found for type `rustc::mir::Place<'_>` in the current scope
[01:19:43]    --> src/tools/clippy/clippy_lints/src/redundant_clone.rs:148:44
[01:19:43]     |
[01:19:43] 148 |                     if *res == mir::Place::Local(cloned);
[01:19:43]     |                                |
[01:19:43]     |                                variant not found in `rustc::mir::Place<'_>`
[01:19:43]     |                                help: did you mean: `local`
[01:19:43] 
[01:19:43] 
[01:19:43] error[E0599]: no variant named `Local` found for type `rustc::mir::Place<'_>` in the current scope
[01:19:43]    --> src/tools/clippy/clippy_lints/src/redundant_clone.rs:234:47
[01:19:43]     |
[01:19:43] 234 |         if let mir::Operand::Move(mir::Place::Local(local)) = &args[0];
[01:19:43]     |                                   |
[01:19:43]     |                                   variant not found in `rustc::mir::Place<'_>`
[01:19:43]     |                                   help: did you mean: `local`
[01:19:43] 
[01:19:43] 
[01:19:43] error[E0599]: no variant named `Local` found for type `rustc::mir::Place<'_>` in the current scope
[01:19:43]    --> src/tools/clippy/clippy_lints/src/redundant_clone.rs:260:59
[01:19:43]     |
[01:19:43] 260 |             if let mir::StatementKind::Assign(mir::Place::Local(local), v) = &stmt.kind {
[01:19:43]     |                                               |
[01:19:43]     |                                               variant not found in `rustc::mir::Place<'_>`
[01:19:43]     |                                               help: did you mean: `local`
[01:19:43] 
[01:19:43] 
[01:19:43] error[E0599]: no variant named `Local` found for type `rustc::mir::Place<'_>` in the current scope
[01:19:43]    --> src/tools/clippy/clippy_lints/src/redundant_clone.rs:148:44
[01:19:43]     |
[01:19:43] 148 |                     if *res == mir::Place::Local(cloned);
[01:19:43]     |                                |
[01:19:43]     |                                variant not found in `rustc::mir::Place<'_>`
[01:19:43]     |                                help: did you mean: `local`
[01:19:43] 
[01:19:43] 
[01:19:43] error[E0599]: no variant named `Local` found for type `rustc::mir::Place<'_>` in the current scope
[01:19:43]    --> src/tools/clippy/clippy_lints/src/redundant_clone.rs:234:47
[01:19:43]     |
[01:19:43] 234 |         if let mir::Operand::Move(mir::Place::Local(local)) = &args[0];
[01:19:43]     |                                   |
[01:19:43]     |                                   variant not found in `rustc::mir::Place<'_>`
[01:19:43]     |                                   help: did you mean: `local`
[01:19:43] 
[01:19:43] 
[01:19:43] error[E0599]: no variant named `Local` found for type `rustc::mir::Place<'_>` in the current scope
[01:19:43]    --> src/tools/clippy/clippy_lints/src/redundant_clone.rs:260:59
[01:19:43]     |
[01:19:43] 260 |             if let mir::StatementKind::Assign(mir::Place::Local(local), v) = &stmt.kind {
[01:19:43]     |                                               |
[01:19:43]     |                                               variant not found in `rustc::mir::Place<'_>`
[01:19:43]     |                                               help: did you mean: `local`
[01:19:43] 
---
[01:23:58]    Compiling rls-analysis v0.16.12
[01:24:01] error[E0531]: cannot find tuple struct/variant `Local` in this scope
[01:24:01]    --> src/tools/clippy/clippy_lints/src/redundant_clone.rs:298:13
[01:24:01]     |
[01:24:01] 298 |             Local(local) => return Some((*local, deref || field)),
[01:24:01] help: possible candidates are found in other modules, you can import them into scope
[01:24:01]     |
[01:24:01] 1   | use rustc::hir::Node::Local;
[01:24:01]     |
---
[01:24:01] 
[01:24:06] error[E0599]: no variant named `Local` found for type `rustc::mir::Place<'_>` in the current scope
[01:24:06]    --> src/tools/clippy/clippy_lints/src/redundant_clone.rs:148:44
[01:24:06]     |
[01:24:06] 148 |                     if *res == mir::Place::Local(cloned);
[01:24:06]     |                                |
[01:24:06]     |                                variant not found in `rustc::mir::Place<'_>`
[01:24:06]     |                                help: did you mean: `local`
[01:24:06] 
[01:24:06] 
[01:24:06] error[E0599]: no variant named `Local` found for type `rustc::mir::Place<'_>` in the current scope
[01:24:06]    --> src/tools/clippy/clippy_lints/src/redundant_clone.rs:234:47
[01:24:06]     |
[01:24:06] 234 |         if let mir::Operand::Move(mir::Place::Local(local)) = &args[0];
[01:24:06]     |                                   |
[01:24:06]     |                                   variant not found in `rustc::mir::Place<'_>`
[01:24:06]     |                                   help: did you mean: `local`
[01:24:06] 
[01:24:06] 
[01:24:06] error[E0599]: no variant named `Local` found for type `rustc::mir::Place<'_>` in the current scope
[01:24:06]    --> src/tools/clippy/clippy_lints/src/redundant_clone.rs:260:59
[01:24:06]     |
[01:24:06] 260 |             if let mir::StatementKind::Assign(mir::Place::Local(local), v) = &stmt.kind {
[01:24:06]     |                                               |
[01:24:06]     |                                               variant not found in `rustc::mir::Place<'_>`
[01:24:06]     |                                               help: did you mean: `local`
[01:24:06] 
---
[01:30:05]    Compiling miri v0.1.0 (/checkout/src/tools/miri)
[01:30:07] error[E0599]: no variant named `Local` found for type `rustc::mir::Place<'_>` in the current scope
[01:30:07]    --> src/tools/miri/src/lib.rs:125:44
[01:30:07]     |
[01:30:07] 125 |     let dest = ecx.eval_place(&mir::Place::Local(args.next().unwrap()))?;
[01:30:07]     |                                |
[01:30:07]     |                                variant not found in `rustc::mir::Place<'_>`
[01:30:07]     |                                help: did you mean: `local`
[01:30:07] 
[01:30:07] 
[01:30:07] error[E0599]: no variant named `Local` found for type `rustc::mir::Place<'_>` in the current scope
[01:30:07]    --> src/tools/miri/src/lib.rs:129:44
[01:30:07]     |
[01:30:07] 129 |     let dest = ecx.eval_place(&mir::Place::Local(args.next().unwrap()))?;
[01:30:07]     |                                |
[01:30:07]     |                                variant not found in `rustc::mir::Place<'_>`
[01:30:07]     |                                help: did you mean: `local`
[01:30:07] 
[01:30:07] 
[01:30:07] error[E0599]: no variant named `Local` found for type `rustc::mir::Place<'_>` in the current scope
[01:30:07]    --> src/tools/miri/src/lib.rs:141:44
[01:30:07]     |
[01:30:07] 141 |     let dest = ecx.eval_place(&mir::Place::Local(args.next().unwrap()))?;
[01:30:07]     |                                |
[01:30:07]     |                                variant not found in `rustc::mir::Place<'_>`
[01:30:07]     |                                help: did you mean: `local`
[01:30:07] 
[01:30:07] 
[01:30:07] error[E0599]: no variant named `Local` found for type `rustc::mir::Place<'_>` in the current scope
[01:30:07]    --> src/tools/miri/src/lib.rs:440:47
[01:30:07]     |
[01:30:07] 440 |         let arg = ecx.eval_place(&mir::Place::Local(args.next().unwrap()))?;
[01:30:07]     |                                   |
[01:30:07]     |                                   variant not found in `rustc::mir::Place<'_>`
[01:30:07]     |                                   help: did you mean: `local`
[01:30:07] 
[01:30:07] 
[01:30:07] error[E0599]: no variant named `Local` found for type `rustc::mir::Place<'_>` in the current scope
[01:30:07]    --> src/tools/miri/src/lib.rs:445:47
[01:30:07]     |
[01:30:07] 445 |         let arg = ecx.eval_place(&mir::Place::Local(args.next().unwrap()))?;
[01:30:07]     |                                   |
[01:30:07]     |                                   variant not found in `rustc::mir::Place<'_>`
[01:30:07]     |                                   help: did you mean: `local`
[01:30:07] 
[01:30:07] 
[01:30:07] error[E0599]: no variant named `Local` found for type `rustc::mir::Place<'_>` in the current scope
[01:30:07]    --> src/tools/miri/src/fn_call.rs:271:61
[01:30:07]     |
[01:30:07] 271 |                 let arg_dest = this.eval_place(&mir::Place::Local(arg_local))?;
[01:30:07]     |                                                 |
[01:30:07]     |                                                 variant not found in `rustc::mir::Place<'_>`
[01:30:07]     |                                                 help: did you mean: `local`
[01:30:07] 
[01:30:07] 
[01:30:08] error[E0599]: no variant named `Local` found for type `rustc::mir::Place<'_>` in the current scope
[01:30:08]    --> src/tools/miri/src/tls.rs:154:53
[01:30:08]     |
[01:30:08] 154 |             let dest = this.eval_place(&mir::Place::Local(arg_local))?;
[01:30:08]     |                                         |
[01:30:08]     |                                         variant not found in `rustc::mir::Place<'_>`
[01:30:08]     |                                         help: did you mean: `local`
[01:30:08] 
---
travis_time:end:028bd270:start=1551012905615709395,finish=1551012905623469110,duration=7759715
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:070c78b0
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:02e8fc68
travis_time:start:02e8fc68
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:2705f757
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

bors · 2019-02-25T09:26:44Z

☔ The latest upstream changes (presumably #57609) made this pull request unmergeable. Please resolve the merge conflicts.

spastorino · 2019-02-27T17:25:35Z

@oli-obk just in case conversation in Zulip went unnoticed. This is ready to r+.

oli-obk · 2019-02-27T17:34:44Z

@bors r+

Centril · 2019-03-01T04:05:22Z

Seems to be a bit-rotty PR, so @bors p=20

oli-obk · 2019-03-01T09:10:43Z

@bors r+

bors · 2019-03-01T09:10:44Z

📌 Commit 0f993d5 has been approved by oli-obk

bors · 2019-03-01T10:00:29Z

⌛ Testing commit 0f993d5 with merge 17add24...

@oli-obk

Put Local, Static and Promoted as one Base variant of Place Related to #52708 The `Place` 2.0 representation use a `Base` variant for `Local`, `Static` and `Promoted` so we start making this change in the current `Place` to make the following steps simpler. r? @oli-obk

bors · 2019-03-01T13:30:34Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing 17add24 to master...

rust-highfive · 2019-03-01T13:31:46Z

📣 Toolstate changed by #58631!

Tested on commit 17add24.
Direct link to PR: #58631

💔 miri on windows: test-fail → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
💔 miri on linux: test-fail → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).

@oli-obk

Tested on commit rust-lang/rust@17add24. Direct link to PR: <rust-lang/rust#58631> 💔 miri on windows: test-fail → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra). 💔 miri on linux: test-fail → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).

@oli-obk

Place::Local(x) is now Place::Base(PlaceBase::Local(x)) We need to merge this after the beta cut for this rust-lang/rust#58631 to work. /cc @oli-obk

nnethercote · 2019-04-02T04:21:57Z

FWIW, I was just looking at peak memory usage and I identified the Place/PlaceBase split as a source of wasted space and I started combining them and then I thought "hmm..." and I used git blame to find this PR.

In other words, I noticed this change.

spastorino · 2019-04-02T21:23:13Z

@nnethercote this was an intermediate step, we are going after this ...

struct Place {
    base: PlaceBase,
    projection: PlaceProjection,
}
enum PlaceProjection {
    Base,
    Projection(Box<PlaceProjection>),
    Deref,
    Index(..),
    ...
}

Going to provide a PR for this probably this week.

matthewjasper · 2019-04-02T21:46:38Z

That's makes Place even larger.

oli-obk · 2019-04-03T07:16:38Z

Since the final layout will be

struct Place<'tcx> {
    base: PlaceBase,
    projection: &'tcx [PlaceProjection],
}
enum PlaceProjection {
    Projection(Box<PlaceProjection>),
    Deref,
    Index(..),
    ...
}

we can also box Place at all/most use sites, so any uses of it will just be one pointer sized.

I think that ideally we'd be able to have all occurences of Place be &'tcx Place and the type be

struct Place {
    base: PlaceBase,
    projection: [PlaceProjection],
}

But I don't know how to initialize such a type without statically knowing the length of the projection field.

eddyb · 2019-04-03T08:37:15Z

FWIW &'tcx [PlaceProjection] is not an interned projection list.
Look at ty::List, which is how the interner keeps things, and note that the length is behind the pointer.

Also, we shouldn't combine the base and the projections, IMO.
Ideally the base can change without the projections needing reinterning (think renaming locals).

eddyb · 2019-04-03T08:42:12Z

I expect PlaceBase to be 4 bytes in size eventually (locals- anything other than a local should always be accessed via a borrow, which can allow e.g. borrowing statics/promoteds - by-value access for those doesn't really make sense anyway)

And a ty::List<PlaceProjection> pointer is thin, so Place would be twice the pointer width.

@nnethercote Does that work for you?

spastorino · 2019-04-03T12:32:38Z

That's makes Place even larger.

Yes, sorry. I've pasted an intermediate step. The final layout as @eddyb has said is ...

struct Place<'tcx> {
    base: PlaceBase,
    projection: &'tcx [PlaceProjection],
}
enum PlaceProjection {
    Projection(Box<PlaceProjection>),
    Deref,
    Index(..),
    ...
}

oli-obk · 2019-04-03T12:36:36Z

Not quite, @eddyb is right, &'tcx [T] is a pointer and a length, while &'tcx ty::List<T> is one pointer, so we'll want to have

struct Place<'tcx> {
    base: PlaceBase,
    projection: &'tcx ty::List<PlaceProjection>,
}
enum PlaceProjection {
    Projection(Box<PlaceProjection>),
    Deref,
    Index(..),
    ...
}

RalfJung · 2019-04-03T13:04:43Z

What is the point of the recursive Projection(Box<PlaceProjection>) variant?

oli-obk · 2019-04-03T13:15:07Z

Ups, that's just a copy paste bug. In the List situation we won't have any more recursion in the future, that's just a leftover right now. The next step is actually

enum Place {
    Base(PlaceBase)
    Projection(Box<PlaceProjection>),
}
struct PlaceProjection {
    base: Place,
    projection: PlaceProjectionElem,
}
enum PlaceProjectionElem {
    Leaf,
    Deref,
    Index(..),
    ...
}

while the target situation is

struct Place {
    base: PlaceBase
    projections: &'tcx ty::List<PlaceProjectionElem>,
}
// Note: no more PlaceProjection
enum PlaceProjectionElem {
    // Note: no more "Leaf"
    Deref,
    Index(..),
    ...
}

nnethercote · 2019-04-04T04:38:57Z

@nnethercote Does that work for you?

I'm a bit confused by all the above back and forth. Place was 16 bytes before this started, which was good. Then it became 24 bytes, which is worse. 32 bytes would be even worse. Smaller is better.

eddyb · 2019-04-04T14:57:21Z

@nnethercote The plan is for it to become 16 bytes (PlaceBase the size of Local, plus a &'tcx ty::List<PlaceProjectionElem> which is pointer-sized).
But it will get worse before it gets better, otherwise we'll never get where we want to get.

But also, Place being small is not by itself good. It was/is a linked list, which is much worse than an array.

spastorino · 2019-04-04T16:36:30Z

Yeah, sorry my bad I was confusing you a lot. Thanks @eddyb for clarying it.
The last version as stated will be 24 bytes.

nnethercote · 2019-04-04T21:27:55Z

But also, Place being small is not by itself good.

I can see that an array is better than a linked list. But note that the size of Place affects the size of mir::Statement So even statements that don't contain a Place field get bigger if Place gets bigger. And the size of mir::Statement affects peak memory size.

Anyway, if the final size will be 16 bytes, that's great.

rust-highfive assigned oli-obk Feb 22, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 22, 2019

oli-obk requested changes Feb 22, 2019

View reviewed changes

spastorino force-pushed the place2_1 branch 2 times, most recently from 7e9b0f8 to 4b9d48f Compare February 23, 2019 04:15

oli-obk approved these changes Feb 23, 2019

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 Feb 23, 2019

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 24, 2019

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 24, 2019

This was referenced Feb 26, 2019

Place::Local(x) is now Place::Base(PlaceBase::Local(x)) rust-lang/miri#647

Merged

Place::Local(x) is now Place::Base(PlaceBase::Local(x)) rust-lang/rust-clippy#3823

Merged

spastorino force-pushed the place2_1 branch 2 times, most recently from 3534f50 to 4cfe141 Compare February 26, 2019 19:22

rust-lang deleted a comment from rust-highfive Feb 27, 2019

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 1, 2019

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 1, 2019

bors merged commit 0f993d5 into rust-lang:master Mar 1, 2019

rust-highfive mentioned this pull request Mar 1, 2019

miri no longer builds after rust-lang/rust#58631 #58841

Closed

bors mentioned this pull request Mar 1, 2019

Include bounds from promoted constants in NLL #57202

Merged

spastorino mentioned this pull request Apr 4, 2019

change how MIR represents places #52708

Closed


		// initialize the box contents:
		unpack!(block = this.into(&Place::Local(result).deref(), block, value));

Put Local, Static and Promoted as one Base variant of Place #58631

Put Local, Static and Promoted as one Base variant of Place #58631

Conversation

spastorino commented Feb 22, 2019

oli-obk left a 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

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

bors commented Feb 22, 2019

spastorino commented Feb 23, 2019

oli-obk commented Feb 23, 2019

bors commented Feb 23, 2019

Centril commented Feb 23, 2019

matthewjasper commented Feb 23, 2019

bors commented Feb 24, 2019

bors commented Feb 24, 2019

rust-highfive commented Feb 24, 2019

bors commented Feb 25, 2019

spastorino commented Feb 27, 2019

oli-obk commented Feb 27, 2019

Centril commented Mar 1, 2019

oli-obk commented Mar 1, 2019

bors commented Mar 1, 2019

bors commented Mar 1, 2019

bors commented Mar 1, 2019

rust-highfive commented Mar 1, 2019

nnethercote commented Apr 2, 2019

spastorino commented Apr 2, 2019

matthewjasper commented Apr 2, 2019 • edited Loading

oli-obk commented Apr 3, 2019

eddyb commented Apr 3, 2019

eddyb commented Apr 3, 2019

spastorino commented Apr 3, 2019

oli-obk commented Apr 3, 2019

RalfJung commented Apr 3, 2019

oli-obk commented Apr 3, 2019

nnethercote commented Apr 4, 2019

eddyb commented Apr 4, 2019

spastorino commented Apr 4, 2019

nnethercote commented Apr 4, 2019

matthewjasper commented Apr 2, 2019 •

edited

Loading