Stream: t-compiler/wg-mir-opt

Topic: VarDebugInfo using constants instead of locals


eddyb (Feb 07 2020 at 07:58, on Zulip):

@oli @Wesley Wiser hey, do you think it would be cheap to check in the constant folder if a local always has a constant value whenever initialized?

eddyb (Feb 07 2020 at 07:59, on Zulip):

I guess it could be its own pass post-constant-folding, that checks for one initialization and no mutation

eddyb (Feb 07 2020 at 08:01, on Zulip):

I want to have debug x => _123; replaced with e.g. debug x => const 42usize; wherever possible

eddyb (Feb 07 2020 at 08:03, on Zulip):

the other thing we could do is debug a => &b.c; (value of debug var a is always the address of a place with no runtime indexing or derefs)

eddyb (Feb 07 2020 at 08:05, on Zulip):

the latter wouldn't count as a borrow because only the debugger would see it, and also both it and the constant form could use llvm.dbg.value

eddyb (Feb 07 2020 at 08:07, on Zulip):

the goal is to reduce the impact of debuginfo on optimizations and codegen

eddyb (Feb 07 2020 at 08:08, on Zulip):

but maybe I should do some statistics on the kinds of things that are e.g. unnecessarily on the stack in debug mode

Wesley Wiser (Feb 07 2020 at 10:46, on Zulip):

@eddyb By "constant folder" do you mean const-prop or something else?

eddyb (Feb 07 2020 at 21:18, on Zulip):

yeah I guess it's called const-prop

eddyb (Feb 07 2020 at 21:19, on Zulip):

I'd argue propagation is just replacing uses with the value at the definition, whereas folding is e.g. _5 = 1 + 2 => _5 = 3 (there's no propagation there)

eddyb (Feb 07 2020 at 21:19, on Zulip):

the current pass is a combination of propagation and folding

Wesley Wiser (Feb 07 2020 at 21:23, on Zulip):

Sure yeah

Wesley Wiser (Feb 07 2020 at 21:23, on Zulip):

I can't imagine it would be expensive

Wesley Wiser (Feb 07 2020 at 21:24, on Zulip):

We've already done that check and know what locals satisfy that constraint

eddyb (Feb 07 2020 at 21:26, on Zulip):

oh right you're not relying on dataflow?

eddyb (Feb 07 2020 at 21:26, on Zulip):

I guess you couldn't have, @ecstatic-morse's stuff is pretty recent

Wesley Wiser (Feb 07 2020 at 21:30, on Zulip):

Nah, not yet

Wesley Wiser (Jun 03 2020 at 12:38, on Zulip):

@eddyb I took a stab at implementing this. Does this seem about right to you? https://github.com/rust-lang/rust/compare/master...wesleywiser:consts_in_debuginfo

oli (Jun 03 2020 at 14:13, on Zulip):

quick side note just from looking at the tests: use Display formatting for the constant instead of Debug formatting

Wesley Wiser (Jun 03 2020 at 14:14, on Zulip):

Thanks! Will do

Wesley Wiser (Jun 04 2020 at 13:21, on Zulip):

Yeah, that's way better:

-         debug x => _1;                   // in scope 1 at $DIR/scalar_literal_propagation.rs:3:9: 3:10
+         debug x => const 1u32;           // in scope 1 at $DIR/scalar_literal_propagation.rs:3:9: 3:10
eddyb (Jun 04 2020 at 15:12, on Zulip):

I wouldn't change VariableKind

eddyb (Jun 04 2020 at 15:13, on Zulip):

it refers to the LHS not the RHS even if it's computed from the RHS.... it's kind of messed up

eddyb (Jun 04 2020 at 15:13, on Zulip):

changing PerLocalVarDebugInfo also doesn't seem necessary

Wesley Wiser (Jun 04 2020 at 15:14, on Zulip):

Ah yeah I think I only matched against it here so that's not necessary https://github.com/rust-lang/rust/compare/master...wesleywiser:consts_in_debuginfo#diff-59edf51044167945d3d123c17e706bfdR544

eddyb (Jun 04 2020 at 15:15, on Zulip):

so hmm starting from the MIR

eddyb (Jun 04 2020 at 15:15, on Zulip):

I wouldn't split the DebugInfo in VarDebugInfo

eddyb (Jun 04 2020 at 15:16, on Zulip):

so VarDebugInfoValue, although it doesn't feel great either. I wonder what I used in the SROA PR

eddyb (Jun 04 2020 at 15:16, on Zulip):

@Wesley Wiser VarDebugInfoContents

Wesley Wiser (Jun 04 2020 at 15:17, on Zulip):

Is that in your SROA PR?

