Stream: t-compiler/wg-nll

Topic: issue-51641-n-squared-lint-loop


nikomatsakis (Jun 19 2018 at 20:31, on Zulip):

@lqd I wrote up some mentoring instructions on issue #51641

nikomatsakis (Jun 19 2018 at 20:31, on Zulip):

let me know if they make sense :)

lqd (Jun 19 2018 at 20:32, on Zulip):

oh awesome thank you, I will surely need them :)

lqd (Jun 19 2018 at 20:37, on Zulip):

@nikomatsakis yeah makes sense :)

lqd (Jun 20 2018 at 13:45, on Zulip):

@nikomatsakis hello :) couple note/questions:
- I was going to 1st remove the quadratic loop like you suggested in the instructions, before looking at gathering information on what it fixes, and how to do that in another existing walk or else, is that ok ?
- do I need to do anything with these TODOs in our new visitor ? copy these comments ?
- I assume I should put the visitor in borrow_check/mod.rs ?
- I planned on adding a method to the MirBorrowckContext to find the used muts, however it's divided into 3 impls in this file, should I just put my method in the 1st one ? :)
- my planned visitor looks like the following, does this match what you were expecting ?

struct FindUsedMutsVisitor<'visit, 'cx: 'visit, 'gcx: 'tcx, 'tcx: 'cx> {
    needles: FxHashSet<Local>,
    mbcx: &'visit mut MirBorrowckCtxt<'cx, 'gcx, 'tcx>,
}
nikomatsakis (Jun 20 2018 at 13:57, on Zulip):

first, I would put the new code in a submodule :)

nikomatsakis (Jun 20 2018 at 13:57, on Zulip):

before looking at gathering information on what it fixes, and how to do that in another existing walk or else, is that ok ?

yes, definitely

nikomatsakis (Jun 20 2018 at 13:58, on Zulip):

meh, just delete. Not sure why that's even there.

nikomatsakis (Jun 20 2018 at 13:59, on Zulip):

I'm not sure I understand.. I think the answer though is:

sort of:

// new_mod.rs
impl MirBorrowckCtxt<..> {
    crate fn gather_stuff() {
        let Visitor = Visitor { ... }
    }
}

struct Visitor { ... }

impl MirVisitor for Visitor { ... }
nikomatsakis (Jun 20 2018 at 13:59, on Zulip):

that visitor looks about right yes

lqd (Jun 20 2018 at 14:01, on Zulip):

what I mean with the 3 impls is this: 1) https://github.com/rust-lang/rust/blob/master/src/librustc_mir/borrow_check/mod.rs#L793 2) https://github.com/rust-lang/rust/blob/master/src/librustc_mir/borrow_check/mod.rs#L1394 and 3) https://github.com/rust-lang/rust/blob/master/src/librustc_mir/borrow_check/mod.rs#L2139

lqd (Jun 20 2018 at 14:05, on Zulip):

so I was wondering why MirBorrowckCtxt was split in 3, but I guess it's for readability

nikomatsakis (Jun 20 2018 at 14:48, on Zulip):

I personally think borrow_check/mod.rs has grown way beyond the size it should have :)

nikomatsakis (Jun 20 2018 at 14:48, on Zulip):

but often we do divide up methods by "theme"

nikomatsakis (Jun 20 2018 at 14:48, on Zulip):

I tend to prefer do that in distinct modules

nikomatsakis (Jun 20 2018 at 14:48, on Zulip):

not within one file :)

nikomatsakis (Jun 20 2018 at 14:48, on Zulip):

my guess is that those 3 impls arose...organically, let's say

nikomatsakis (Jun 20 2018 at 14:48, on Zulip):

I think this code is somewhat in need of reorg

nikomatsakis (Jun 20 2018 at 14:48, on Zulip):

it's been evolving a lot and with a lot of distinct people doing PRs etc =)

lqd (Jun 20 2018 at 14:49, on Zulip):

:)

nikomatsakis (Jun 20 2018 at 14:49, on Zulip):

that said, no reason to add to it

nikomatsakis (Jun 20 2018 at 14:49, on Zulip):

hence my suggestion to isolate your changes to a distinct module

nikomatsakis (Jun 20 2018 at 14:49, on Zulip):

they seem like a coherent unit to me...

lqd (Jun 20 2018 at 14:49, on Zulip):

would used_muts.rs be a good name ? so that maybe in the future we can move the rest of the used mut lint there ?

nikomatsakis (Jun 20 2018 at 14:49, on Zulip):

+1

nikomatsakis (Jun 20 2018 at 14:49, on Zulip):

yeah, something like that

nikomatsakis (Jun 20 2018 at 14:50, on Zulip):

I think in my ideal world there'd be a used_mut submodule with (probably) multiple further submodules

nikomatsakis (Jun 20 2018 at 14:50, on Zulip):

since I suspect there's enough complex logic that it merits it

nikomatsakis (Jun 20 2018 at 14:50, on Zulip):

but anyway

lqd (Jun 20 2018 at 14:51, on Zulip):

yeah rn the module is 60 lines or so -- I think I'm almost done

lqd (Jun 20 2018 at 16:16, on Zulip):

@nikomatsakis I have to go now but here is the PR https://github.com/rust-lang/rust/pull/51660

lqd (Jun 20 2018 at 17:03, on Zulip):

@simulacrum I was wondering if we needed a perf run, so thanks for the try build :)

simulacrum (Jun 20 2018 at 17:04, on Zulip):

