Stream: t-compiler/wg-mir-opt

Topic: const prop into operands

oli (Jul 19 2020 at 09:31, on Zulip):

Basically we have this very specific optimization for const propping into the operands in function arguments: https://github.com/rust-lang/rust/blob/0701419e96d94e5493c7ebfcecb66511ab0aa778/src/librustc_mir/transform/const_prop.rs#L1102

oli (Jul 19 2020 at 09:32, on Zulip):

I think we can remove most of that and add a visit_operand method to the visitor that does the Operand::Copy or Operand::Move to Operand::Const conversion if the local is a known constant

lcnr (Jul 19 2020 at 09:45, on Zulip):

Thanks :thumbs_up:

Xavier Denis (Jul 19 2020 at 10:10, on Zulip):

Awesome! I was going to start this after doing Boolean identities but the more the merrier :)

lcnr (Jul 19 2020 at 10:17, on Zulip):

The following code would appear to be incomplete, because
the function Operand::place() returns None if the
Operand is of the variant Operand::Constant.

lcnr (Jul 19 2020 at 10:18, on Zulip):

what should be done for Operand::Constant here, afaict we go from Operand::Copy|Move to Operand::Constant

lcnr (Jul 19 2020 at 10:18, on Zulip):

so I don't see what we would do for constants if they appeared here

oli (Jul 19 2020 at 10:19, on Zulip):

don't do anything for Operand::Const

oli (Jul 19 2020 at 10:19, on Zulip):

it's already constant ^^ nothing to propagate here

lcnr (Jul 19 2020 at 10:19, on Zulip):

yeah, but I don't understand that comment rn

lcnr (Jul 19 2020 at 10:19, on Zulip):

https://github.com/rust-lang/rust/blob/0701419e96d94e5493c7ebfcecb66511ab0aa778/src/librustc_mir/transform/const_prop.rs#L1108-L1111

oli (Jul 19 2020 at 10:20, on Zulip):

oh there's a comment? :grinning:

lcnr (Jul 19 2020 at 10:20, on Zulip):

I want to replace this with for opr in args { self.visit_operand(opr, location) }

oli (Jul 19 2020 at 10:20, on Zulip):

oooh

oli (Jul 19 2020 at 10:22, on Zulip):

I think we should just have walk_terminator directly at the beginning of visit_terminator

oli (Jul 19 2020 at 10:22, on Zulip):

then you don't need any replacement code for the thing you linked

lcnr (Jul 19 2020 at 10:22, on Zulip):

don't we already use super_terminator here?

lcnr (Jul 19 2020 at 10:23, on Zulip):

https://github.com/rust-lang/rust/blob/0701419e96d94e5493c7ebfcecb66511ab0aa778/src/librustc_mir/transform/const_prop.rs#L999

lcnr (Jul 19 2020 at 10:26, on Zulip):

do all mir-opt-tests run with mir-opt-level =2 by default?

lcnr (Jul 19 2020 at 10:33, on Zulip):

this felt slightly too easy so I might have missed something here: #74507

oli (Jul 19 2020 at 10:38, on Zulip):

O_o

oli (Jul 19 2020 at 10:39, on Zulip):

I think you can remove the TerminatorKind::SwitchInt arm, too

oli (Jul 19 2020 at 10:39, on Zulip):

and the else arm in Assert

lcnr (Jul 19 2020 at 10:45, on Zulip):

:thumbs_up:

Jake Goulding (Jul 19 2020 at 14:24, on Zulip):

As a casual observer, I’d love for these things to be documented as expected to improve compile performance (eg reduced LLVM IR) improve runtime performance (eg reduced code in the final executable) or other.

But as a casual observer, I don’t want to make any demands 😇

lcnr (Jul 19 2020 at 18:29, on Zulip):

it doesn't look like this improves the compile time https://github.com/rust-lang/rust/pull/74507#issuecomment-660685419

@oli do we want to restrict this to mir-opt-level = 2?

Félix Fischer (Jul 19 2020 at 18:33, on Zulip):

I think we do

Félix Fischer (Jul 19 2020 at 18:34, on Zulip):

This might be related to CGU partitioning, which has been a problem in the past (see: the propagation step into function arguments)

Félix Fischer (Jul 19 2020 at 18:34, on Zulip):

@Wesley Wiser do you think that's the case here as well?

Félix Fischer (Jul 19 2020 at 18:35, on Zulip):

That this regresses because of the CGU partitioning scheme?

Wesley Wiser (Jul 19 2020 at 18:40, on Zulip):

I think per https://github.com/rust-lang/compiler-team/issues/319 it needs to go under mir-opt-level=3

Félix Fischer (Jul 19 2020 at 18:51, on Zulip):

Oh, true!

Félix Fischer (Jul 19 2020 at 18:51, on Zulip):

Xavier Denis (Jul 19 2020 at 19:37, on Zulip):

would it be worth investigating the regressions further?

Xavier Denis (Jul 19 2020 at 19:38, on Zulip):

because doing this opt would unlock a lot of further opportunities for optimization

Xavier Denis (Jul 19 2020 at 19:46, on Zulip):

looking at the worst test case it seems to spend 15s more in LLVM_lto_optimize, 5 more in LLVM_thin_lto_import and 8 more in LLVM_module_codegen_emit_obj, could it be that this transformation is too aggressive and blowing up the size of code that needs to be generated?

lqd (Jul 19 2020 at 20:17, on Zulip):

there's been a design meeting on this CGU partitioning issue already, see this topic (I don't remember what the plan is, maybe the incremental compilation wg will look at it)

Félix Fischer (Jul 19 2020 at 21:04, on Zulip):

