Stream: t-compiler/wg-nll

Topic: #53643 optimize UCD


nikomatsakis (Aug 27 2018 at 12:52, on Zulip):

@mikhail-m1 hi =)

nikomatsakis (Aug 27 2018 at 12:53, on Zulip):

This was my minimized test case that I used to make compiles complete in a reasonable amount of time

nikomatsakis (Aug 27 2018 at 12:53, on Zulip):

in particular this constant is the one we are interested in

nikomatsakis (Aug 27 2018 at 12:53, on Zulip):

the temporaries result from expressions like this

 ((0,0,65), &[(0,0,97)]), ((0,0,66), &[(0,0,98)]), ((0,0,67), &[(0,0,99)]),
nikomatsakis (Aug 27 2018 at 12:54, on Zulip):

something like &[(0, 0, 97)] winds up generating MIR like:

nikomatsakis (Aug 27 2018 at 12:54, on Zulip):
TMP0 = (0, 0, 97)
TMP1 = [move TMP0]
TMP2 = &TMP1 // <-- these is the borrows that are killing us
nikomatsakis (Aug 27 2018 at 12:55, on Zulip):

example:

        _157 = (const 0u8, const 0u8, const 118u8); // bb0[305]: scope 0 at ucd.rs:14:16: 14:25
                                         // ty::Const
                                         // + ty: u8
                                         // + val: Scalar(Bits { size: 1, bits: 0 })
                                         // mir::Constant
                                         // + span: ucd.rs:14:17: 14:18
                                         // + ty: u8
                                         // + literal: Const { ty: u8, val: Scalar(Bits { size: 1, bits: 0 }) }
                                         // ty::Const
                                         // + ty: u8
                                         // + val: Scalar(Bits { size: 1, bits: 0 })
                                         // mir::Constant
                                         // + span: ucd.rs:14:19: 14:20
                                         // + ty: u8
                                         // + literal: Const { ty: u8, val: Scalar(Bits { size: 1, bits: 0 }) }
                                         // ty::Const
                                         // + ty: u8
                                         // + val: Scalar(Bits { size: 1, bits: 118 })
                                         // mir::Constant
                                         // + span: ucd.rs:14:21: 14:24
                                         // + ty: u8
                                         // + literal: Const { ty: u8, val: Scalar(Bits { size: 1, bits: 118 }) }
        _156 = [move _157];              // bb0[306]: scope 0 at ucd.rs:14:15: 14:26
        _155 = &_156;                    // bb0[307]: scope 0 at ucd.rs:14:14: 14:26
mikhail-m1 (Aug 29 2018 at 18:08, on Zulip):

hey @nikomatsakis , I'd like to discuss some issues with create immutable temporary variables, unfortunately immutable temporary vars are not expected now, i commented assert about it and now mir borrowck panics for

fn main() {
    match &mut &Some(5i32) {
        Some(n) => {
            *n += 1; //~ ERROR cannot assign to immutable
            let _ = n;
        }
        None => {},
    };
}

because don't know how to output error for temp var, there is another panic during compiling stdlib, but may be i create immutable temps to often :), I've made just POC implementation for the first step.

nikomatsakis (Aug 29 2018 at 18:15, on Zulip):

you mean you commented out this assert?

nikomatsakis (Aug 29 2018 at 18:15, on Zulip):

because don't know how to output error for temp var

hmm, interesting, can you say more about the error? did you push your branch anywhere?

nikomatsakis (Aug 29 2018 at 18:16, on Zulip):

I guess that it is some bit of code trying to suggest "maybe you should make this a mut variable" ?

nikomatsakis (Aug 29 2018 at 18:16, on Zulip):

in that case, we would presumably just disable the suggestion for now...

nikomatsakis (Aug 29 2018 at 18:16, on Zulip):

the libstd thing is a bit more concerning :)

mikhail-m1 (Aug 29 2018 at 18:19, on Zulip):

you mean you commented out this assert?

yes,

I will prevent a panic and check and collect more info..

mikhail-m1 (Aug 29 2018 at 18:29, on Zulip):

now three errors are emitted vs one:

error[E0594]: cannot assign to immutable borrowed content `*n` (Ast)
 --> test/x.rs:4:13
  |
4 |             *n += 1; //~ ERROR cannot assign to immutable
  |             ^^^^^^^ cannot borrow as mutable

error[E0596]: cannot borrow `temp` as mutable, as it is not declared as mutable (Mir)
 --> test/x.rs:2:11
  |
