Stream: t-compiler/wg-mir-opt

Topic: const propagation


Wesley Wiser (Apr 25 2019 at 13:10, on Zulip):

Are there any other tasks that could be worked on while @Santiago Pastorino works on the places refactoring? I'd love to get involved :slight_smile:

oli (Apr 25 2019 at 13:14, on Zulip):

One thing we have is a const propagator pass that does not propagate constants XD

oli (Apr 25 2019 at 13:16, on Zulip):

there are a few Place related changes, but they conflict with the current place work, so I'd rather delay that

oli (Apr 25 2019 at 13:17, on Zulip):

We want to have stable identifiers for e.g. basic blocks (https://paper.dropbox.com/doc/Topic-MIR-2.0-and-MIR-Optimizations--Ab7LrtY0ql6SAFF3bvSTlMgWAQ-BwHR7kOhxDwL6vuAUoSTQ) so that removing a block does not invalidate the ids

oli (Apr 25 2019 at 13:18, on Zulip):

according to the all hands notes we should be comparing notes with cranelift

oli (Apr 25 2019 at 13:22, on Zulip):

also we'll want some sort of transaction-like mir passes so we can do e.g. inlining, see if that improved anything, if not: just drop the inlined code and keep the old

oli (Apr 25 2019 at 13:23, on Zulip):

one funky idea is to make MIR independent of rustc, not sure if we can do that in small steps

oli (Apr 25 2019 at 13:25, on Zulip):

but we can start by pulling all types that do not depend on general compiler types from rustc::mir to rustc_target or similar

Wesley Wiser (Apr 25 2019 at 13:25, on Zulip):

One thing we have is a const propagator pass that does not propagate constants XD

This is interesting. Is there an issue with more details?

oli (Apr 25 2019 at 13:27, on Zulip):

lol I just found https://github.com/rust-lang/rust/issues/28238

oli (Apr 25 2019 at 13:27, on Zulip):

I guess we can close that

oli (Apr 25 2019 at 13:28, on Zulip):

hmm.. maybe need to see if my two issues have been fixed

oli (Apr 25 2019 at 13:30, on Zulip):

so.. unfortunately I think we have no additional info, but I think it shouldn't be too hard to make the const propagator replace Assign rhs with the evaluated value if one has been found (instead of just carrying the values in a shadow datastructure and lint about const eval failures)

Wesley Wiser (Apr 25 2019 at 13:34, on Zulip):

Ok. I have a vague idea of what const propagation is supposed to do but I haven't looked at the code yet. I'll do that soon and come back to you if I have any questions.

Wesley Wiser (Apr 28 2019 at 15:31, on Zulip):

Ok. I've read through the const_prop module and I think I've got a handle on how it currently works.

Wesley Wiser (Apr 28 2019 at 15:31, on Zulip):

I see there's a lot of TODO and FIXME comments that will probably have to be addressed soon

Wesley Wiser (Apr 28 2019 at 15:33, on Zulip):

Am I correct that in order to get this to actually const prop instead of just lint, I need to add some code here-ish to mutate rval to be the value we have?

Wesley Wiser (Apr 28 2019 at 15:33, on Zulip):

@oli

oli (Apr 28 2019 at 15:36, on Zulip):

yea, you'd need to turn it into a MutVisitor in order to get mut access and then replace the rval with an Rvalue::Use(Operand::Constant(...))

oli (Apr 28 2019 at 15:36, on Zulip):

note that the replacing should not happen with -O0, not even sure if -O1 should have it

Wesley Wiser (Apr 28 2019 at 15:37, on Zulip):

Because it's slow or some other reason?

Wesley Wiser (Apr 28 2019 at 15:40, on Zulip):

Also, it took a bit of experimentation to get a test case that seemed to actually trigger this codepath in the current const_prop code. Does this look like a reasonable test case?

const A: [i32; 1] = [42];

fn main() {
    let x = A[0];

    std::process::exit(x);
}
Wesley Wiser (Apr 28 2019 at 15:40, on Zulip):

(I added the exit call just to make sure x wouldn't be removed as dead code.

oli (Apr 28 2019 at 18:33, on Zulip):

Slow might be one thing, but that's not really a problem since we're running all of it anyway for diagnostics

oli (Apr 28 2019 at 18:33, on Zulip):

the problem is that it'll do source code folding so debug information disappears

oli (Apr 28 2019 at 18:34, on Zulip):

I don't think we have a good setup for this kind of thing yet

oli (Apr 28 2019 at 18:34, on Zulip):

Shouldn't let x = 1 + 1; already trigger the code you linked?

oli (Apr 28 2019 at 18:35, on Zulip):

I mean let x = 0u8 - 1; causes a diagnostic via const_prop, so something is happening

oli (Apr 28 2019 at 18:35, on Zulip):

and I hope let x = (1u8 - 1) - 1; also triggers a diagnostic

Wesley Wiser (Apr 28 2019 at 20:08, on Zulip):

Oh, perhaps. I thought I tried let x = 1 + 1 but didn't see anything in the log output

Wesley Wiser (Apr 28 2019 at 20:34, on Zulip):

Ah, ok. I think I tried that before I really understood what was going on.

Wesley Wiser (Apr 28 2019 at 20:34, on Zulip):

That works fine.

Wesley Wiser (Apr 28 2019 at 20:34, on Zulip):

Now I just need to figure out how to handle ScalarPair lol

Wesley Wiser (Apr 29 2019 at 02:03, on Zulip):

This seems to have worked https://github.com/rust-lang/rust/commit/f20b16235409465c84a396321b3d1abbd74e0d3a

oli (Apr 29 2019 at 13:25, on Zulip):

@Wesley Wiser if you prefer I can do the const_eval refactoring, this is technical debt built up by me

Wesley Wiser (Apr 29 2019 at 14:37, on Zulip):

@oli No, I don't mind at all unless you really want to do it :)

oli (Apr 29 2019 at 18:10, on Zulip):

Heh, getting more eyes on that code is great! It's mostly been @RalfJ and I throwing PRs at each other

oli (Apr 29 2019 at 18:10, on Zulip):

So, all yours!

Wesley Wiser (Apr 30 2019 at 01:10, on Zulip):

@oli Ok, so I think I followed your instructions correctly. That left one instance of self.mir: https://github.com/rust-lang/rust/blob/00859e3e653973120006aaf3227823062dde1ba7/src/librustc_mir/transform/const_prop.rs#L546

I wasn't sure what to do with that so I tried just removing the need to get the span in the first place. There was really only one line that needed it: https://github.com/rust-lang/rust/blob/00859e3e653973120006aaf3227823062dde1ba7/src/librustc_mir/transform/const_prop.rs#L257

It seemed to compile even with removing that. Here's the full diff of my changes:
https://github.com/rust-lang/rust/compare/master...wesleywiser:const_prop_refactoring?expand=1#diff-9e103702275cbef342c2decd3395bf3b#L546

oli (Apr 30 2019 at 11:03, on Zulip):

@Wesley Wiser Hmm... I believe I added that to improve diagnostic spans, does ./x.py test --stage 1 src/test/ui --bless --test-args const change no stderr files?

Wesley Wiser (Apr 30 2019 at 11:03, on Zulip):

I'm not sure. I'm getting an ICE earlier in the build

Wesley Wiser (Apr 30 2019 at 11:04, on Zulip):
thread 'rustc' panicked at 'index out of bounds: the len is 0 but the index is 0', /Users/wesley/code/rust/rust/src/libcore/slice/mod.rs:2687:10
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
   1: std::sys_common::backtrace::print
   2: std::panicking::default_hook::{{closure}}
   3: std::panicking::default_hook
   4: rustc::util::common::panic_hook
   5: std::panicking::rust_panic_with_hook
   6: std::panicking::continue_panic_fmt
   7: rust_begin_unwind
   8: core::panicking::panic_fmt
   9: core::panicking::panic_bounds_check
  10: rustc::mir::Mir::return_ty
  11: <rustc_mir::transform::const_prop::ConstProp as rustc_mir::transform::MirPass>::run_pass
Wesley Wiser (Apr 30 2019 at 11:06, on Zulip):

Does this part look right to you? https://github.com/wesleywiser/rust/blob/94bd101dc679acdf1af16b3b487503711e0466a2/src/librustc_mir/transform/const_prop.rs#L132-L143

Wesley Wiser (Apr 30 2019 at 11:09, on Zulip):

I assume the issue is that Visitor::super_visit_mir() calls Mir::return_ty()but by this point, mir.local_decls has been replaced with an empty IndexVec and so the return local's index is out of bounds

Wesley Wiser (Apr 30 2019 at 11:10, on Zulip):

Should I just clone mir.local_decls instead?

oli (Apr 30 2019 at 11:27, on Zulip):

for now that seems reasonable. Add a FIXME so we'll clean it up

oli (Apr 30 2019 at 11:28, on Zulip):

copy_prop doesn't care about visit_ty and it seems overly expensive to have all visitors fetch those types when most of them don' t use them

Wesley Wiser (Apr 30 2019 at 23:43, on Zulip):

@oli I pushed a PR with just the const_eval changes (#60428). I also fixed the ICE and got a test run of the ui tests. I'm seeing three changes to test output (see this commit for the test diff).

oli (May 01 2019 at 07:03, on Zulip):

yea, that diff is what I feared

oli (May 01 2019 at 07:12, on Zulip):

what happens if you use c.span instead of source_info.span?

oli (May 01 2019 at 07:12, on Zulip):

is that the right span or is it of the const definition site instead of the use site?

oli (May 01 2019 at 07:13, on Zulip):

otherwise I guess we'll have to change the visitor to pass down the SourceInfo of the Rvalue or Statement

Wesley Wiser (May 01 2019 at 11:51, on Zulip):

@oli Oh, that worked pretty well actually

Wesley Wiser (May 01 2019 at 11:51, on Zulip):

Pushing a diff now...

Wesley Wiser (May 01 2019 at 11:53, on Zulip):

https://github.com/rust-lang/rust/commit/84335207f18542e195fdd8cd412097ca6e7d2496

oli (May 01 2019 at 11:56, on Zulip):

oh, sweet, that's even an improvement

Wesley Wiser (May 02 2019 at 19:55, on Zulip):

@oli Ok, I think after that PR you r+'d merges, the code is ready to begin actually doing constant propagation. You mentioned that we shouldn't always do this because it will mess up debuginfo. What flag(s) should I gate the optimization under? mir-opt? Regular opt?

oli (May 03 2019 at 08:33, on Zulip):

@Wesley Wiser there's some optimization level checking code in the MIR inliner, you can just do the same thing for const prop and not do the copying if the level is not high enough. We should probably start at O3 and can move to lower levels later

Wesley Wiser (May 07 2019 at 11:57, on Zulip):

@oli

create a helper function for obtaining an Operand from a ty::Const (or maybe even from a Scalar + Ty, although in the future we'll probably need the ty::Const, too)

Do you mean add a helper function to OpTy (OpTy is aliased as Const on line 82)?

oli (May 07 2019 at 11:57, on Zulip):

oh

oli (May 07 2019 at 11:57, on Zulip):

right

oli (May 07 2019 at 11:58, on Zulip):

wait

oli (May 07 2019 at 11:58, on Zulip):

no I think I meant ty::Const, but let me check

oli (May 07 2019 at 12:00, on Zulip):

in https://github.com/rust-lang/rust/pull/60597/files#diff-9e103702275cbef342c2decd3395bf3bR506 you need an Operand::Constant to put into the Rvalue::Use and in https://github.com/rust-lang/rust/pull/60597/files#diff-9e103702275cbef342c2decd3395bf3bR527 you need Operand::Constant for the Vec of the tuple elements

oli (May 07 2019 at 12:00, on Zulip):

so the helper should directly produce Operand (which would always be Operand::Const), in order to reduce duplication between the sites going from a Scalar + Ty to Operand

Wesley Wiser (May 07 2019 at 12:02, on Zulip):

Oh, ok. But the helper function goes from rustc_mir::interpret::Scalar to Operand right?

Wesley Wiser (May 07 2019 at 12:02, on Zulip):

I don't have a Const to turn into an Operand

oli (May 07 2019 at 12:12, on Zulip):

heh, right, my comment about Const was about ty::Const, which you'd have to create

oli (May 07 2019 at 12:12, on Zulip):

but that's unnecessary right now

oli (May 07 2019 at 12:12, on Zulip):

might become necessary in the future

oli (May 07 2019 at 12:12, on Zulip):

but then we can split the helper into two

Wesley Wiser (May 07 2019 at 12:13, on Zulip):

Ok, sounds good. I'll push up the changes in a minute

Wesley Wiser (May 08 2019 at 10:00, on Zulip):

@oli Did you still want to do a perf run of #60597?

oli (May 08 2019 at 10:17, on Zulip):

no, you're right, it's useless right now

oli (May 08 2019 at 10:17, on Zulip):

we'll do that when we lower the O level

Wesley Wiser (May 08 2019 at 10:22, on Zulip):

Ok

Wesley Wiser (May 08 2019 at 10:22, on Zulip):

If you want, I can push a temporary commit to remove the o3 check just for the perf run

oli (May 08 2019 at 10:37, on Zulip):

nah, we'll do that in a separate PR

oli (May 08 2019 at 10:37, on Zulip):

I mean... we could experiment with moving this to O1 since most of the work is being done anyway

Wesley Wiser (May 08 2019 at 10:39, on Zulip):

True

Wesley Wiser (May 08 2019 at 10:40, on Zulip):

Let me push a commit just so we can see what the performance impact is and then we can decide whether to put it in O1 or O3

oli (May 08 2019 at 10:40, on Zulip):

sgtm

oli (May 08 2019 at 10:41, on Zulip):

(if you're alright with the PR taking longer to get merged, otherwise we can do a PR with just the move to O1)

Wesley Wiser (May 08 2019 at 10:44, on Zulip):

It's fine with me either way. eddyb still needs to review and if that happens soon, we can do the move to O1 in another PR as you said

oli (May 08 2019 at 10:44, on Zulip):

heh ping @eddyb

Wesley Wiser (May 09 2019 at 15:48, on Zulip):

@oli Do you have any thoughts about what optimization level this should be gated on? The performance results are in this comment.

oli (May 09 2019 at 15:52, on Zulip):

Yea, imo we should go with O1

oli (May 09 2019 at 15:53, on Zulip):

uncontroversial would be O2, so we could start there though

Wesley Wiser (May 09 2019 at 15:53, on Zulip):

O1 is also what I was thinking

Wesley Wiser (May 09 2019 at 15:53, on Zulip):

I'll make that change tonight and fix any test fallout

Wesley Wiser (May 10 2019 at 11:56, on Zulip):

Are the changes to to codegen tests concerning? When -O is enabled, they seem to optimize to the same thing.

Diff between mir-opt-level=0 and mir-opt-level=1

oli (May 10 2019 at 13:11, on Zulip):

oh :confused: That's not good

oli (May 10 2019 at 13:11, on Zulip):

llvm will clean it up, but still

oli (May 10 2019 at 13:11, on Zulip):

I guess our constant -> llvm lowering step isn't very good

oli (May 10 2019 at 13:12, on Zulip):

that may explain why the unoptimized case regressed in the perf run

oli (May 10 2019 at 13:15, on Zulip):

I can kinda see how that would happen... Maybe we should try to read a ScalarPair out of a constant before converting it to llvm

oli (May 10 2019 at 13:17, on Zulip):

Basically if we have TyLayout withAbi::ScalarPair, but ConstValue::ByRef, we'd read a pair from the Allocation. I think I tried to setup some code for that a while back, but never got around to actually ripping it out of InterpCtx and moving it to Allocation

Wesley Wiser (May 10 2019 at 13:27, on Zulip):

I guess the other thing would be to improve the const propagation a bit more. As it is, you end up with code like:

      _2 = (const 2u32, const false);
      assert(!move (_2.1: bool), "attempt to add with overflow") -> bb1;

where !move (_2.1: bool) should just become const false and then simplify branches should take care of it from there

oli (May 10 2019 at 13:29, on Zulip):

yea, there's some code that will emit the attempt to add with overflow diagnostic from assertions know to trigger. That same place could overwrite the _2.1 with the propagated value. I don't know if we should do that in the same PR though

oli (May 10 2019 at 13:29, on Zulip):

the same thing goes for other branches with know-constant branching (which are turned into goto once the constant is promoted into the terminator)

Wesley Wiser (May 10 2019 at 13:32, on Zulip):

Sure, that could be done in another PR. I guess we should gate this under O2 then?

Wesley Wiser (May 10 2019 at 13:32, on Zulip):

I think part of the issue is this https://github.com/rust-lang/rust/blob/0ac53da03dad79655e2f3e65a58f94a2f3314d5f/src/librustc_mir/transform/const_prop.rs#L527

Wesley Wiser (May 10 2019 at 13:33, on Zulip):

Once a Local is determined to be propagatable, shouldn't uses of it also be propagatable?

Wesley Wiser (May 10 2019 at 13:33, on Zulip):

I don't think that currently happens but I might be wrong

oli (May 10 2019 at 13:35, on Zulip):

ah, no that's a different issue

oli (May 10 2019 at 13:36, on Zulip):

we need dataflow in order to know what we can actually promote

oli (May 10 2019 at 13:36, on Zulip):

but at that point we should be merging const prop and copy prop I guess

Wesley Wiser (May 10 2019 at 13:36, on Zulip):

Ah ok

Wesley Wiser (May 10 2019 at 13:37, on Zulip):

I'm surprised move (_2.1: bool) isn't already const propagated

Wesley Wiser (May 10 2019 at 13:37, on Zulip):

Since it's just reading a constant

oli (May 10 2019 at 13:37, on Zulip):

oh it is, but your code only writes propagations to locals

oli (May 10 2019 at 13:37, on Zulip):

this is a propagation into an Operand

Wesley Wiser (May 10 2019 at 13:38, on Zulip):

Oh gotcha

oli (May 10 2019 at 13:38, on Zulip):

right now as the PR stands there is no propagation into terminators

oli (May 10 2019 at 13:38, on Zulip):

just into assignment statements

Wesley Wiser (May 10 2019 at 13:38, on Zulip):

Is that still in scope for ConstProp?

oli (May 10 2019 at 13:38, on Zulip):

yea

oli (May 10 2019 at 13:39, on Zulip):

that's what I meant with "not in this PR"

Wesley Wiser (May 10 2019 at 13:39, on Zulip):

:thumbs_up:

Wesley Wiser (May 10 2019 at 13:39, on Zulip):

Do you want me to switch this back to O2 then?

oli (May 10 2019 at 13:39, on Zulip):

yea, I guess go back to O2 and I'll try to figure out the better llvm codegen story

Wesley Wiser (May 10 2019 at 13:39, on Zulip):

So there's no regressions

Wesley Wiser (May 10 2019 at 13:39, on Zulip):

Alright

Wesley Wiser (May 10 2019 at 13:39, on Zulip):

Thanks!

oli (May 10 2019 at 13:39, on Zulip):

I don't remember what the problems were, maybe they have become obsolete

Wesley Wiser (May 10 2019 at 13:40, on Zulip):

That would be nice

Wesley Wiser (May 12 2019 at 00:30, on Zulip):

@oli ConstProp into terminators helps a lot https://github.com/rust-lang/rust/pull/60745#issuecomment-491530334

oli (May 12 2019 at 09:32, on Zulip):

wow, that's awesome

oli (May 12 2019 at 09:33, on Zulip):

I guess it makes sense, because subsequent passes can eliminate a lot of basic blocks this way

Wesley Wiser (May 13 2019 at 02:48, on Zulip):

It looks like it's improved the codegen quite a bit from those results I posted the other day. The code generated is still different but it may actually be reasonable now.

Wesley Wiser (May 13 2019 at 02:51, on Zulip):

Latest results: https://gist.github.com/wesleywiser/548f58348bd2d24fcf7e5aa1603f53c6/revisions

Wesley Wiser (May 13 2019 at 02:53, on Zulip):

There's still some dead variables in there but removing the panic checks seem like a win to me. I don't know a lot about LLVM though.

oli (May 13 2019 at 08:18, on Zulip):

wait this is for release mode!?

Wesley Wiser (May 13 2019 at 10:47, on Zulip):

No, this is -Copt-level=0

Wesley Wiser (May 13 2019 at 10:51, on Zulip):

With -Copt-level=3, there's no change in generated IR. LLVM optimizes everything to

; Function Attrs: norecurse nounwind readnone uwtable
define i32 @nothing() unnamed_addr #0 {
start:
  ret i32 4
}
oli (May 13 2019 at 11:09, on Zulip):

ah ok, that makes sense

simulacrum (May 14 2019 at 00:53, on Zulip):

still probably makes LLVM faster at doing it's thing

Wesley Wiser (May 14 2019 at 10:34, on Zulip):

Hey @eddyb, what did you mean with this feedback? https://github.com/rust-lang/rust/pull/60597/files/ba7b8312929f39b890f437063f2eec27984fdd19#r283381539

eddyb (May 14 2019 at 10:35, on Zulip):

what I mean is that you should be able to take value: Const<'tcx> and turn it into ty::Const

eddyb (May 14 2019 at 10:35, on Zulip):

the same way miri does when you do tcx.const_eval or w/e

eddyb (May 14 2019 at 10:36, on Zulip):

@oli knows this better than me

Wesley Wiser (May 14 2019 at 10:36, on Zulip):

We've already done const-eval at this point and got a OpTy

Wesley Wiser (May 14 2019 at 10:36, on Zulip):

but we need to turn the OpTy into an Rvalue

Wesley Wiser (May 14 2019 at 10:37, on Zulip):

(there's a type Const = OpTy; statement much earlier in this file)

Wesley Wiser (May 14 2019 at 10:42, on Zulip):

The specific branch you commented on handles the case where OpTy is a Immediate::ScalarPair which happens when evaluating checked math.

Wesley Wiser (May 14 2019 at 10:43, on Zulip):

This test case triggers that code path: https://github.com/rust-lang/rust/pull/60597/files/ba7b8312929f39b890f437063f2eec27984fdd19#diff-441c8694f396fc233e098b0ed39d59a1

Wesley Wiser (May 14 2019 at 10:44, on Zulip):

@eddyb ^ :slight_smile:

eddyb (May 14 2019 at 10:45, on Zulip):

my point is that it doesn't matter what kind of OpTy this is

eddyb (May 14 2019 at 10:45, on Zulip):

it can be uniformly converted into a ty::Const

eddyb (May 14 2019 at 10:45, on Zulip):

and it should be going through that pathway

eddyb (May 14 2019 at 10:46, on Zulip):

which probably does even more validity checks

eddyb (May 14 2019 at 10:47, on Zulip):

I don't think you're using literally tcx.const_eval(...), but rather using miri directly

eddyb (May 14 2019 at 10:47, on Zulip):

tcx.const_eval(...) in particular has a conversion step, from miri to ty::Const

Wesley Wiser (May 14 2019 at 10:49, on Zulip):

Ok

Wesley Wiser (May 14 2019 at 10:49, on Zulip):

Looks like we use eval_promoted https://github.com/rust-lang/rust/blob/ba7b8312929f39b890f437063f2eec27984fdd19/src/librustc_mir/transform/const_prop.rs#L328

oli (May 14 2019 at 11:01, on Zulip):

The problem is code like binary op evaluation: https://github.com/rust-lang/rust/blob/ba7b8312929f39b890f437063f2eec27984fdd19/src/librustc_mir/transform/const_prop.rs#L479

oli (May 14 2019 at 11:02, on Zulip):

for checked bin ops you get a pair of the result + the overflow bit

Wesley Wiser (May 14 2019 at 11:11, on Zulip):

I don't see any functions for converting an MPlaceTy to a ty::Const

oli (May 14 2019 at 11:19, on Zulip):

mplace->const is private: https://github.com/rust-lang/rust/blob/13fde05b12c28e1ed66bd13fdf1ea392f166b811/src/librustc_mir/const_eval.rs#L65

oli (May 14 2019 at 11:19, on Zulip):

and I don't think we should use it

oli (May 14 2019 at 11:20, on Zulip):

@eddyb converting to a ty::Const would be extra effort, what's the problem you see with the mir::Aggregate?

eddyb (May 14 2019 at 11:20, on Zulip):

I think the current implementation is hacky

eddyb (May 14 2019 at 11:20, on Zulip):

and we should be doing something that's both more general and validity-checked

oli (May 14 2019 at 11:21, on Zulip):

hm. that will require useless addtional miri allocations (useless because they will often not end up in llvm)

eddyb (May 14 2019 at 11:22, on Zulip):

because ty::Const can't represent arbitrary pairs?

eddyb (May 14 2019 at 11:22, on Zulip):

what was the reasoning behind that?

eddyb (May 14 2019 at 11:23, on Zulip):

can you at least move the scalar -> ty::Const logic to miri, share it with tcx.const_eval(...) and have it do validity checks?

oli (May 14 2019 at 11:23, on Zulip):

the reasoning was that we didn't really use that feature and most uses just want slice information

eddyb (May 14 2019 at 11:23, on Zulip):

and then we can keep the ty::Tuple hack with a comment that it might be better to just represent the scalar pair case in ty::Const

oli (May 14 2019 at 11:25, on Zulip):

ah, now I understand what issue you have with this.

oli (May 14 2019 at 11:27, on Zulip):

Ok, so we can expose https://github.com/rust-lang/rust/blob/13fde05b12c28e1ed66bd13fdf1ea392f166b811/src/librustc_mir/const_eval.rs#L85 via a wrapper that also does https://github.com/rust-lang/rust/blob/13fde05b12c28e1ed66bd13fdf1ea392f166b811/src/librustc_mir/const_eval.rs#L493 first

eddyb (May 14 2019 at 11:30, on Zulip):

yeah that makes sense

oli (May 14 2019 at 11:30, on Zulip):

I'm not sure how one would get an invalid constant here, we should probably just ICE when that happens. We start out with validated constants and are const propagating the various operations via miri code. What do you think about doing validation directly on the operations just like miri the tool does it and then don't do the propagation if validation fails

eddyb (May 14 2019 at 11:30, on Zulip):

sure

oli (May 14 2019 at 11:31, on Zulip):

basically the const prop optimizations can then be used more wildly, because they will be rolled back if anything looks awry

oli (May 14 2019 at 11:35, on Zulip):

@Wesley Wiser So, as a first step we'd need a test ensuring that &1 as *const i32 as usize does not result in a ty::Const of type usize with value Scalar(Ptr(AllocId(some_number), 0))

oli (May 14 2019 at 11:35, on Zulip):

(after optimizations)

oli (May 14 2019 at 11:35, on Zulip):

that will probably fail right now

oli (May 14 2019 at 11:39, on Zulip):

Then you can add a call to ecx.validate_operand(op, vec![], Some(&mut RefTracking::new(op)), /*const_mode*/true)?; to the conversion method.

oli (May 14 2019 at 11:39, on Zulip):

For good measure throw the conversion method onto InterpCx, since I believe it needs nothing from the const propagator

Wesley Wiser (May 14 2019 at 11:40, on Zulip):

Alright

Wesley Wiser (May 14 2019 at 11:43, on Zulip):

So this:

fn main() {
    let x = &1 as *const i32 as usize;
    read(x);
}

results in

        _3 = &(promoted[0]: i32);
        _2 = move _3 as *const i32 (Misc);
        _1 = move _2 as usize (Misc);
        _4 = const read(move _1) -> bb1;
Wesley Wiser (May 14 2019 at 11:43, on Zulip):

(Before and after my changes)

Wesley Wiser (May 14 2019 at 11:43, on Zulip):

Which seems correct to me

Wesley Wiser (May 14 2019 at 11:44, on Zulip):

Right?

oli (May 14 2019 at 11:48, on Zulip):

hmm. I guess we haven't implemented &

oli (May 14 2019 at 11:49, on Zulip):

use an intermediate const FOO: &i32 = &1; let x = FOO as *const i32 as usize;

Wesley Wiser (May 14 2019 at 11:51, on Zulip):

I don't see any difference there either:

        _2 = const Unevaluated(DefId(0/0:5 ~ test[317d]::main[0]::FOO[0]), []) : &i32 as *const i32 (Misc); // bb0[1]: scope 0 at test.rs:6:13: 6:16
                                         // ty::Const
                                         // + ty: &i32
                                         // + val: Unevaluated(DefId(0/0:5 ~ test[317d]::main[0]::FOO[0]), [])
                                         // mir::Constant
                                         // + span: test.rs:6:13: 6:16
                                         // + ty: &i32
                                         // + literal: Const { ty: &i32, val: Unevaluated(DefId(0/0:5 ~ test[317d]::main[0]::FOO[0]), []) }
        _1 = move _2 as usize (Misc);
        StorageDead(_2);
        _3 = const read(move _1) -> bb1;
Wesley Wiser (May 14 2019 at 11:52, on Zulip):

(Guessing that you're correct and & isn't implemented leading to the Unevaluated stuff)

oli (May 14 2019 at 11:53, on Zulip):

No, Unevaluated happens because we're early in the optimization pipeline

oli (May 14 2019 at 11:53, on Zulip):

& is not implemented for const prop

Wesley Wiser (May 14 2019 at 11:54, on Zulip):

Oh, ok

oli (May 14 2019 at 12:23, on Zulip):

hm, oh sorry I forgot about this thread. Let's see why this isn't getting optimized

oli (May 14 2019 at 12:27, on Zulip):

I don't get it. All the parts are there. are you testing this with your PR and the appropriate optimization levels?

Wesley Wiser (May 14 2019 at 12:31, on Zulip):

Here's what I'm doing:

rustc +stage1-2 --emit mir -C overflow-checks=on -Copt-level=0 -Z mir-opt-level=3 -Z dump-mir="" test.rs
Wesley Wiser (May 14 2019 at 12:31, on Zulip):
narada:rust wesley$ cat test.rs
#[inline(never)]
fn read<T>(_: T) { }

fn main() {
    const FOO: &i32 = &1;
    let x = FOO as *const i32 as usize;
    read(x);
}
oli (May 14 2019 at 12:32, on Zulip):

very weird. I'm assuming 1i32 as u32 works though?

Wesley Wiser (May 14 2019 at 12:32, on Zulip):
narada:rust wesley$ cat mir_dump/rustc.main.003-018.ConstProp.after.mir
// MIR for `main`
// source = MirSource { instance: Item(DefId(0/0:4 ~ test[317d]::main[0])), promoted: None }
// pass_name = ConstProp
// disambiguator = after

fn  main() -> () {
    let mut _0: ();                      // return place in scope 0 at test.rs:4:11: 4:11
    let mut _2: *const i32;              // in scope 0 at test.rs:6:13: 6:30
    let mut _3: &i32;                    // in scope 0 at test.rs:6:13: 6:16
    let mut _4: &i32;                    // in scope 0 at test.rs:6:13: 6:16
    let mut _5: ();                      // in scope 0 at test.rs:7:5: 7:12
    let mut _6: usize;                   // in scope 0 at test.rs:7:10: 7:11
    scope 1 {
        let _1: usize;                   // "x" in scope 1 at test.rs:6:9: 6:10
    }
    scope 2 {
    }

    bb0: {
        StorageLive(_1);                 // bb0[0]: scope 0 at test.rs:6:9: 6:10
        StorageLive(_2);                 // bb0[1]: scope 0 at test.rs:6:13: 6:30
        StorageLive(_3);                 // bb0[2]: scope 0 at test.rs:6:13: 6:16
        StorageLive(_4);                 // bb0[3]: scope 0 at test.rs:6:13: 6:16
        _4 = const Unevaluated(DefId(0/0:5 ~ test[317d]::main[0]::FOO[0]), []) : &i32; // bb0[4]: scope 0 at test.rs:6:13: 6:16
                                         // ty::Const
                                         // + ty: &i32
                                         // + val: Unevaluated(DefId(0/0:5 ~ test[317d]::main[0]::FOO[0]), [])
                                         // mir::Constant
                                         // + span: test.rs:6:13: 6:16
                                         // + ty: &i32
                                         // + literal: Const { ty: &i32, val: Unevaluated(DefId(0/0:5 ~ test[317d]::main[0]::FOO[0]), []) }
        _3 = _4;                         // bb0[5]: scope 0 at test.rs:6:13: 6:16
        _2 = move _3 as *const i32 (Misc); // bb0[6]: scope 0 at test.rs:6:13: 6:16
        StorageDead(_3);                 // bb0[7]: scope 0 at test.rs:6:15: 6:16
        _1 = move _2 as usize (Misc);    // bb0[8]: scope 0 at test.rs:6:13: 6:39
        StorageDead(_2);                 // bb0[9]: scope 0 at test.rs:6:38: 6:39
        StorageDead(_4);                 // bb0[10]: scope 0 at test.rs:6:39: 6:40
        StorageLive(_6);                 // bb0[11]: scope 2 at test.rs:7:10: 7:11
        _6 = _1;                         // bb0[12]: scope 2 at test.rs:7:10: 7:11
        _5 = const read(move _6) -> bb1; // bb0[13]: scope 2 at test.rs:7:5: 7:12
                                         // ty::Const
                                         // + ty: fn(usize) {read::<usize>}
                                         // + val: Scalar(Bits { size: 0, bits: 0 })
                                         // mir::Constant
                                         // + span: test.rs:7:5: 7:9
                                         // + ty: fn(usize) {read::<usize>}
                                         // + literal: Const { ty: fn(usize) {read::<usize>}, val: Scalar(Bits { size: 0, bits: 0 }) }
    }

    bb1: {
        StorageDead(_6);                 // bb1[0]: scope 2 at test.rs:7:11: 7:12
        _0 = ();                         // bb1[1]: scope 0 at test.rs:4:11: 8:2
        StorageDead(_1);                 // bb1[2]: scope 0 at test.rs:8:1: 8:2
        return;                          // bb1[3]: scope 0 at test.rs:8:2: 8:2
    }

    bb2 (cleanup): {
        resume;                          // bb2[0]: scope 0 at test.rs:4:1: 8:2
    }
}
Wesley Wiser (May 14 2019 at 12:33, on Zulip):

If I do let x = 1i32 as u32, I get this:

        _1 = const 1i32 as u32 (Misc);   // bb0[1]: scope 0 at test.rs:5:13: 5:24
                                         // ty::Const
                                         // + ty: i32
                                         // + val: Scalar(Bits { size: 4, bits: 1 })
                                         // mir::Constant
                                         // + span: test.rs:5:13: 5:17
                                         // + ty: i32
                                         // + literal: Const { ty: i32, val: Scalar(Bits { size: 4, bits: 1 }) }
        StorageLive(_3);                 // bb0[2]: scope 2 at test.rs:6:10: 6:11
        _3 = _1;                         // bb0[3]: scope 2 at test.rs:6:10: 6:11
        _2 = const read(move _3) -> bb1;
oli (May 14 2019 at 12:35, on Zulip):

ok, that is super odd. Because const prop works (https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=e3ce68eb4e44d21853e27f67ff989576)

oli (May 14 2019 at 12:35, on Zulip):

did you build with debug assertions?

oli (May 14 2019 at 12:36, on Zulip):

can you run with RUSTC_LOG=rustc_mir::transforms::const_prop and past the output somewhere?

Wesley Wiser (May 14 2019 at 12:36, on Zulip):

I think so?

Wesley Wiser (May 14 2019 at 12:36, on Zulip):

/me checking

Wesley Wiser (May 14 2019 at 12:36, on Zulip):
TRACE 2019-05-14T12:36:33Z: rustc_mir::transform::const_prop: ConstProp starting for DefId(0/0:4 ~ test[317d]::main[0])
TRACE 2019-05-14T12:36:33Z: rustc_mir::transform::const_prop: visit_statement: StorageLive(_1)
TRACE 2019-05-14T12:36:33Z: rustc_mir::transform::const_prop: visit_statement: _1 = const 1i32 as u32 (Misc)
TRACE 2019-05-14T12:36:33Z: rustc_mir::transform::const_prop: checking whether OpTy { op: Indirect(MemPlace { ptr: Ptr(Pointer { alloc_id: AllocId(0), offset: Size { raw: 0 }, tag: () }), align: Align { pow2: 2 }, meta: None }), layout: TyLayout { ty: u32, details: LayoutDetails { variants: Single { index: 0 }, fields: Union(0), abi: Scalar(Scalar { value: Int(I32, false), valid_range: 0..=4294967295 }), align: AbiAndPrefAlign { abi: Align { pow2: 2 }, pref: Align { pow2: 2 } }, size: Size { raw: 4 } } } } can be stored to _1
TRACE 2019-05-14T12:36:33Z: rustc_mir::transform::const_prop: visit_constant: const 1i32
TRACE 2019-05-14T12:36:33Z: rustc_mir::transform::const_prop: visit_statement: StorageLive(_3)
TRACE 2019-05-14T12:36:33Z: rustc_mir::transform::const_prop: visit_statement: _3 = _1
TRACE 2019-05-14T12:36:33Z: rustc_mir::transform::const_prop: visit_constant: const read
TRACE 2019-05-14T12:36:33Z: rustc_mir::transform::const_prop: visit_statement: StorageDead(_3)
TRACE 2019-05-14T12:36:33Z: rustc_mir::transform::const_prop: visit_statement: _0 = ()
TRACE 2019-05-14T12:36:33Z: rustc_mir::transform::const_prop: visit_statement: StorageDead(_1)
TRACE 2019-05-14T12:36:33Z: rustc_mir::transform::const_prop: ConstProp done for DefId(0/0:4 ~ test[317d]::main[0])
TRACE 2019-05-14T12:36:33Z: rustc_mir::transform::const_prop: ConstProp starting for DefId(0/0:3 ~ test[317d]::read[0])
TRACE 2019-05-14T12:36:33Z: rustc_mir::transform::const_prop: visit_statement: _0 = ()
TRACE 2019-05-14T12:36:33Z: rustc_mir::transform::const_prop: ConstProp done for DefId(0/0:3 ~ test[317d]::read[0])
Wesley Wiser (May 14 2019 at 12:37, on Zulip):

debug-assertions = true in my config.toml

oli (May 14 2019 at 12:37, on Zulip):

oh, ignore my example! that was just a check

Wesley Wiser (May 14 2019 at 12:37, on Zulip):

(I've got to run unfortunately)

oli (May 14 2019 at 12:37, on Zulip):

what's the output on your code?

oli (May 14 2019 at 12:37, on Zulip):

no prob! cu

Wesley Wiser (May 14 2019 at 12:37, on Zulip):

The MIR output

Wesley Wiser (May 14 2019 at 12:37, on Zulip):

?

oli (May 14 2019 at 12:37, on Zulip):

No, that you already pasted

oli (May 14 2019 at 12:37, on Zulip):

the trace

oli (May 14 2019 at 12:38, on Zulip):

oh I'm stupid

oli (May 14 2019 at 12:38, on Zulip):

ignore me, you posted everything

oli (May 14 2019 at 12:38, on Zulip):

I'll look into it

Wesley Wiser (May 14 2019 at 12:38, on Zulip):

Thanks @oli and @eddyb!

Wesley Wiser (May 14 2019 at 12:38, on Zulip):

ttyl

oli (May 14 2019 at 13:16, on Zulip):

So I have no clue why it isn't working. We'll need some more debug output. https://github.com/rust-lang/rust/blob/b92b1a76e175f396d7986177d0a2d5907bbba888/src/librustc_mir/transform/const_prop.rs#L547 and all other places setting can_const_prop to false should emit a trace with the local and the reason

oli (May 14 2019 at 13:17, on Zulip):

So https://github.com/rust-lang/rust/blob/b92b1a76e175f396d7986177d0a2d5907bbba888/src/librustc_mir/transform/const_prop.rs#L527 too

oli (May 14 2019 at 13:19, on Zulip):

From your debug trace we hit the line before https://github.com/rust-lang/rust/blob/b92b1a76e175f396d7986177d0a2d5907bbba888/src/librustc_mir/transform/const_prop.rs#L588 but never the one after

Wesley Wiser (May 14 2019 at 13:23, on Zulip):

I can add some of those traces when I get back home tonight

Wesley Wiser (May 14 2019 at 13:24, on Zulip):

I've also seen some things like:

_2 = const 1i32;
_3 = move _2;
_4 = move _3;

defeat const prop.

Wesley Wiser (May 14 2019 at 13:25, on Zulip):

In some situations _3 will be can_const_prop = false

oli (May 14 2019 at 13:42, on Zulip):

if _3 is written twice or a variable, then that is ok

oli (May 14 2019 at 13:42, on Zulip):

we are only const propagating temporaries atm

oli (May 14 2019 at 13:44, on Zulip):

hmm... variables that are only ever written once should be ok, too. I just have a feeling I tried that before and realized there's some odd way it doesn't work

oli (May 14 2019 at 13:44, on Zulip):

maybe let x = if true { 42 } else { 43 };? not sure

Wesley Wiser (May 14 2019 at 13:47, on Zulip):

I have a change locally that improves some of that which I think is ok but would have to be reviewed carefully.

Wesley Wiser (May 14 2019 at 13:47, on Zulip):

It doesn't break any tests though

oli (May 14 2019 at 13:47, on Zulip):

yea, we should do that in a separate PR

oli (May 14 2019 at 13:48, on Zulip):

same thing with this weird issue around casts

Wesley Wiser (May 14 2019 at 13:51, on Zulip):

Regarding the conversion method, should I move that from const_eval.rs to eval_context.rs where InterpretCtx lives? Or should I just make it pub so that it can be called from the conversion method?

oli (May 14 2019 at 13:55, on Zulip):

I thought you created that method in transforms/const_prop.rs?

oli (May 14 2019 at 13:55, on Zulip):

we may be talking about different conversion methods :D

Wesley Wiser (May 14 2019 at 13:55, on Zulip):

Yeah, I think so lol

oli (May 14 2019 at 13:55, on Zulip):

the ones in const_eval.rs are overkill

Wesley Wiser (May 14 2019 at 13:55, on Zulip):

Then you can add a call to ecx.validate_operand(op, vec![], Some(&mut RefTracking::new(op)), /*const_mode*/true)?; to the conversion method.

Wesley Wiser (May 14 2019 at 13:55, on Zulip):

Did you mean a new conversion method?

Wesley Wiser (May 14 2019 at 13:56, on Zulip):

Or the stuff I had added in const_prop?

oli (May 14 2019 at 13:56, on Zulip):

I mean the Scalar -> ty::Const you wrote in const_prop

Wesley Wiser (May 14 2019 at 13:56, on Zulip):

Ah ok

oli (May 14 2019 at 13:56, on Zulip):

just validate the OpTy directly and do your conversion afterwards

Wesley Wiser (May 14 2019 at 13:57, on Zulip):

Ok

Wesley Wiser (May 14 2019 at 13:57, on Zulip):

I'm not sure what you meant here then: https://rust-lang.zulipchat.com/#narrow/stream/189540-t-compiler.2Fwg-mir-opt/topic/const.20propagation/near/165613905

oli (May 14 2019 at 13:57, on Zulip):

that was one of the options. And not my favourite one ^^

Wesley Wiser (May 14 2019 at 13:57, on Zulip):

Ok got it

Wesley Wiser (May 14 2019 at 13:58, on Zulip):

Calling ecx.validate_operand from the Scalar -> Const function in const_prop should be easy enough then

Wesley Wiser (May 15 2019 at 12:01, on Zulip):

The reason why const 1i32 as u32 (Misc); doesn't get const-propagates is because it gets evaluated to an Indirect which replace_with_const doesn't handle:

TRACE 2019-05-15T11:58:05Z: rustc_mir::transform::const_prop: storing OpTy { op: Indirect(MemPlace { ptr: Ptr(Pointer { alloc_id: AllocId(0), offset: Size { raw: 0 }, tag: () }), align: Align { pow2: 2 }, meta: None }), layout: TyLayout { ty: u32, details: LayoutDetails { variants: Single { index: 0 }, fields: Union(0), abi: Scalar(Scalar { value: Int(I32, false), valid_range: 0..=4294967295 }), align: AbiAndPrefAlign { abi: Align { pow2: 2 }, pref: Align { pow2: 2 } }, size: Size { raw: 4 } } } } to _2
oli (May 16 2019 at 12:59, on Zulip):

ha! good catch

oli (May 16 2019 at 12:59, on Zulip):

I don't think anything speaks against handling Indirect by just creating a ByRef constant

oli (May 16 2019 at 13:00, on Zulip):

oh, maybe something does

oli (May 16 2019 at 13:00, on Zulip):

heh, this can be interesting

oli (May 16 2019 at 13:00, on Zulip):

I have written everything very defensively, so it should work, but I may have forgotten some places

oli (May 16 2019 at 13:00, on Zulip):

So if the Indirect doesn't point to the start of an allocation, we may run into trouble down the road

oli (May 16 2019 at 13:01, on Zulip):

I mean ideally we would not even create a duplicate of the allocation in llvm, but just create the const allocation and point to the sub-part of it

oli (May 16 2019 at 13:02, on Zulip):

oh, that may already work (the deduplication in llvm, not sure about the pointing at the right place)

Wesley Wiser (May 16 2019 at 13:24, on Zulip):

Ok. I can look into handling Indirect soon-ish.

Wesley Wiser (May 16 2019 at 13:24, on Zulip):

#60597 is ready for review again I believe.

Wesley Wiser (May 16 2019 at 14:48, on Zulip):

Thanks oli!

Wesley Wiser (May 19 2019 at 20:53, on Zulip):

@oli I'm not sure what RFC your comment is referring to but I think changing the SwitchInt test to call a function other than std::process:exit() should fix the build error on wasm.

oli (May 19 2019 at 20:54, on Zulip):

lolwat, sorry. I commented on the wrong PR

oli (May 19 2019 at 20:55, on Zulip):

sorry, removed the comment and commented on the correct PR

oli (May 19 2019 at 20:55, on Zulip):

E-too-many-tabs

Wesley Wiser (May 19 2019 at 20:59, on Zulip):

Haha I figured :)

Zoxc (May 19 2019 at 21:14, on Zulip):

You should const prop into Yield terminators too. Just repeating here since github likes to hide comments

oli (May 19 2019 at 21:19, on Zulip):

hmm curious. I would have assumed Yield to do the same thing that return does and just write to a magic local before the terminator

Zoxc (May 19 2019 at 21:23, on Zulip):

Nah, it's more source code like. Not sure why we use Pascal style returns in MIR =P

oli (May 19 2019 at 22:10, on Zulip):

because it allows some optimizations like filling in the return value element wise instead of creating a local, filling that in and copying all over at once in the return

oli (May 19 2019 at 22:10, on Zulip):

the same should go for yields

Zoxc (May 19 2019 at 22:24, on Zulip):

It would probably optimize to the same code post-transformation anyway

Wesley Wiser (May 20 2019 at 03:26, on Zulip):

@Zoxc Do you have an example which generates Yields? Whenever I try it, I just get MIR that's already had the Yield lowered: https://gist.github.com/wesleywiser/8af2517dfa5254cd226b8ae6631266e7

Zoxc (May 20 2019 at 03:29, on Zulip):

You'll have to run ConstProp before the transform

Wesley Wiser (May 20 2019 at 03:30, on Zulip):

Oh, ok. I see

Zoxc (May 20 2019 at 03:31, on Zulip):

I think it was moved up due to an issue with one of the later passes

Zoxc (May 20 2019 at 03:32, on Zulip):

The commit which added the Deaggregator moved it

oli (May 20 2019 at 09:02, on Zulip):

hmm... we'd have to run ConstProp multiple times then or move const qualif also before the generator lowering

oli (May 20 2019 at 09:02, on Zulip):

because ConstProp can't run before const_qualif

oli (May 20 2019 at 09:03, on Zulip):

const_qualif is a guaranteed optimization (wrt promotion)

oli (May 20 2019 at 09:03, on Zulip):

const prop may mess that up

centril (May 20 2019 at 22:36, on Zulip):

Have you tested whether always running const prop would help compile times? i.e. also for debug?

Wesley Wiser (May 20 2019 at 22:46, on Zulip):

@centril Yes: https://perf.rust-lang.org/compare.html?start=5f1924c9922c640108d2225d6b68e69e589b94ae&end=7f66c41c4e89b51c8b68f7019cad02c0f46cd1e1

centril (May 20 2019 at 22:47, on Zulip):

cool

Zoxc (May 20 2019 at 22:53, on Zulip):

Why does it help check builds?

Wesley Wiser (May 20 2019 at 22:58, on Zulip):

I assume it's because CTFE tests run less code?

Last update: Nov 17 2019 at 07:15UTC