@Xavier Denis regarding the code generation: we seem to currently be at a local optimum, i.e. the code generation unit partitioning scheme for incremental compilation seems to be tuned in favor of how the compiler behaves today, and its performance gets easily broken by changes in how we generate code (e.g. with better MIR opts)

Félix Fischer (Jul 19 2020 at 21:05, on Zulip):

@Wesley Wiser was looking into it. If I remember correctly, they did a refactor of the current scheme to prepare it for future changes.

Wesley Wiser (Jul 19 2020 at 21:06, on Zulip):

Yeah, I think this is something the wg-incr-comp group wants to work on

Wesley Wiser (Jul 19 2020 at 21:06, on Zulip):

But, of course, anyone is welcome to dig in and look for solutions :slight_smile:

Félix Fischer (Jul 19 2020 at 21:06, on Zulip):

They were also experimenting with generating a lot more CGUs, so that incremental compilation didn't have to re-do as much work as it is doing today for trivial changes like adding a println!

Félix Fischer (Jul 19 2020 at 21:07, on Zulip):

Because big CGUs help for runtime performance, which we don't really care about in debug incremental compilation

Félix Fischer (Jul 19 2020 at 21:08, on Zulip):

I would bet that at least half of the current "implemented but disabled" mir-opts regress just from the CGU partitioning scheme alone x3

Xavier Denis (Jul 19 2020 at 21:33, on Zulip):

is there any way to debug CGU partitioning? ie: is it possible to see how the units are determined before & after a change?

Félix Fischer (Jul 19 2020 at 22:10, on Zulip):

No idea. That sounds like it could be super useful to debug the quality of the scheme, tho!

Félix Fischer (Jul 19 2020 at 22:11, on Zulip):

@Wesley Wiser please tell me if I'm pinging you too much, but I think you're like the only one in the team who's remotely familiar with the CGU partitioning step?

Wesley Wiser (Jul 20 2020 at 00:30, on Zulip):

It's fine :)

Wesley Wiser (Jul 20 2020 at 00:31, on Zulip):

I've been using RUSTC_LOG=rustc_mir::monomorphize::partitioning {rustc or cargo invocation here}

Wesley Wiser (Jul 20 2020 at 00:31, on Zulip):

I believe there's also a -Z flag which dumps some data on this

lqd (Jul 20 2020 at 09:20, on Zulip):

yes -Z print-mono-items=lazy(or say, "eager") :)

lqd (Jul 20 2020 at 09:21, on Zulip):

(which I'm slowly working on, to try and make slightly more helpful for different use cases, like at least 1. seeing the gains of polymorphization AKA "david's use case", 2. seeing the advantages/drawbacks of our cgu partitioning scheme and process and diff that AKA "wesley/wg-incr's", 3. having more visibility into the amount/cost of monomorphizations and the likes AKA "niko/matklad's/everyone with long compile times")

Xavier Denis (Jul 20 2020 at 12:27, on Zulip):

it'd be awesome to print the size estimates of different items so that we could compare the before and after of a change like this and see what actually changed

lcnr (Jul 21 2020 at 21:51, on Zulip):

restricted this to mir-opt-level=3. As we now have the same locations for all operand propagations, this also disables it for SwitchInt and Assert, which isn't great. Not sure how to deal with that without just closing this PR :sweat_smile:

lcnr (Jul 22 2020 at 07:49, on Zulip):

breaks an incremental test

oli (Jul 22 2020 at 07:49, on Zulip):

yea, see also my comment on the PR :wink:

oli (Jul 22 2020 at 07:50, on Zulip):

let's not regress things while blocking only the advanced things under mir-opt-level 3

lcnr (Jul 22 2020 at 07:50, on Zulip):

i still want to know why this happens though, we probably remove the changed span due to the opt

lcnr (Jul 22 2020 at 07:50, on Zulip):

which seems interesting

oli (Jul 22 2020 at 07:51, on Zulip):

oh, you mean "breaks a test" as in turning off propagation makes it mark less stuff as dirty?

lcnr (Jul 22 2020 at 07:51, on Zulip):

more stuff is dirty

lcnr (Jul 22 2020 at 07:51, on Zulip):

if the optimization is not run at all

oli (Jul 22 2020 at 07:52, on Zulip):

right, that makes sense

lcnr (Jul 22 2020 at 07:52, on Zulip):

how can I emit the final mir in a test?

lcnr (Jul 22 2020 at 07:53, on Zulip):

I want to see what exactly changes here

oli (Jul 22 2020 at 07:53, on Zulip):

if we optimize away an if false in both cases, the contents of the branch don't matter

oli (Jul 22 2020 at 07:53, on Zulip):

uh.... something something -Z emir=mir?

lcnr (Jul 22 2020 at 07:53, on Zulip):

using// EMIT_MIR rustc.try_identity.SimplifyLocals.after.mir

oli (Jul 22 2020 at 07:53, on Zulip):

I alway use -Z dump-mir=all and be done with it XD

Dániel Buga (Sep 23 2020 at 13:42, on Zulip):

Has anyone checked if the compile time regression went away with LLVM 11?

oli (Sep 23 2020 at 13:53, on Zulip):

I don't think so. Feel free to open a PR trying out setting the mir-opt-level to 1 and ping me, I'll start a perf run

Dániel Buga (Sep 23 2020 at 13:59, on Zulip):

Thanks! Hope I did it correctly

Dániel Buga (Sep 23 2020 at 16:43, on Zulip):

The results are in, and at least there's no 24% regression anywhere, but I don't know if it counts as good enough or not.

Last update: Jan 22 2021 at 13:00UTC