Don't need it but can't hurt.

nikomatsakis (Jun 20 2018 at 18:05, on Zulip):

@lqd I left one minor nit but looks good

lqd (Jun 20 2018 at 18:16, on Zulip):

oh the match on the PlaceContext ok

lqd (Jun 20 2018 at 18:20, on Zulip):

maybe there’s already a similar fn somewhere checking if a Place is an assignment ?

nikomatsakis (Jun 20 2018 at 18:27, on Zulip):

I feel like .. sort of maybe? check the liveness code perhaps?

nikomatsakis (Jun 20 2018 at 18:28, on Zulip):

@lqd I was thinking of this fn

lqd (Jun 20 2018 at 18:39, on Zulip):

thanks, I will check it out once I’m back to the computer

lqd (Jun 20 2018 at 21:34, on Zulip):

if I extracted matching PlaceContext::Call and PlaceContext::Store into a separate fn used by collect_writes, used_muts, and liveness:categorize, I wouldn't know where to keep this comment which seems important if in the future the success and unwind paths are handled differently ?

lqd (Jun 20 2018 at 21:55, on Zulip):

I can at the very least share the whole thing with find_assignments easily

nikomatsakis (Jun 20 2018 at 21:56, on Zulip):

maybe just do that for now... though I think you could just call the def/use categorizer as is

nikomatsakis (Jun 20 2018 at 21:56, on Zulip):

and look for Defs

nikomatsakis (Jun 20 2018 at 21:56, on Zulip):

and consider that to be an assignment

nikomatsakis (Jun 20 2018 at 21:56, on Zulip):

and leave the comment where it is :)

lqd (Jun 20 2018 at 21:57, on Zulip):

even if collect_writes didn't look for AsmOutput or Storage* ?

lqd (Jun 20 2018 at 21:57, on Zulip):

if so, :thumbs_up:

lqd (Jun 20 2018 at 22:05, on Zulip):

(oh maybe you meant calling the categorizer in used_mut only)

nikomatsakis (Jun 21 2018 at 09:12, on Zulip):

even if collect_writes didn't look for AsmOutput or Storage* ?

hmm, AsmOutput seems ok -- maybe even a bug that we were ignored it -- but the storage stuff is a bit questionable

nikomatsakis (Jun 21 2018 at 09:12, on Zulip):

tbh StorageDead and friends don't seem like a "def" or a "use"

lqd (Jun 21 2018 at 09:17, on Zulip):

I think we don't really need collect_writes really, it might have been made for that loop mostly. Where it's used it only needs the first location

nikomatsakis (Jun 21 2018 at 09:19, on Zulip):

what is collect_writes?

nikomatsakis (Jun 21 2018 at 09:19, on Zulip):

do you mean find_assignments?

lqd (Jun 21 2018 at 09:20, on Zulip):

yeah find_assignments, which is in util/collect_writes.rs

nikomatsakis (Jun 21 2018 at 09:20, on Zulip):

hmm. maybe so. I'm not convinced that ignoring other writes is correct in that code...maybe it is...

nikomatsakis (Jun 21 2018 at 09:21, on Zulip):

but it seems like in general it's a footgun to walk ALL the IR

lqd (Jun 21 2018 at 09:21, on Zulip):

yeah

nikomatsakis (Jun 21 2018 at 09:25, on Zulip):

I'd be ok with changing to find_some_assignment or something like that

lqd (Jun 21 2018 at 09:25, on Zulip):

I'm not sure finding storage* locations in the single use of find_assignment will have no impact so I'm not sure what to do about that. I was in the meantime running the tests on using the categorizer only in the new gather_used_mut

lqd (Jun 21 2018 at 09:27, on Zulip):

@nikomatsakis btw http://perf.rust-lang.org/compare.html?start=637fd2e0487281adac99000602f5e74c3bf151a8&end=737c5cc95040e9efce65cb13a51a0df0f842cef4&stat=instructions%3Au

nikomatsakis (Jun 21 2018 at 09:28, on Zulip):

I think @lqd it's fine to just have two methods for now

nikomatsakis (Jun 21 2018 at 09:28, on Zulip):

that is, we don't have to consolidate with the helper from liveness

nikomatsakis (Jun 21 2018 at 09:28, on Zulip):

it feels like a separate PR

lqd (Jun 21 2018 at 09:28, on Zulip):

ok yeah

nikomatsakis (Jun 21 2018 at 09:28, on Zulip):

also, nice results

lqd (Jun 21 2018 at 09:29, on Zulip):

maybe as a later phase when we can consolidate into another walk

lqd (Jun 21 2018 at 09:29, on Zulip):

I tried removing the loop and it seemed like only this test failed

lqd (Jun 21 2018 at 09:33, on Zulip):

so something like a is_place_assignment in util/collect_writes.rs would be ok ?

nikomatsakis (Jun 21 2018 at 09:45, on Zulip):

sounds good

lqd (Jun 21 2018 at 10:52, on Zulip):

@nikomatsakis I should also make this fn say AsmOutput is an assignment shouldn't I, here it's like a Store to me

lqd (Jun 21 2018 at 12:14, on Zulip):

so I did that and pushed the shared function to https://github.com/rust-lang/rust/pull/51660 :)

lqd (Jun 22 2018 at 10:47, on Zulip):

sweet, the PR landed

nikomatsakis (Jun 22 2018 at 14:10, on Zulip):

nice

Last update: Nov 21 2019 at 14:20UTC