2 |     match &mut &Some(5i32) {
  |           ^^^^^^^^^^^^^^^^
  |           |
  |           cannot borrow as mutable
  |           try removing `&mut` here

error[E0594]: cannot assign to `*n` which is behind a `&` reference (Mir)
 --> test/x.rs:4:13
  |
4 |             *n += 1; //~ ERROR cannot assign to immutable
  |             ^^^^^^^ `n` is a `&` reference, so the data it refers to cannot be written

temp is a stub name, compiling libstd

nikomatsakis (Aug 29 2018 at 18:30, on Zulip):

cannot borrow temp as mutable, as it is not declared as mutable

nikomatsakis (Aug 29 2018 at 18:30, on Zulip):

that seems curious

nikomatsakis (Aug 29 2018 at 18:30, on Zulip):

can I see your diff?

nikomatsakis (Aug 29 2018 at 18:30, on Zulip):

(e.g., can you push commits somewhere?)

mikhail-m1 (Aug 29 2018 at 18:32, on Zulip):

creating it

mikhail-m1 (Aug 29 2018 at 18:39, on Zulip):

https://github.com/mikhail-m1/rust/commit/d1109b8c75e65aed079dba7128ff8723597342c9 I didn't change as_rval, just checking by trace

mikhail-m1 (Aug 29 2018 at 18:44, on Zulip):

MIR

    let mut _2: &'5s mut &'22s std::option::Option<i32>;
    let _3: &'22s std::option::Option<i32>;
    let mut _4: std::option::Option<i32>;
        _4 = std::option::Option<i32>::Some(const 5i32,); // bb0[4]: scope 0 at test/x.rs:2:17: 2:27
        _3 = &'22s _4;                   // bb0[6]: scope 0 at test/x.rs:2:16: 2:27
        _2 = &'5s mut _3;                // bb0[8]: scope 0 at test/x.rs:2:11: 2:27
 ```
nikomatsakis (Aug 29 2018 at 18:45, on Zulip):

see this comment, more in a sec

mikhail-m1 (Aug 29 2018 at 18:52, on Zulip):

see this comment, more in a sec

yes, right now i just check that as_place that create temp is called from as_rvalue Borrow, I think result will be the same for this sample, of cause it's wrong in general

nikomatsakis (Aug 29 2018 at 18:55, on Zulip):

the result is not the same

nikomatsakis (Aug 29 2018 at 18:55, on Zulip):

in particular

nikomatsakis (Aug 29 2018 at 18:55, on Zulip):

well, let me double check :)

nikomatsakis (Aug 29 2018 at 18:55, on Zulip):

right, I think it will not be the same

nikomatsakis (Aug 29 2018 at 18:55, on Zulip):

if you have EXPR1 defined as &EXPR2

nikomatsakis (Aug 29 2018 at 18:56, on Zulip):

what we want to do is to create an (immutable) temporary to store EXPR2

nikomatsakis (Aug 29 2018 at 18:56, on Zulip):

but we are creating, iiuc, an immutable temporary to store EXPR1

nikomatsakis (Aug 29 2018 at 18:57, on Zulip):

make sense?

nikomatsakis (Aug 29 2018 at 18:58, on Zulip):

this would, I believe, give rise to those spurious errors you are reporting... e.g., if we have &mut &Some(5i32), then we will create an immutable temp storing &Some(5i32), which can't be borrowed mutably

nikomatsakis (Aug 29 2018 at 18:58, on Zulip):

but in my version, the temp that stores &Some(5i32) would be mutable

nikomatsakis (Aug 29 2018 at 18:59, on Zulip):

(but the temp storing Some(5i32) would not)

mikhail-m1 (Aug 29 2018 at 18:59, on Zulip):

can you change the MIR sample?

nikomatsakis (Aug 29 2018 at 18:59, on Zulip):

yep

nikomatsakis (Aug 29 2018 at 19:00, on Zulip):
 let mut _2: &'5s mut &'22s std::option::Option<i32>;
    let mut _3: &'22s std::option::Option<i32>; // <-- changed to be mut
    let _4: std::option::Option<i32>; // <-- changed to be immut
        _4 = std::option::Option<i32>::Some(const 5i32,); // bb0[4]: scope 0 at test/x.rs:2:17: 2:27
        _3 = &'22s _4;                   // bb0[6]: scope 0 at test/x.rs:2:16: 2:27
        _2 = &'5s mut _3;                // bb0[8]: scope 0 at test/x.rs:2:11: 2:27
 ```
nikomatsakis (Aug 29 2018 at 19:00, on Zulip):

note the // <-- comments

mikhail-m1 (Aug 29 2018 at 19:00, on Zulip):

thanks

mikhail-m1 (Aug 29 2018 at 19:02, on Zulip):

i will try to do this

nikomatsakis (Aug 29 2018 at 19:02, on Zulip):

I think we may have to extend as_place

nikomatsakis (Aug 29 2018 at 19:02, on Zulip):

or add an optional variant

nikomatsakis (Aug 29 2018 at 19:02, on Zulip):

that includes the mutability of temporary needed

nikomatsakis (Aug 29 2018 at 19:02, on Zulip):

though we can hack it in another way perhaps

pnkfelix (Aug 29 2018 at 21:56, on Zulip):

hmm I did not realize that @mikhail-m1 was still working on this

mikhail-m1 (Sep 02 2018 at 19:53, on Zulip):

@nikomatsakis I created a review, no it's 18s vs 68s on nightly. https://github.com/rust-lang/rust/pull/53909

nikomatsakis (Sep 04 2018 at 17:24, on Zulip):

nice! Let's do a perf run, I guess

nikomatsakis (Sep 04 2018 at 17:25, on Zulip):

maybe I'll do a local build too, I'm curious to profile that result

lqd (Sep 04 2018 at 17:32, on Zulip):

btw https://github.com/rust-lang/rust/pull/53942 results are -60% on ucd

simulacrum (Sep 04 2018 at 18:34, on Zulip):

Still ~400% afterwards, but better than the current >100%.

nikomatsakis (Sep 04 2018 at 19:00, on Zulip):

yeah, I saw. nice!

lqd (Sep 04 2018 at 20:56, on Zulip):

the try build is ready for rust-timerification https://github.com/rust-lang/rust/pull/53909#issuecomment-418501219

mikhail-m1 (Sep 05 2018 at 13:49, on Zulip):

hi @nikomatsakis , what should I do with this comment?

nikomatsakis (Sep 05 2018 at 13:51, on Zulip):

@mikhail-m1 I think we can easily tell if a local variable has any moves by using the MoveData

nikomatsakis (Sep 05 2018 at 13:51, on Zulip):

which I suppose you are familiar-ish with :)

