Stream: t-compiler/wg-mir-opt

Topic: collector collects optimized out promoteds


oli (Jun 06 2019 at 06:42, on Zulip):

I haven't checked in detail, but reviewing @Santiago Pastorino 's PR https://github.com/rust-lang/rust/pull/61554/commits/e786f631b815d171051279e0d6cfe055c75bec2e made me realize that we are collecting all promoteds of a mir::Body, even if they are completely unused (e.g. due to const prop optimizing *&1 to 1). I'm not sure how to test this, I think I've seen some sort of tests that check for items ending up in the final binary and we may already have promoted tests there. So we should make such a test with mir optimizations enabled, see whether dead promoteds end up in there and then fix it

oli (Jun 06 2019 at 06:42, on Zulip):

Most likely the fix is to implement PlaceBase::Static(box Static{ kind:StaticKind::Promoted... where we currently have StaticKind::Static in collector.rs

oli (Jul 23 2019 at 14:25, on Zulip):

@Saleem Jaffer this could be an immediate gain, because llvm would need to do less work

Saleem Jaffer (Jul 23 2019 at 15:21, on Zulip):

I'll pick this up.

Saleem Jaffer (Jul 29 2019 at 11:27, on Zulip):

https://github.com/saleemjaffer/rust/blob/745c76d657ebbe29265100e5381f8bf326c54567/src/librustc_mir/monomorphize/collector.rs#L664 is the only place in collector.rs where StaticKind::Static exists. How do I proceed with this?

Saleem Jaffer (Jul 29 2019 at 11:28, on Zulip):

Also @oli could you explain this a bit

made me realize that we are collecting all promoteds of a mir::Body, even if they are completely unused (e.g. due to const prop optimizing *&1 to 1)

If you can explain/guide me to some links about const prop that would be helpful.

Wesley Wiser (Jul 29 2019 at 12:49, on Zulip):

@Saleem Jaffer const prop is here https://github.com/rust-lang/rust/blob/8b94e9e9188b65df38a5f1ae723617dc2dfb3155/src/librustc_mir/transform/const_prop.rs

Wesley Wiser (Jul 29 2019 at 12:52, on Zulip):

I believe what @oli is saying is that if you have something like this:

let x = 1;
let y = 2;

let z = x + y;

Const promotion will do this:

let x = _promoted[0];
let y = _promoted[1];

let z = x + y;

but then const prop will notice that you're adding constants and do that for you creating another promoted:

let x = _promoted[0];
let y = _promoted[1];

let z = _promoted[2]; //3

but now we're not really using _promoted[0] or _promoted[1] anymore

Wesley Wiser (Jul 29 2019 at 12:52, on Zulip):

So we don't really need to spend the time & cpu cycles to collect them

Wesley Wiser (Jul 29 2019 at 12:52, on Zulip):

(Technically this is a bad example because assigning the value to a variable is considered a use, but I think you get the general idea)

Saleem Jaffer (Jul 29 2019 at 13:37, on Zulip):

@Wesley Wiser So does this mean that there would be no promoted[0] and promoted[1], but a single promoted for the result of 1 + 2, i.e. 3 wrt the example you mentioned?

Wesley Wiser (Jul 29 2019 at 14:09, on Zulip):

@Saleem Jaffer Yeah

Wesley Wiser (Jul 29 2019 at 14:10, on Zulip):

Currently though, we don't do anything to avoid collecting unused promoteds so for values that are no longer needed after optimization, we still collect them.

Wesley Wiser (Jul 29 2019 at 14:10, on Zulip):

At least, that's my understanding

Saleem Jaffer (Jul 29 2019 at 15:10, on Zulip):

Thanks for explaining :)

oli (Jul 29 2019 at 17:06, on Zulip):

It's even simpler than that. *&1 will create a promoted for the 1, const prop turns the entire expression into a constant 1 and we still collect the promoted

oli (Jul 29 2019 at 17:07, on Zulip):

Variables are a bit messy, as @Wesley Wiser noted

oli (Jul 29 2019 at 17:09, on Zulip):

So as a repro, having a library with just pub fn foo() -> i32 { *&1 } should not cause any promoted to be collected, but right now it does

Saleem Jaffer (Jul 29 2019 at 17:13, on Zulip):

@oli By “collection” do you mean use the promoted for the LLVM representation?

Saleem Jaffer (Jul 29 2019 at 17:14, on Zulip):

And also, I checked in collector.rs. There is only usage site. So how do I proceed with the issue?

Saleem Jaffer (Jul 29 2019 at 17:33, on Zulip):

Also, in the small example that you mentioned, will there be 2 collections? One for the promoted and the other for the constant?

oli (Jul 29 2019 at 20:01, on Zulip):

Yes, the constant one is fine

oli (Jul 29 2019 at 20:01, on Zulip):

Collector.rs accesses the promoted vector

oli (Jul 29 2019 at 20:02, on Zulip):

That use site should be removed

oli (Jul 29 2019 at 20:02, on Zulip):

And a new one added in visit static

oli (Jul 29 2019 at 20:03, on Zulip):

Or visit place? I don't remember

oli (Jul 29 2019 at 20:03, on Zulip):

Wherever we match on StaticKind

oli (Jul 29 2019 at 20:04, on Zulip):

Do an exhaustive match

Saleem Jaffer (Jul 30 2019 at 04:45, on Zulip):

This is the code in collector.rs

fn visit_place_base(&mut self,
                        place_base: &mir::PlaceBase<'tcx>,
                        _context: mir::visit::PlaceContext,
                        location: Location) {
        match place_base {
            PlaceBase::Static(box Static { kind: StaticKind::Static(def_id), .. }) => {
                debug!("visiting static {:?} @ {:?}", def_id, location);

                let tcx = self.tcx;
                let instance = Instance::mono(tcx, *def_id);
                if should_monomorphize_locally(tcx, &instance) {
                    self.output.push(MonoItem::Static(*def_id));
                }
            }
            PlaceBase::Static(box Static { kind: StaticKind::Promoted(_), .. }) => {
                // FIXME: should we handle promoteds here instead of eagerly in collect_neighbours?
            }
            PlaceBase::Local(_) => {
                // Locals have no relevance for collector
            }
        }
    }

There already is an exhaustive match on StaticKind here. Should I handle the FIXME part?

oli (Jul 30 2019 at 06:30, on Zulip):

yes :laughing: Great that there's a FIXME there

Saleem Jaffer (Jul 30 2019 at 10:49, on Zulip):

@oli So I went over the code, but could not find a way to figure out whether a promoted has been converted to a constant.
Any tips how do I go ahead?

Saleem Jaffer (Jul 31 2019 at 12:36, on Zulip):

@oli
From what I understand we need to do something like this (This code does not compile):

fn visit_place_base(&mut self,
                        place_base: &mir::PlaceBase<'tcx>,
                        _context: mir::visit::PlaceContext,
                        location: Location) {
        match place_base {
            PlaceBase::Static(box Static { kind: StaticKind::Static(def_id), .. }) => {
                debug!("visiting static {:?} @ {:?}", def_id, location);

                let tcx = self.tcx;
                let instance = Instance::mono(tcx, *def_id);
                if should_monomorphize_locally(tcx, &instance) {
                    self.output.push(MonoItem::Static(*def_id));
                }
            }
            PlaceBase::Static(box Static { kind: StaticKind::Promoted(_), .. }) => {
                match tcx.const_eval(param_env.and(cid)) {
                    Ok(val) => collect_const(tcx, val, instance.substs, output),
                    Err(_) => {},
                }
            }
            PlaceBase::Local(_) => {
                // Locals have no relevance for collector
            }
        }
    }
Saleem Jaffer (Jul 31 2019 at 12:37, on Zulip):

Basically, I guess in the promoted arm we need to do a const_eval to check whether const prop turns the promoted to a constant?

oli (Jul 31 2019 at 13:11, on Zulip):

Hmmm curious. You're right, the collector can const eval, because it's monomorphic. What does the current promotion collection code do? You should be able to copy that

oli (Jul 31 2019 at 13:11, on Zulip):

I think we can unwrap the const eval call

oli (Jul 31 2019 at 13:14, on Zulip):

https://github.com/saleemjaffer/rust/blob/745c76d657ebbe29265100e5381f8bf326c54567/src/librustc_mir/monomorphize/collector.rs#L1237

oli (Jul 31 2019 at 13:14, on Zulip):

Do that

oli (Jul 31 2019 at 13:14, on Zulip):

XD

oli (Jul 31 2019 at 13:14, on Zulip):

And then remove that for loop

Saleem Jaffer (Jul 31 2019 at 13:16, on Zulip):

What's "monomorphic"? I've seen this a lot in the mir code.

Wesley Wiser (Jul 31 2019 at 13:44, on Zulip):

Haskell wiki has a rather concise definition https://wiki.haskell.org/Monomorphism

Wesley Wiser (Jul 31 2019 at 13:45, on Zulip):

In Rust, a function is monomorphic if it doesn't have any (remaining) const generic parameters, type parameters, or lifetimes.

Wesley Wiser (Jul 31 2019 at 13:47, on Zulip):

So

fn example<'a, T>() { ... }

is polymorphic

but this instance of the function:

example<'static, u8>();

is monomorphic.

Last update: Nov 17 2019 at 08:20UTC