eddyb (Jun 04 2020 at 15:17, on Zulip):

yeah

Wesley Wiser (Jun 04 2020 at 15:17, on Zulip):

#48300 right?

eddyb (Jun 04 2020 at 15:17, on Zulip):

there it's used for simple leaves vs composite (each field has its own VarDebugInfoContents)

eddyb (Jun 04 2020 at 15:18, on Zulip):

yeah

eddyb (Jun 04 2020 at 15:20, on Zulip):

@Wesley Wiser so, the reason changing PerLocalVarDebugInfo doesn't make sense to me is that it's specifically the information you need to emit debuginfo for initializing a given Local

eddyb (Jun 04 2020 at 15:20, on Zulip):

if it's a constant there's no local involved

Wesley Wiser (Jun 04 2020 at 15:20, on Zulip):

Yeah

Wesley Wiser (Jun 04 2020 at 15:21, on Zulip):

The reason I did that was I needed all the same fields except projection

eddyb (Jun 04 2020 at 15:21, on Zulip):

you don't need to gather them into a Vec tho

Wesley Wiser (Jun 04 2020 at 15:21, on Zulip):

Which is not a great reason to hack it in there lol

eddyb (Jun 04 2020 at 15:22, on Zulip):

you don't need "var name" for constants

eddyb (Jun 04 2020 at 15:23, on Zulip):

since they can't have it anyway (only fn args and instructions can have names)

eddyb (Jun 04 2020 at 15:24, on Zulip):

so you should just get away with spill + dbg_var_addr

eddyb (Jun 04 2020 at 15:25, on Zulip):

oh I see, you're triggering it from debug_introduce_locals

Wesley Wiser (Jun 04 2020 at 15:26, on Zulip):

Yeah

eddyb (Jun 04 2020 at 15:26, on Zulip):

okay so uhhh my advice to keep this simple likely requires renaming compute_per_local_var_debug_info

Wesley Wiser (Jun 04 2020 at 15:27, on Zulip):

I did it that way because the current system seems pretty oriented around locals which we've removed for constants.

eddyb (Jun 04 2020 at 15:27, on Zulip):

and instead of per_local[place.local].push( it can do dbg_var_addr directly

eddyb (Jun 04 2020 at 15:27, on Zulip):

well it's oriented around locals in a very specific way that makes no sense for constants

Wesley Wiser (Jun 04 2020 at 15:28, on Zulip):

Yeah

eddyb (Jun 04 2020 at 15:28, on Zulip):

specifically it's on-demand for locals

eddyb (Jun 04 2020 at 15:28, on Zulip):

or on-assignment rather

Wesley Wiser (Jun 04 2020 at 15:28, on Zulip):

Ok, I think I'm following that.

eddyb (Jun 04 2020 at 15:28, on Zulip):

no need to do that kind of deferral for constants

Wesley Wiser (Jun 04 2020 at 15:28, on Zulip):

And yeah if we do it that way I don't need to collect to a vec or modify PerLocalDebugInfo

eddyb (Jun 04 2020 at 15:29, on Zulip):

mhmm

eddyb (Jun 04 2020 at 15:29, on Zulip):

thanks for pointing me to this btw

Wesley Wiser (Jun 04 2020 at 15:29, on Zulip):

Thanks, that's really helpful!

eddyb (Jun 04 2020 at 15:29, on Zulip):

as you may have noticed I've been kind of out of it

eddyb (Jun 04 2020 at 15:29, on Zulip):

where "it" is everything

Wesley Wiser (Jun 04 2020 at 15:29, on Zulip):

np!

Wesley Wiser (Jun 04 2020 at 15:29, on Zulip):

I knew you'd probably be interested :slight_smile:

Wesley Wiser (Jun 04 2020 at 15:30, on Zulip):

Re the SROA changes. Do you still think I should try to incorporate those into this?

eddyb (Jun 04 2020 at 15:31, on Zulip):

just the name :P

eddyb (Jun 04 2020 at 15:32, on Zulip):

i.e. value -> contents

eddyb (Jun 04 2020 at 15:32, on Zulip):

only if you think it makes sense

Wesley Wiser (Jun 04 2020 at 15:32, on Zulip):

Ah ok

eddyb (Jun 04 2020 at 15:32, on Zulip):

I forget exactly what my line of reasoning was

Wesley Wiser (Jun 04 2020 at 15:32, on Zulip):

That's totally fine

Wesley Wiser (Jun 04 2020 at 15:32, on Zulip):

I have no strong opinions here lol

Wesley Wiser (Jun 04 2020 at 15:34, on Zulip):

Thanks for taking a look at this! I really appreciate it

Last update: Jul 02 2020 at 19:50UTC