nikomatsakis (Sep 05 2018 at 13:52, on Zulip):

in particular if we iterate over all child-move-paths of the variable

mikhail-m1 (Sep 05 2018 at 13:52, on Zulip):

ok, so if it has has any move we cannot ignore it, right?

nikomatsakis (Sep 05 2018 at 13:52, on Zulip):

and check whether the path map is empty

nikomatsakis (Sep 05 2018 at 13:52, on Zulip):

yeah

nikomatsakis (Sep 05 2018 at 13:53, on Zulip):

basically similar to the storage-dead thing

nikomatsakis (Sep 05 2018 at 13:54, on Zulip):

probably we want to rename the "has storage dead" actually

nikomatsakis (Sep 05 2018 at 13:54, on Zulip):

to "has storage dead or moves" or something like that

mikhail-m1 (Sep 05 2018 at 13:56, on Zulip):

ok, and I'm going to make a change suggested by @pnkfelix https://github.com/rust-lang/rust/pull/53909#discussion_r214856869

nikomatsakis (Sep 05 2018 at 13:58, on Zulip):

sounds good

mikhail-m1 (Sep 05 2018 at 21:42, on Zulip):

@nikomatsakis just want to check I added move out next way

            for move_out in &move_data.moves {
                if let Place::Local(index) = move_data.move_paths[move_out.path].place {
                    has_storage_dead_or_moved.insert(index);
                }
            }

but you mentioned path_map

nikomatsakis (Sep 05 2018 at 21:43, on Zulip):

oh, that's .. smarter than what I suggested :)

nikomatsakis (Sep 05 2018 at 21:43, on Zulip):

well wait

nikomatsakis (Sep 05 2018 at 21:43, on Zulip):

it's a good overall approach but I think the if let is wrong

nikomatsakis (Sep 05 2018 at 21:43, on Zulip):

in particular, if you have a move from something like a.b.c

nikomatsakis (Sep 05 2018 at 21:43, on Zulip):

we still want to disqualify a

nikomatsakis (Sep 05 2018 at 21:44, on Zulip):

so we could do something like walk up the MovePathIndex parent links

nikomatsakis (Sep 05 2018 at 21:44, on Zulip):

until we find the root MPI

nikomatsakis (Sep 05 2018 at 21:44, on Zulip):

which will be a local

nikomatsakis (Sep 05 2018 at 21:44, on Zulip):

and/or we could do that on the place too

mikhail-m1 (Sep 05 2018 at 21:44, on Zulip):

yes, i just want to check for loop and get path code

nikomatsakis (Sep 05 2018 at 21:44, on Zulip):

there is .. probably .. an existing method

mikhail-m1 (Sep 06 2018 at 18:52, on Zulip):

I updated the PR yesterday

nikomatsakis (Sep 06 2018 at 18:54, on Zulip):

oh, nice

lqd (Sep 06 2018 at 18:56, on Zulip):

remember that whoever r+ will owe njn $5

nikomatsakis (Sep 06 2018 at 18:57, on Zulip):

@mikhail-m1 btw, in the older version, did all tests pass?

nikomatsakis (Sep 06 2018 at 18:57, on Zulip):

if so, maybe I need to make a test that would've failed...

nikomatsakis (Sep 06 2018 at 18:57, on Zulip):

(ah, I recall thinking that would be potentially hard to do actually due to the limitations on constants)

nikomatsakis (Sep 06 2018 at 19:04, on Zulip):

@mikhail-m1 I left a few nits

mikhail-m1 (Sep 06 2018 at 21:41, on Zulip):

@nikomatsakis updated once more and rustfmt librustc_mir/build/expr

Last update: Nov 21 2019 at 13:05UTC