Stream: t-compiler/const-eval

Topic: dataflow-based const qualification MVP


Dylan MacKenzie (Jun 02 2019 at 20:56, on Zulip):

What are the odds that a "dedicated beginner" (me) could implement dataflow-based const qualification?

I've read some of the background information (#53819), so I have a rough idea of what is needed. I think this is the next logical step now that #58403 has been merged. I'll briefly explain how I would implement this so you can tell me if I'm way off base.

There's currently 4 different qualifications: HasMutInterior, NeedsDrop, IsNotPromotable, IsNotImplicitlyPromotable. Dataflow analysis would track the whether each local is definitely not Qual or maybe Qual (where Qual is one of the aforementioned qualifications). A const block like let a = if 1 == 2 { Some(Vec::new()) } else { None } would result in a being maybe NeedsDrop. There's already a generic framework for bitvector dataflow analysis on MIR, but I'm not sure if it will work here. The join operation for this analysis is just a union (if a local is maybe qualified from one entrypoint and definitely not qualified from another, it becomes maybe qualifed), but trans is a bit more complex since when one local is assigned to another, its qualification bits must also be copied over. I don't think a simple gen/kill framework allows this.

I don't want to slow down implementation of this feature if it's already on someone's todo list, since I will probably be pretty slow. But if @eddyb is super busy, maybe I could help move this along?

Dylan MacKenzie (Jun 02 2019 at 21:12, on Zulip):

Sorry for the extensive edits; this got posted while I was editing it.

RalfJ (Jun 02 2019 at 21:18, on Zulip):

https://github.com/rust-lang/rust/pull/60166/files#r289447620 should probably also land before this is tackled

RalfJ (Jun 02 2019 at 21:19, on Zulip):

Oh and is there any reason IsNotPromotable is not called IsNotConst? I thought implicit promotion is the only promotion we'd have, I never saw the other thing ("normal const stuff") be called "promotion".

Dylan MacKenzie (Jun 02 2019 at 22:15, on Zulip):

In my reading, I found PR #59796 that made the change. The justification appears in https://github.com/rust-lang/rust/pull/58403#discussion_r265978523, but I don't understand the second bullet point under "explicit promotion".

RalfJ (Jun 03 2019 at 07:18, on Zulip):

so seems like this is about & in const context? Hm. But something also has to check things like foo in const X = foo to e.g. not call run-time methods etc...

oli (Jun 03 2019 at 07:40, on Zulip):

Yea.... IsNotPromotable is a bad name and should be IsNotConst, We only have special code for IsNotImplicitlyPromotable since we don't promote const fn calls in regular functions

RalfJ (Jun 03 2019 at 08:39, on Zulip):

but @eddyb in that post linked above seems to say we renamed it from IsNotConst to IsNotImplicitlyPromotable?

RalfJ (Jun 03 2019 at 08:40, on Zulip):

I guess part of the problem here is that in const X: &i32 = &foo();, this is also called "promotion".

oli (Jun 03 2019 at 09:08, on Zulip):

yea

RalfJ (Jun 03 2019 at 09:16, on Zulip):

but then by which name does the analysis go that checks the part "outside the & in a const?

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

well, that's not one analysis, but two: IsNotConst and IsNotImplicitlyPromotable

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

inside a const you only run IsNotConst

RalfJ (Jun 03 2019 at 09:26, on Zulip):

I thought IsNotConst aka IsNotPromotable is what gets used inside the &? Or is that the same thing as outside? (IIRC not, because of concerns like interior mutability)

eddyb (Jun 03 2019 at 10:03, on Zulip):

wait what

eddyb (Jun 03 2019 at 10:03, on Zulip):

I thought we decided IsNotConst was a pointless name?

eddyb (Jun 03 2019 at 10:04, on Zulip):

"not const" is either "not promotable" or "an error anyway"

eddyb (Jun 03 2019 at 10:04, on Zulip):

depending on context

eddyb (Jun 03 2019 at 10:04, on Zulip):

but then by which name does the analysis go that checks the part "outside the & in a const?

it doesn't have a qualification name, that just emits errors

eddyb (Jun 03 2019 at 10:05, on Zulip):

like, the qualifications are properties of values, and "const-checking errored" isn't really one

eddyb (Jun 03 2019 at 10:05, on Zulip):

@oli @RalfJ does that make sense?

eddyb (Jun 03 2019 at 10:06, on Zulip):

@Dylan MacKenzie "There's already a generic framework for bitvector dataflow analysis on MIR, but I'm not sure if it will work here" - yes, that is precisely what we mean by "dataflow"

eddyb (Jun 03 2019 at 10:06, on Zulip):

I never intended for anyone to write a dataflow implementation themselves, I'm sorry if that was ever an interpretation of anything I said

eddyb (Jun 03 2019 at 10:09, on Zulip):

@Dylan MacKenzie so, each Qualif becomes one dataflow implementation (and, yeah, IsNotX should become MaybeNotX and the rest get prefixed with Maybe) - you can do this generically, using the Qualif trait, FWIW

eddyb (Jun 03 2019 at 10:10, on Zulip):

union is | and on assignment you gen (which is |=, really)

eddyb (Jun 03 2019 at 10:10, on Zulip):

you'd never kill anything atm (maybe in the future if we allow an assignment to "clear" flags)

eddyb (Jun 03 2019 at 10:10, on Zulip):

oh I guess you do kill MaybeNeedsDrop on an Operand::Move, sorry

eddyb (Jun 03 2019 at 10:10, on Zulip):

but that's it

eddyb (Jun 03 2019 at 10:11, on Zulip):

like, https://github.com/rust-lang/rust/blob/master/src/librustc_mir/dataflow/impls/storage_liveness.rs is pretty okay template to start off with

eddyb (Jun 03 2019 at 10:12, on Zulip):

computing the dataflow should be easy, the slightly trickier part is inspecting the results (because we don't have an ergonomic cursor for it yet)

eddyb (Jun 03 2019 at 10:13, on Zulip):

or, actually, we don't have to rename anything, you could have Maybe<NeedsDrop> where Maybe is the wrapper that does dataflow

eddyb (Jun 03 2019 at 10:15, on Zulip):

@RalfJ the most important thing is something like some_simd_intrinsic_with_required_const_arg(a, b, 123) - that's forceful promotion so it allows const fn calls, unlike implicit promotion

RalfJ (Jun 03 2019 at 10:20, on Zulip):

like, the qualifications are properties of values, and "const-checking errored" isn't really one

that's weird, where is const-checking happening then?^^

RalfJ (Jun 03 2019 at 10:22, on Zulip):

that's forceful promotion so it allows const fn calls, unlike implicit promotion

this is still not syntactically visible as const context. Seems dangerous to me.

RalfJ (Jun 03 2019 at 10:22, on Zulip):

Last time we were like "sure let's just allow const fn there" we painted ourselves into a very bad corner. Did we just repeat that mistake...? :(

RalfJ (Jun 03 2019 at 10:23, on Zulip):

also, "calls const fn" is not a property of a value, so I am very confused now about what these "qualifications" are

RalfJ (Jun 03 2019 at 14:16, on Zulip):

@eddyb ^

eddyb (Jun 03 2019 at 14:18, on Zulip):

const-checking "just happens". it emits errors

eddyb (Jun 03 2019 at 14:19, on Zulip):

it's not reified into flags. it either errors and prevents compilation from succeeding, or doesn't

eddyb (Jun 03 2019 at 14:19, on Zulip):

"the computation of this value may involve a const fn call" is a better description, I guess

RalfJ (Jun 03 2019 at 14:19, on Zulip):

const-checking "just happens". it emits errors

well that does not seem to work: https://github.com/rust-lang/rust/pull/61399#discussion_r289727281

RalfJ (Jun 03 2019 at 14:20, on Zulip):

there we have code that calls a non-const intrinsic, which is allowed -- the check supposed to prevent that is in IsNotPromotable::in_call

RalfJ (Jun 03 2019 at 14:20, on Zulip):

"the computation of this value may involve a const fn call" is a better description, I guess

it's not values then that matter, but locals? because "the computation of a value" is a nonesneical statement, a value (e.g. 3) is already computed

RalfJ (Jun 03 2019 at 14:21, on Zulip):

I'd love if these qualifications could be stated as syntactic appromximations of dynamic properties of a semantic value, but that is seemingly not the case

eddyb (Jun 03 2019 at 14:21, on Zulip):

I don't mean a concrete value, but things like Operands

RalfJ (Jun 03 2019 at 14:21, on Zulip):

so, expressions?

eddyb (Jun 03 2019 at 14:21, on Zulip):

they're not "expressions" in MIR, that's my point

RalfJ (Jun 03 2019 at 14:22, on Zulip):

well the closest we got to a consensus in https://github.com/rust-lang/unsafe-code-guidelines/pull/40 was to use "expression" for not-yet-computed things

RalfJ (Jun 03 2019 at 14:22, on Zulip):

they're certainly not values either, FWIW :)

RalfJ (Jun 03 2019 at 14:23, on Zulip):

NeedsDrop and HasInteriorMut are properties of values

RalfJ (Jun 03 2019 at 14:23, on Zulip):

though I am also sometimes confused about NeedsDrop, because sometimes it seems to be about whether the computed value needs dropping, and sometimes about whether computing the value requires dropping some other stuff

RalfJ (Jun 03 2019 at 14:24, on Zulip):

the latter really being a "calling non-const fn" kind of check

eddyb (Jun 03 2019 at 14:24, on Zulip):

no, it's definitely not the latter

eddyb (Jun 03 2019 at 14:25, on Zulip):

drops aren't part of the computation, they're side-effects that aren't tracked by any of this

eddyb (Jun 03 2019 at 14:25, on Zulip):

@RalfJ I guess you could s/expression/dataflow (sub)graph

RalfJ (Jun 03 2019 at 14:25, on Zulip):

the point is, we have some checks for stuff that need to happen in const context -- such as only calling const fn, not doing forbidden operations (ptr-int casts or so), and so on. the same checks are also relevant when determining candidates for implicit promotion, and the same checks apply in "explicit promotion". So I thought this would all be the same framework.

RalfJ (Jun 03 2019 at 14:25, on Zulip):

drops aren't part of the computation, they're side-effects that aren't tracked by any of this

drops are definitely part of the computation though... at least the drops happening during the computation

eddyb (Jun 03 2019 at 14:26, on Zulip):

we don't actually look at Drop terminators

RalfJ (Jun 03 2019 at 14:26, on Zulip):

oO

eddyb (Jun 03 2019 at 14:26, on Zulip):

so I don't know why you thought that

RalfJ (Jun 03 2019 at 14:26, on Zulip):

well because its the only thing that makes sense?^^

eddyb (Jun 03 2019 at 14:26, on Zulip):

maybe you confused with the side-effect Operand::Move has?

eddyb (Jun 03 2019 at 14:26, on Zulip):

why would it make sense?

RalfJ (Jun 03 2019 at 14:27, on Zulip):

the only reason we want to prevent drops is to avoid calling drop glue that is not a const fn right?

eddyb (Jun 03 2019 at 14:27, on Zulip):

NeedsDrop is "Drop(x) might not be a noop"

RalfJ (Jun 03 2019 at 14:27, on Zulip):

so for normal calls we look at Call terminators, and then we also need to separately take care of the implicit drop calls

RalfJ (Jun 03 2019 at 14:27, on Zulip):

that's the dynamic property we actually care about: dont execute a Drop terminator

eddyb (Jun 03 2019 at 14:27, on Zulip):

brb then I'll look at the code and explain this a bit better

eddyb (Jun 03 2019 at 14:28, on Zulip):

but anyway, in const contexts, we emit errors. whether we emit all the errors we should is another story, and consititutes bugs, basically

RalfJ (Jun 03 2019 at 14:28, on Zulip):

NeedsDrop is a different thing, I understand that. we also need to make sure const dont need dropping or so.

RalfJ (Jun 03 2019 at 14:28, on Zulip):

but the other thing also has to be checked somewhere

RalfJ (Jun 03 2019 at 14:30, on Zulip):

but taking a step back -- IsNotPromotable::in_call performs some crucial checks that should be done for any function call in const context. and that makes sense, after all this is about whether some function call can be (explicitly?) promoted to context context. So to me this really sounds like an IsNotConst analysis. And the fact that we don't treat it as such seems to already cause bugs -- and I don't understand how we even ended up there.

RalfJ (Jun 03 2019 at 14:33, on Zulip):

I dont see a qualitative difference between the kind of checks that need to be done in const context to determine if this is okay, and the kind of checks that need to be done "on parts of the MIR of a normal function" to determine if we can promote it. many of the checks are very similar, just the latter case should be a bit more restrictive.
The fact that we have two different kinds of things working "on parts of the MIR of a normal function" makes me very uneasy. I don't see any good argument for that. We already have a way to explicitly ask for const context (and hence get everything "promoted"): make it a const item.

RalfJ (Jun 03 2019 at 14:33, on Zulip):

I don't think the niche usecase of SIMD intrinsics with some arguments having to be const justifies making promotability analysis (an extremely subtle piece of code with a history of bugs) more complicated

eddyb (Jun 03 2019 at 14:34, on Zulip):

sigh

eddyb (Jun 03 2019 at 14:35, on Zulip):

so, Drop: we read NeedsDrop, but not write it: https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/qualify_consts.rs#L1357

eddyb (Jun 03 2019 at 14:35, on Zulip):

does this make sense?

eddyb (Jun 03 2019 at 14:36, on Zulip):

NeedsDrop doesn't capture that a Drop has happened or might happen or anything like that

eddyb (Jun 03 2019 at 14:37, on Zulip):

this looks wrong tho https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/qualify_consts.rs#L489

RalfJ (Jun 03 2019 at 14:37, on Zulip):

yeah I got that :) now I am not sure where "the other" drop analysis is happening though but that's okay. probably the same place as some of the other things that I dont know where they are happening.

eddyb (Jun 03 2019 at 14:38, on Zulip):

uhm, there is no other analysis?

eddyb (Jun 03 2019 at 14:38, on Zulip):

I really don't know what you're talking about

RalfJ (Jun 03 2019 at 14:38, on Zulip):

"const checking", wherever that is in the code

eddyb (Jun 03 2019 at 14:39, on Zulip):

no, that is the check

eddyb (Jun 03 2019 at 14:39, on Zulip):

you're looking at it

eddyb (Jun 03 2019 at 14:39, on Zulip):

that's const-checking

RalfJ (Jun 03 2019 at 14:40, on Zulip):

the "other drop analysis" is the one that makes this an error:

struct Foo;
impl Drop for Foo {
    fn drop(&mut self) {}
}
const X: i32 = (3, Foo).0;
eddyb (Jun 03 2019 at 14:40, on Zulip):

what I linked is what causes that error

eddyb (Jun 03 2019 at 14:41, on Zulip):

it really is that simple

RalfJ (Jun 03 2019 at 14:41, on Zulip):

simple?!??

RalfJ (Jun 03 2019 at 14:41, on Zulip):

this is all crazy complicated^^

RalfJ (Jun 03 2019 at 14:41, on Zulip):

simple would be "walk the MIR, if there is a Drop terminator then complain". or something like that.

eddyb (Jun 03 2019 at 14:41, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/qualify_consts.rs#L1370-L1371

eddyb (Jun 03 2019 at 14:41, on Zulip):

it's right here

eddyb (Jun 03 2019 at 14:41, on Zulip):

yes, this is what this is https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/qualify_consts.rs#L1348

eddyb (Jun 03 2019 at 14:41, on Zulip):

this is just a visit of the MIR

RalfJ (Jun 03 2019 at 14:42, on Zulip):

but this is... not part of any qualif...?

eddyb (Jun 03 2019 at 14:42, on Zulip):

it checks NeedsDrop because this runs pre-elaboration (drop elaboration has to run after borrowck but this code must run before borrowck)

eddyb (Jun 03 2019 at 14:42, on Zulip):

no, it's inside this impl block https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/qualify_consts.rs#L923-L928

RalfJ (Jun 03 2019 at 14:43, on Zulip):

hm okay

eddyb (Jun 03 2019 at 14:44, on Zulip):

the qualifs have been split out, so everything should be fairly readable now

RalfJ (Jun 03 2019 at 14:44, on Zulip):

the overall structure is still making my head explode

eddyb (Jun 03 2019 at 14:44, on Zulip):

they are computed structurally, and then read by the checking code

RalfJ (Jun 03 2019 at 14:44, on Zulip):

like, so where's the thing that prevents &(0, Foo).0 from being promoted?

eddyb (Jun 03 2019 at 14:45, on Zulip):

once we move to dataflow they'll be even more decoupled

eddyb (Jun 03 2019 at 14:45, on Zulip):

@RalfJ that is the promotion check also reading NeedsDrop

eddyb (Jun 03 2019 at 14:45, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/qualify_consts.rs#L760

RalfJ (Jun 03 2019 at 14:45, on Zulip):

can't be that check because that shows an error (those mode != Fn things are very hard to understand... I got it now but there's no comments, at least not the places where these decisions are being made)

RalfJ (Jun 03 2019 at 14:47, on Zulip):

(and yes it did get better than it was before, but the structure is still different from what I'd expect so I am still confused^^)

eddyb (Jun 03 2019 at 14:48, on Zulip):

mode != Fn means this is not a runtime-only Fn

eddyb (Jun 03 2019 at 14:48, on Zulip):

maybe Fn should be named RuntimeFn?

eddyb (Jun 03 2019 at 14:48, on Zulip):

would that help?

RalfJ (Jun 03 2019 at 14:48, on Zulip):

yeah so this is basuically if mode.is_const_context()?

eddyb (Jun 03 2019 at 14:49, on Zulip):

yes

RalfJ (Jun 03 2019 at 14:49, on Zulip):

:+1:

eddyb (Jun 03 2019 at 14:49, on Zulip):

or, rather .may_const_eval (since ConstFn can be called at runtime too)

RalfJ (Jun 03 2019 at 14:49, on Zulip):

sure but that's still "const context" as far as I am concerned

eddyb (Jun 03 2019 at 14:49, on Zulip):

and "const context" is ambiguous because it could be interpreted as Mode::Const

eddyb (Jun 03 2019 at 14:49, on Zulip):

right

RalfJ (Jun 03 2019 at 14:50, on Zulip):

"const context" means "typechecked according to the rules of the const-type-system"

RalfJ (Jun 03 2019 at 14:50, on Zulip):

may_const_eval is somewhat ambiguous becuase code in a RuntimeFn may const-eval if it gets promoted

eddyb (Jun 03 2019 at 14:51, on Zulip):

hmm right

eddyb (Jun 03 2019 at 14:51, on Zulip):

crap

eddyb (Jun 03 2019 at 14:51, on Zulip):

that return true is indeed a bug

RalfJ (Jun 03 2019 at 14:51, on Zulip):

which is the other thing that I am not clear about yet (and where I think we have a bug)

eddyb (Jun 03 2019 at 14:51, on Zulip):

ARGH

eddyb (Jun 03 2019 at 14:51, on Zulip):

there are no tests for it!

RalfJ (Jun 03 2019 at 14:51, on Zulip):

it deeply saddens me that we got two kinds of promotion :(

RalfJ (Jun 03 2019 at 14:51, on Zulip):

for what?

RalfJ (Jun 03 2019 at 14:51, on Zulip):

ah

eddyb (Jun 03 2019 at 14:52, on Zulip):

@oli we merged a boolean confusion back in my refactor and no tests caught it

RalfJ (Jun 03 2019 at 14:52, on Zulip):

so it should be... return false? no that makes less sense?

eddyb (Jun 03 2019 at 14:52, on Zulip):

the _ => {} should be _ => return true

eddyb (Jun 03 2019 at 14:52, on Zulip):

it's a whitelist

RalfJ (Jun 03 2019 at 14:53, on Zulip):

ah and the return true should be {}?

oli (Jun 03 2019 at 14:53, on Zulip):

that entire match is useless where it is

oli (Jun 03 2019 at 14:53, on Zulip):

it should be in https://github.com/rust-lang/rust/blob/7096ff0ce16b0544b717986ec335798b3151dd8e/src/librustc_mir/transform/qualify_consts.rs#L1229 I believe

RalfJ (Jun 03 2019 at 14:54, on Zulip):

that was the point I was getting to. which of the "qualifs" are used for "const checking"? I saw NeedsDrop is used, but what about the others? like, checking that calls are const fn?

RalfJ (Jun 03 2019 at 14:54, on Zulip):

seems like most of "const checking" is also relevant for deciding if something should get promoted, which is why I originally thought that this was all done inside "qualifs"

RalfJ (Jun 03 2019 at 14:55, on Zulip):

but it seems instead we have some of that duplicated (like checking for NeedsDrop at the Drop terminator for "const checking", and checking for NeedsDrop somewhere in the IsNotPromotable analysis)

eddyb (Jun 03 2019 at 14:56, on Zulip):

we don't check for NeedsDrop in IsNotPromotable

eddyb (Jun 03 2019 at 14:56, on Zulip):

like, the IsNot are general-purpose "for other reasons"

RalfJ (Jun 03 2019 at 14:56, on Zulip):

well we check for "is there any qualif set"

RalfJ (Jun 03 2019 at 14:57, on Zulip):

at https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/qualify_consts.rs#L760 as you showed me

RalfJ (Jun 03 2019 at 14:57, on Zulip):

that is basically a duplicate of the Drop terminator check

eddyb (Jun 03 2019 at 14:57, on Zulip):

yeah

RalfJ (Jun 03 2019 at 14:57, on Zulip):

where I thought that this would be shared (not just the analysis, but the check itself) -- part of why I was so confused

eddyb (Jun 03 2019 at 14:59, on Zulip):

I mean the check is... just asking if it's set

eddyb (Jun 03 2019 at 14:59, on Zulip):

anyway, let's focus on figuring out this intrinsic-related bug :/

RalfJ (Jun 03 2019 at 15:00, on Zulip):

and then same with the const fn checks, which we seem to have at https://github.com/rust-lang/rust/blob/7096ff0ce16b0544b717986ec335798b3151dd8e/src/librustc_mir/transform/qualify_consts.rs#L1243 and https://github.com/rust-lang/rust/blob/7096ff0ce16b0544b717986ec335798b3151dd8e/src/librustc_mir/transform/qualify_consts.rs#L495

RalfJ (Jun 03 2019 at 15:00, on Zulip):

I mean the check is... just asking if it's set

sure but it has to remember to do that in a place that has nothing to do with Drop.

eddyb (Jun 03 2019 at 15:01, on Zulip):

yes because that's how we defined the &rvalue promotion

RalfJ (Jun 03 2019 at 15:01, on Zulip):

we defined the extensional effect, not how it gets achieved.

eddyb (Jun 03 2019 at 15:01, on Zulip):

as opposed to checking if there is a Drop for that temporary

eddyb (Jun 03 2019 at 15:01, on Zulip):

which we can technically do nowadays

eddyb (Jun 03 2019 at 15:01, on Zulip):

it was just never considered

RalfJ (Jun 03 2019 at 15:02, on Zulip):

I see

eddyb (Jun 03 2019 at 15:02, on Zulip):

the Drop check came later

eddyb (Jun 03 2019 at 15:02, on Zulip):

and it was a relaxation of NeedsDrop values being completely banned from compile-time

eddyb (Jun 03 2019 at 15:03, on Zulip):

@oli confirmed boolean confusion https://github.com/rust-lang/rust/commit/f04424acd1bf894d1dc930c2a347871ea8b96dfa#diff-c2552a106550d05b69d5e07612f0f812L391

eddyb (Jun 03 2019 at 15:04, on Zulip):

do you want to fix this or should I?

eddyb (Jun 03 2019 at 15:04, on Zulip):

I want this fixed before we move anything around

eddyb (Jun 03 2019 at 15:05, on Zulip):

so what this did is allow calls to intrinsics other than those listed, to be promoted

RalfJ (Jun 03 2019 at 15:05, on Zulip):

lol

RalfJ (Jun 03 2019 at 15:06, on Zulip):

but does that mean that so far nobody can actually use these intrinsics in a rustc_const_arg? So we could just stick to not allowing them there?

eddyb (Jun 03 2019 at 15:06, on Zulip):

basically the {} and return true are swapped :(

eddyb (Jun 03 2019 at 15:06, on Zulip):

@RalfJ it's important for SIMD people to be able to call their const fns for these arguments

eddyb (Jun 03 2019 at 15:06, on Zulip):

like, this is already considered settled

eddyb (Jun 03 2019 at 15:07, on Zulip):

and on stable AFAIK

RalfJ (Jun 03 2019 at 15:07, on Zulip):

swapping the two branches is not enough though

RalfJ (Jun 03 2019 at 15:07, on Zulip):

because currently we also allow all intrinsics to be called in a const fn

RalfJ (Jun 03 2019 at 15:07, on Zulip):

(except for transmute)

eddyb (Jun 03 2019 at 15:08, on Zulip):

yes but I want this to be fixed, with a test. and maybe evaluate the impact

eddyb (Jun 03 2019 at 15:08, on Zulip):

how many intrinsics have we exposed on stable?

RalfJ (Jun 03 2019 at 15:09, on Zulip):

IIRC just transmute nowadays

RalfJ (Jun 03 2019 at 15:09, on Zulip):

(we un-exposed some of them recently)

eddyb (Jun 03 2019 at 15:09, on Zulip):

oh I see

RalfJ (Jun 03 2019 at 15:09, on Zulip):

but that's for "core intrinsics"

RalfJ (Jun 03 2019 at 15:09, on Zulip):

doesnt SIMD have a metric shit-ton of intrinsics and expose them all?

eddyb (Jun 03 2019 at 15:09, on Zulip):

kinda

RalfJ (Jun 03 2019 at 15:10, on Zulip):

RalfJ it's important for SIMD people to be able to call their const fns for these arguments

that makes sense. but is there any way we could let those things be checked by "const checking"? Seems reasonable to accept the same things there and in const items, without having to write the check twice...

eddyb (Jun 03 2019 at 15:10, on Zulip):

they also abuse "linking" to LLVM intrinsics directly, which is a nightmare

RalfJ (Jun 03 2019 at 15:10, on Zulip):

so it might be that SIMD intrinsics are callable from const fn this way

eddyb (Jun 03 2019 at 15:10, on Zulip):

I really don't know what you mean by "twice"

RalfJ (Jun 03 2019 at 15:11, on Zulip):

like https://github.com/rust-lang/rust/blob/7096ff0ce16b0544b717986ec335798b3151dd8e/src/librustc_mir/transform/qualify_consts.rs#L1243 and https://github.com/rust-lang/rust/blob/7096ff0ce16b0544b717986ec335798b3151dd8e/src/librustc_mir/transform/qualify_consts.rs#L495

RalfJ (Jun 03 2019 at 15:11, on Zulip):

which both separately rule out non-const-fn calls

RalfJ (Jun 03 2019 at 15:11, on Zulip):

I am not sure where the other things are enforced for both of these... like using unsafe operations or casting ptrs to ints

eddyb (Jun 03 2019 at 15:12, on Zulip):

ah I see

eddyb (Jun 03 2019 at 15:12, on Zulip):

well, for const-checking they need to emit errors

eddyb (Jun 03 2019 at 15:12, on Zulip):

that depend on various things

RalfJ (Jun 03 2019 at 15:12, on Zulip):

for rustc_const_arg they also need to emit errors

eddyb (Jun 03 2019 at 15:12, on Zulip):

the error is very different

eddyb (Jun 03 2019 at 15:13, on Zulip):

rustc_const_arg doesn't cause the argument to be checked as a constant

RalfJ (Jun 03 2019 at 15:13, on Zulip):

is there any reason it should be? "you wrote a thing that can't be CTFE'd in a place where we must do CTFE".

eddyb (Jun 03 2019 at 15:13, on Zulip):

although it would be interesting to try...

eddyb (Jun 03 2019 at 15:14, on Zulip):

but it would require a much stranger scheme

oli (Jun 03 2019 at 15:14, on Zulip):

@Mahmut Bulut is looking into statically checking intrinsics in const eval, let's see how bad the fallout is first I guess?

eddyb (Jun 03 2019 at 15:14, on Zulip):

right now we just compute forward

RalfJ (Jun 03 2019 at 15:14, on Zulip):

I mean I guess a slong as "can call const fn" is the only difference between implicit and explicit promotion we should be good (as in, we should have fairly reasonable test coverage)

eddyb (Jun 03 2019 at 15:14, on Zulip):

@oli please ping me here or on IRC about this so I don't keep track of it

oli (Jun 03 2019 at 15:14, on Zulip):

will do

eddyb (Jun 03 2019 at 15:15, on Zulip):

to be clear, are you including the return true bug?

eddyb (Jun 03 2019 at 15:15, on Zulip):

(but seriously, I want that in isolation, because when I made that mistake we had no tests - and we clearly still don't)

oli (Jun 03 2019 at 15:15, on Zulip):

yes, I'll make sure they are separate PRs then

eddyb (Jun 03 2019 at 15:18, on Zulip):

okay ping me on IRC and I'll r+ ASAP

RalfJ (Jun 03 2019 at 15:23, on Zulip):

there's also some overlap between the IsNotPromotable vs IsNotImplicitlyPromotable, and mode. We want to allow const fn calls both inside const items and inside rustc_const_arg arguments.

RalfJ (Jun 03 2019 at 15:23, on Zulip):

Seems weird to have IsNotImplicitlyPromotable look at the mode at all TBH

eddyb (Jun 03 2019 at 15:23, on Zulip):

I did leave a few comments about some stuff like that

eddyb (Jun 03 2019 at 15:23, on Zulip):

although I'm not sure what you mean

RalfJ (Jun 03 2019 at 15:27, on Zulip):

why does IsNotImplicitlyPromotable look at the mode? instead I'd think that in situations like static X: T := &foo, we'd only look at IsNotPromotable

RalfJ (Jun 03 2019 at 15:28, on Zulip):

similar to how we only look at IsNotPromotable for rustc_const_arg

eddyb (Jun 03 2019 at 15:28, on Zulip):

you're saying that's explicit promotion?

RalfJ (Jun 03 2019 at 15:28, on Zulip):

yes

RalfJ (Jun 03 2019 at 15:28, on Zulip):

with the same argument

RalfJ (Jun 03 2019 at 15:28, on Zulip):

its happening inside a context where we have to use const

eddyb (Jun 03 2019 at 15:28, on Zulip):

but that's not even promotion :P

RalfJ (Jun 03 2019 at 15:28, on Zulip):

well it is currently treated as such is it not?

eddyb (Jun 03 2019 at 15:28, on Zulip):

that's just &'static without any promotion happening

RalfJ (Jun 03 2019 at 15:29, on Zulip):

when I did some testing it behaves the same

RalfJ (Jun 03 2019 at 15:29, on Zulip):

ah sorry take a variant of this like (0, &foo)

eddyb (Jun 03 2019 at 15:29, on Zulip):

you need something like f(&g()), I think

RalfJ (Jun 03 2019 at 15:29, on Zulip):

for some reason those are treated differently...

RalfJ (Jun 03 2019 at 15:29, on Zulip):

yeah something like that

RalfJ (Jun 03 2019 at 15:29, on Zulip):

not sure why that makes any difference FWIW^^

RalfJ (Jun 03 2019 at 15:29, on Zulip):

Some(&foo) was my usual way to get there IIRC

eddyb (Jun 03 2019 at 15:29, on Zulip):

tuple might still be top-level, I don't remember

eddyb (Jun 03 2019 at 15:30, on Zulip):

it's the rule from let x = &f();

RalfJ (Jun 03 2019 at 15:30, on Zulip):

I cant remember because in my head it makes no sense to treat them differently than top-level &^^

eddyb (Jun 03 2019 at 15:30, on Zulip):

yeah, Some(&foo()) needs promotion

eddyb (Jun 03 2019 at 15:30, on Zulip):

well, top-level allows more

RalfJ (Jun 03 2019 at 15:30, on Zulip):

but why?

eddyb (Jun 03 2019 at 15:30, on Zulip):

like, &String::new() works but Some(&String::new()) doesn't

RalfJ (Jun 03 2019 at 15:30, on Zulip):

but why?^^

eddyb (Jun 03 2019 at 15:31, on Zulip):

because that's the only thing that fit the constraints we had?

RalfJ (Jun 03 2019 at 15:31, on Zulip):

I dont know what that means

eddyb (Jun 03 2019 at 15:31, on Zulip):

it used to be more relaxed, actually, which was very bad

RalfJ (Jun 03 2019 at 15:31, on Zulip):

but I dont know of any soundness reason to treat them differently

RalfJ (Jun 03 2019 at 15:31, on Zulip):

like, anything you are allowed top-level, you should also be allowed "further in"

eddyb (Jun 03 2019 at 15:31, on Zulip):

if we rely on promotion for the top-level, some things are impossible

eddyb (Jun 03 2019 at 15:32, on Zulip):

unless we start allowing to promote them

RalfJ (Jun 03 2019 at 15:32, on Zulip):

well is there anything we want to allow top-level but not allow in explicit promotion?

eddyb (Jun 03 2019 at 15:32, on Zulip):

and we don't want to promote Some(&ThisNeedsDrop) anywhere

eddyb (Jun 03 2019 at 15:32, on Zulip):

since we want drops to have consistent semantics

RalfJ (Jun 03 2019 at 15:33, on Zulip):

I mean I'd also be fine not considering that to be part of promotion at all, neither top-level nor nested. it's mostly the inconsistency that bothers me.

eddyb (Jun 03 2019 at 15:33, on Zulip):

but we used to allow Some(&anything)

RalfJ (Jun 03 2019 at 15:33, on Zulip):

where?

eddyb (Jun 03 2019 at 15:34, on Zulip):

in constants

RalfJ (Jun 03 2019 at 15:37, on Zulip):

well there need to be some checks, but the same checks as for top-level

RalfJ (Jun 03 2019 at 15:37, on Zulip):

@oli I dont understand the comment at https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/qualify_consts.rs#L1212
// never promote transmute calls but this doesn't even do anything for runtime-functions (i.e., where promotion would matter)

oli (Jun 03 2019 at 15:40, on Zulip):

that comment belongs to https://github.com/rust-lang/rust/blob/7096ff0ce16b0544b717986ec335798b3151dd8e/src/librustc_mir/transform/qualify_consts.rs#L488

RalfJ (Jun 03 2019 at 15:42, on Zulip):

yeah but that's not what that code does^^ okay

RalfJ (Jun 03 2019 at 15:42, on Zulip):

@eddyb for another insteance of a duplicate check, see the two occurrences of is_union in that file

RalfJ (Jun 03 2019 at 15:42, on Zulip):

we basically have most checks twice and it's very hard to tell if they are properly in sync

RalfJ (Jun 03 2019 at 15:49, on Zulip):

also why is it correct to do the "dont promote things that would execute drop during their evaluation" check only on temporaries?

eddyb (Jun 03 2019 at 15:59, on Zulip):

because we don't promote non-temporaries

RalfJ (Jun 03 2019 at 16:02, on Zulip):

ah

eddyb (Jun 03 2019 at 16:03, on Zulip):

@oli before my change the whitelist, this doesn't do anything https://github.com/rust-lang/rust/blob/732a2dc09577f93c5eb9e4f037f7ce6723d8d7eb/src/librustc_mir/transform/qualify_consts.rs#L875

eddyb (Jun 03 2019 at 16:04, on Zulip):

oh god there was an ICE I removed https://github.com/rust-lang/rust/blob/732a2dc09577f93c5eb9e4f037f7ce6723d8d7eb/src/librustc_mir/transform/qualify_consts.rs#L1038-L1041

oli (Jun 03 2019 at 16:04, on Zulip):

@eddyb it does something, it keeps is_const_fn at false

eddyb (Jun 03 2019 at 16:04, on Zulip):

load-bearing delay_span_bugs Q_Q

eddyb (Jun 03 2019 at 16:09, on Zulip):

@oli but there are no errors attached to is_const_fn!

RalfJ (Jun 03 2019 at 16:10, on Zulip):

I tried to cast some of the things you told me into comments @eddyb , please have a look at https://github.com/rust-lang/rust/pull/61492 and check that they reflect what you wanted to say

oli (Jun 03 2019 at 16:10, on Zulip):

yea :confused: I just saw it.

eddyb (Jun 03 2019 at 16:10, on Zulip):

so basically people added intrinsics to the whitelist to disable the ICE

eddyb (Jun 03 2019 at 16:11, on Zulip):

@RalfJ @oli I still think "const context" is ambiguous :/

oli (Jun 03 2019 at 16:11, on Zulip):

I'm sure my original implementation didn't have an ICE...

oli (Jun 03 2019 at 16:11, on Zulip):

not sure what I broke from then on

RalfJ (Jun 03 2019 at 16:13, on Zulip):

RalfJ oli I still think "const context" is ambiguous :/

hm. it is terminology I have been using for a bit, including in some blog posts. and so far nobody complained^^

eddyb (Jun 03 2019 at 16:13, on Zulip):

@RalfJ wait, there is one way to fix this: rename Mode::Const to Mode::OtherConst

RalfJ (Jun 03 2019 at 16:14, on Zulip):

sure I can do that. I guess when I rename things I can also rename Fn to NonConstFn or so?

eddyb (Jun 03 2019 at 16:14, on Zulip):

it's very much contextually ambiguous :P

eddyb (Jun 03 2019 at 16:14, on Zulip):

hmm sure

RalfJ (Jun 03 2019 at 16:16, on Zulip):

RalfJ wait, there is one way to fix this: rename Mode::Const to Mode::OtherConst

what about Mode::ConstItem?

eddyb (Jun 03 2019 at 16:17, on Zulip):

no, because it's a catch-all for anon consts too

eddyb (Jun 03 2019 at 16:17, on Zulip):

like [T; 0], Array<T, 1> etc.

oli (Jun 03 2019 at 16:18, on Zulip):

const_mode: Option<ConstMode>, where Fn is None instead of a variant on Mode?

eddyb (Jun 03 2019 at 16:18, on Zulip):

@RalfJ ftr, the duplication would slowly go away as we allow more things in the const context

RalfJ (Jun 03 2019 at 16:22, on Zulip):

RalfJ ftr, the duplication would slowly go away as we allow more things in the const context

that's a good point

RalfJ (Jun 03 2019 at 16:23, on Zulip):

in the "endgame", what is left to check for general const context? calling only const fn, which you guys are currently struggling to fix for intrinsics ;) , and maybe some unconst stuff? But there'll still be Drop terminator checks, too, and something about interior mutability. (and hopefully Sync...)

eddyb (Jun 03 2019 at 16:25, on Zulip):

I mean having const checks is not a problem

eddyb (Jun 03 2019 at 16:25, on Zulip):

the annoying thing is that some logic must be replicated for qualifying promotion

eddyb (Jun 03 2019 at 16:26, on Zulip):

@RalfJ it's taking me a while to see what you mean but some of the more fine-grained duplication... is part of the cleanup

eddyb (Jun 03 2019 at 16:26, on Zulip):

before it was unduplicated because the qualification was done by mutating while checking!

eddyb (Jun 03 2019 at 16:26, on Zulip):

whereas we want to move to just using the dataflow framework

RalfJ (Jun 03 2019 at 16:27, on Zulip):

the annoying thing is that some logic must be replicated for qualifying promotion

yes

RalfJ (Jun 03 2019 at 16:27, on Zulip):

the cleanup did remove some other duplication though I feel -- or at least made the overall structure much clearer -- that that was still definitely a good step IMO

eddyb (Jun 03 2019 at 16:28, on Zulip):

@RalfJ anyway, I don't think I managed to get across why top-level borrows are "leaked" to 'static instead of promoted

RalfJ (Jun 03 2019 at 16:28, on Zulip):

diagnostics aside, can we phrase "const checking" as "making sure an IsNotConst qualif is not set anywhere"? and then make IsNotPromotable basically a minor extension of that qualif if at all needed?

eddyb (Jun 03 2019 at 16:29, on Zulip):

but that doesn't mean anything :/

eddyb (Jun 03 2019 at 16:29, on Zulip):

qualifications are of "value"s. whatever you want to call them

RalfJ (Jun 03 2019 at 16:29, on Zulip):

RalfJ anyway, I don't think I managed to get across why top-level borrows are "leaked" to 'static instead of promoted

well I understand the checks are weaker. and it makes sense that they would be weaker than promotion un runtime fn. I dont know where that is implemented though.

eddyb (Jun 03 2019 at 16:29, on Zulip):

there are side-effects that don't produce a value

eddyb (Jun 03 2019 at 16:29, on Zulip):

like a -> ! const fn

eddyb (Jun 03 2019 at 16:30, on Zulip):

or, idk, maybe InlineAsm?

eddyb (Jun 03 2019 at 16:30, on Zulip):

or Drop, I guess

eddyb (Jun 03 2019 at 16:30, on Zulip):

@RalfJ no, I mean, why &f() allows more than Some(&f()) - both in constants

eddyb (Jun 03 2019 at 16:32, on Zulip):

anyway, you can disable that behavior by getting rid of this (and using the visit_expr from the "then" side of the if in all cases https://github.com/rust-lang/rust/blob/master/src/librustc/middle/region.rs#L1280-L1301

eddyb (Jun 03 2019 at 16:32, on Zulip):

and then do a crater run to see what breaks

eddyb (Jun 03 2019 at 16:32, on Zulip):

@RalfJ aside from back-compat, I really want &String::from("foo") to work one day

eddyb (Jun 03 2019 at 16:33, on Zulip):

and if you're not at the top-level there is no way that would ever be &'static (unless we add a way to say that String's Drop is "promotable away")

eddyb (Jun 03 2019 at 16:33, on Zulip):

but at the top-level we can use the let rules and say that the "enclosing scope" is 'static

eddyb (Jun 03 2019 at 16:34, on Zulip):

it's both backwards and forwards compatible

eddyb (Jun 03 2019 at 16:34, on Zulip):

that's why we did it that way

RalfJ (Jun 03 2019 at 16:37, on Zulip):

why would Some(&String::from("foo")) not work?

eddyb (Jun 03 2019 at 16:37, on Zulip):

because that already means something

RalfJ (Jun 03 2019 at 16:37, on Zulip):

the way we intern consts, anything that lives when the const is done computing can be interned

eddyb (Jun 03 2019 at 16:38, on Zulip):

like, f(&String::from("foo")) means "call f then drop the String"

RalfJ (Jun 03 2019 at 16:38, on Zulip):

hm

eddyb (Jun 03 2019 at 16:39, on Zulip):

really, look at how let x = &f(); rules work

eddyb (Jun 03 2019 at 16:39, on Zulip):

there are literally over 6 years of history here

RalfJ (Jun 03 2019 at 16:39, on Zulip):

okay when I ran into this I didnt think about Drop, I was more surprised that Some(&Cell::new(0)) does not work. there's no good reason at all for that IMO...

eddyb (Jun 03 2019 at 16:39, on Zulip):

you mean &AtomicUsize, right?

eddyb (Jun 03 2019 at 16:39, on Zulip):

cause that's not Sync

RalfJ (Jun 03 2019 at 16:40, on Zulip):

yeah (thought we dont always properly check Sync during promotion but that's another thing)

eddyb (Jun 03 2019 at 16:41, on Zulip):

but you agree f(&AtomicUsize::new(0)) shouldn't promote, yeah?

eddyb (Jun 03 2019 at 16:42, on Zulip):

(if it promotes, executing that code more than once may see a non-0 there)

eddyb (Jun 03 2019 at 16:42, on Zulip):

Some is a special-case of an ADT constructor that looks like a function call

eddyb (Jun 03 2019 at 16:42, on Zulip):

we can change things about it if we wanted to. it's a red herring

eddyb (Jun 03 2019 at 16:43, on Zulip):

actual functions are far more important

RalfJ (Jun 03 2019 at 16:43, on Zulip):

but you agree f(&AtomicUsize::new(0)) shouldn't promote, yeah?

I dont know what "promotion" means any more. ;) but I think static FOO = f(&AtomicUsize::new(0)) should work (and same for const)

RalfJ (Jun 03 2019 at 16:44, on Zulip):

well maybe not for const if the reference ends up in the result

RalfJ (Jun 03 2019 at 16:44, on Zulip):

interior mutability and const, yada yada (we'll hopefully soon have interning check for that)

RalfJ (Jun 03 2019 at 16:44, on Zulip):

but for static

eddyb (Jun 03 2019 at 16:44, on Zulip):

it's too tenuous IMO

eddyb (Jun 03 2019 at 16:45, on Zulip):

you'd agree x = f(&AtomicUsize::new(0)) in a loop even in a static should never be promoted, right?

RalfJ (Jun 03 2019 at 16:45, on Zulip):

...?

RalfJ (Jun 03 2019 at 16:45, on Zulip):

again I dont know what promotion means here

RalfJ (Jun 03 2019 at 16:45, on Zulip):

but I see no reason not to just execute that code using normal CTFE rules and then intern the result

RalfJ (Jun 03 2019 at 16:46, on Zulip):

of coruse it shouldnt "factor out" the AtomicUsize::new

eddyb (Jun 03 2019 at 16:46, on Zulip):

promotion means there is only one AtomicUsize and it gets reused instead of each iteration having its own

RalfJ (Jun 03 2019 at 16:46, on Zulip):

so I guess it'd be more like what you call "leaking"

eddyb (Jun 03 2019 at 16:46, on Zulip):

you can't "leak" in a loop

eddyb (Jun 03 2019 at 16:46, on Zulip):

you only have one local

RalfJ (Jun 03 2019 at 16:47, on Zulip):

so?

eddyb (Jun 03 2019 at 16:47, on Zulip):

there is something special about "this only happens once" that can make this work, but it's very much a special-case

RalfJ (Jun 03 2019 at 16:48, on Zulip):

hm. seems like my mental model for & in static is just off

eddyb (Jun 03 2019 at 16:49, on Zulip):

if you think of const and static RHSes as const fn functions that are used to initialize them, that would help a lot

RalfJ (Jun 03 2019 at 16:49, on Zulip):

my idea was to just compile to MIR and evaluate that, and because we dont drop the top "stack frame" (corresponding to the body of the item) we can just leak everything in that frame that we still need

eddyb (Jun 03 2019 at 16:49, on Zulip):

your mental model might be that of the pre-miri evaluator which did something very silly for &foo

RalfJ (Jun 03 2019 at 16:49, on Zulip):

if you think of const and static RHSes as const fn functions that are used to initialize them, that would help a lot

well but that does not explain why &foo() works

eddyb (Jun 03 2019 at 16:50, on Zulip):

@RalfJ so, what happens to per-loop iteration StorageLive...StorageDead?

eddyb (Jun 03 2019 at 16:50, on Zulip):

yes, &foo() is very much a special case

RalfJ (Jun 03 2019 at 16:50, on Zulip):

well sure if there's a StorageDead that cannot be used in the final value

eddyb (Jun 03 2019 at 16:51, on Zulip):

everything except the top-level special-case has Drops and StorageDeads!

RalfJ (Jun 03 2019 at 16:51, on Zulip):

yes, &foo() is very much a special case

in particular it's a special case not just for the analysis but for MIR generation as well, right?

eddyb (Jun 03 2019 at 16:51, on Zulip):

it's like a regulat function

eddyb (Jun 03 2019 at 16:51, on Zulip):

&foo() is a special-case only for MIR generation

eddyb (Jun 03 2019 at 16:51, on Zulip):

all of the behavior falls out of that

RalfJ (Jun 03 2019 at 16:51, on Zulip):

&foo() is a special-case only for MIR generation

well you linked me to some part where it's a special case of the analysis?

eddyb (Jun 03 2019 at 16:52, on Zulip):

what analysis? middle::region?

RalfJ (Jun 03 2019 at 16:52, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc/middle/region.rs#L1280-L1301
oh wait that's not in const_qualif

eddyb (Jun 03 2019 at 16:52, on Zulip):

lol

RalfJ (Jun 03 2019 at 16:52, on Zulip):

I assumed it had to be^^

eddyb (Jun 03 2019 at 16:52, on Zulip):

no, because it's not promotion

RalfJ (Jun 03 2019 at 16:52, on Zulip):

all of the behavior falls out of that

okay that helps a lot!

eddyb (Jun 03 2019 at 16:53, on Zulip):

you can just not run the promotion pass and top-level in const/static would still have the behavior of top-level in let

RalfJ (Jun 03 2019 at 16:54, on Zulip):

you can just not run the promotion pass and top-level in const/static would still have the behavior of top-level in let

that's not what I meant though, I meant & in statics could generally be "not promotion". but at least I understand now where the distinction is coming from.

eddyb (Jun 03 2019 at 16:54, on Zulip):

you could do that but then you can't have normal drops

eddyb (Jun 03 2019 at 16:54, on Zulip):

and I'd rather "doesn't get dropped" be the exception not the rule

eddyb (Jun 03 2019 at 16:55, on Zulip):

like, I want { let mut v = vec![]; ... v.iter().sum() } to very much behave like runtime code

eddyb (Jun 03 2019 at 16:55, on Zulip):

and drop the Vec

eddyb (Jun 03 2019 at 16:56, on Zulip):

you can't have that and "all & are &'static"

eddyb (Jun 03 2019 at 16:56, on Zulip):

not to mention conflicting borrows

RalfJ (Jun 03 2019 at 16:56, on Zulip):

well we clean up the CTFE context

RalfJ (Jun 03 2019 at 16:56, on Zulip):

so drop-or-no-drop is not observable

eddyb (Jun 03 2019 at 16:56, on Zulip):

that doesn't matter, it still needs to be checked

eddyb (Jun 03 2019 at 16:57, on Zulip):

we used to not have a const checking pass

eddyb (Jun 03 2019 at 16:57, on Zulip):

and just eval'd it which did whatever

eddyb (Jun 03 2019 at 16:57, on Zulip):

and we used to just not run the borrowck on const expressions

RalfJ (Jun 03 2019 at 16:58, on Zulip):

and that was bad?

RalfJ (Jun 03 2019 at 16:58, on Zulip):

I actually hope to equip CTFE/Miri with enough checks that that should still reject anything we have to reject

eddyb (Jun 03 2019 at 16:58, on Zulip):

yes, for the same reason we have a borrow-checker!

eddyb (Jun 03 2019 at 16:59, on Zulip):

and that's universal quantification

eddyb (Jun 03 2019 at 16:59, on Zulip):

on generics, on ambient state, and on dataflow paths

RalfJ (Jun 03 2019 at 16:59, on Zulip):

sure the checks would get delayed to instantiation time

RalfJ (Jun 03 2019 at 16:59, on Zulip):

which is probably not great

eddyb (Jun 03 2019 at 17:10, on Zulip):

it's not just not great, it's counter to almost everything Rust stands for!

RalfJ (Jun 03 2019 at 17:14, on Zulip):

uh... hyperbole etc?^^

eddyb (Jun 03 2019 at 17:15, on Zulip):

no, I mean, Rust is all about detecting statically detectable things ahead of time

eddyb (Jun 03 2019 at 17:16, on Zulip):

generics and traits in Rust are designed with polymorphic checks (mainly borrowck) in mind

eddyb (Jun 03 2019 at 17:16, on Zulip):

and also, being able to reason about runtime functions without knowing anything about their bodies

RalfJ (Jun 03 2019 at 17:45, on Zulip):

well the failure mode is less catastrophic when "uncheked" code gets executed in CTFE vs. "for real" (aka, no UB). but I do agree that it makes sense to have these analyses! I just usually understand analysis by first understanding the dynamic property they want to prove, and then relating things to that. but that's not how const-qualif is explained, documented or designed (from what I can see).

RalfJ (Jun 03 2019 at 17:50, on Zulip):

IOW I am just lamenting not having made enough progress on https://github.com/rust-rfcs/const-eval/issues/17 ^^

Dylan MacKenzie (Jun 03 2019 at 20:17, on Zulip):

Based on the ensuing discussion, I think the answer to my first question:

What are the odds that a "dedicated beginner" (me) could implement dataflow-based const qualification?

Is "no" for most definitions of "beginner" and "dedicated". I may be able to help with other things like writing tests and collecting knowledge about promotion into one place.

I would like to explain my mental model after watching oli-obk's compiler lecture on miri and reading some of the relevant bits of rustc over the weekend. Obviously I've probably overlooked some things.

There's two kinds of validation we want to run on const blocks (I know the definition of "valid" changes subtly depending on the exact context): a pre-monomorphization static check and a dynamic check that occurs as miri is running. The goal is to catch errors as early as possible in the analysis while providing good diagnostics. Some operations (e.g. calls to non-const functions or casting pointers to integers) can be forbidden statically with a flow-insensitive analysis (iterating over the basic blocks). However, some requirements of const blocks (e.g. whether a value with interior mutability appears in the return place) require a path-sensitive analysis.

Currently, control-flow is forbidden in CTFE, so there is only one possible execution path (unwind edges are ignored). We can enumerate this path simply by iterating over the basic blocks, meaning our naive analysis is actually path-sensitive for this simple class of CFGs. However, there are still some cases (associated consts) where errors can only be caught post-monomorphization.

We want to extend CTFE to arbitrarily complex CFGs but still catch as many errors statically as possible. We can do a conservative flow-sensitive analysis (dataflow) to find all places where a value definitely, maybe, or definitely does not meet a condition (e.g. HasMutInterior) (the tri-state comes from the combination two separate dataflow passes as in MaybeUninitializedPlaces and MaybeInitializedPlaces). I don't know whether you've decided to only allow things that can be statically proven to be const-safe, or if you want to run miri when something is only maybe const-safe. The first would disallow stuff like const x: Option<&Cell<u32>> = if true { None } else { Some(&Cell::new(0)) }, but the second would allow it (I'm still not 100% on the promotion rules, maybe this example is wrong).

eddyb (Jun 03 2019 at 20:21, on Zulip):

definitely only allow if static checks succeeded

Dylan MacKenzie (Jun 03 2019 at 20:25, on Zulip):

like, I want { let mut v = vec![]; ... v.iter().sum() } to very much behave like runtime code

@eddyb A somewhat unrelated question, could we short-circuit qualification with a simple type-based analysis for this example assuming the vec holds a primitive type?

Dylan MacKenzie (Jun 03 2019 at 20:27, on Zulip):

If we know type of the result of CTFE cannot possibly have interior mutability or a drop impl, we don't need to do any of the path-sensitive analysis right?

RalfJ (Jun 03 2019 at 20:43, on Zulip):

collecting knowledge about promotion into one place.

yes that would be great! that place would be https://github.com/rust-rfcs/const-eval/

RalfJ (Jun 03 2019 at 20:44, on Zulip):

I plan to incorporate some of what I learned today, but I am not sure when I will have time :/

RalfJ (Jun 03 2019 at 20:45, on Zulip):

However, there are still some cases (associated consts) where errors can only be caught post-monomorphization.

we use the types in those cases and remain conservative

RalfJ (Jun 03 2019 at 20:46, on Zulip):

so, the checks in miri should actually never fire as of right now. (once we allow transmutes and fn ptrs in const code this will change though.)

Mahmut Bulut (Jun 04 2019 at 15:58, on Zulip):

I tried using _ => {} and also return false. It is not visiting that point in qualify_const for a test case I am testing on. It is compiling anyway. I don't know where is the whitelist checks for intrinsics occurs.

Mahmut Bulut (Jun 04 2019 at 16:01, on Zulip):

For both cases I used this test

#![feature(core_intrinsics)]
const FOO: &usize = &std::intrinsics::size_of::<u32>();
Mahmut Bulut (Jun 04 2019 at 16:03, on Zulip):

I didn't understand where const context but not const fn checks are…

oli (Jun 04 2019 at 16:21, on Zulip):

I think this is just dead code that you removed. Not dead in the sense that it wasn't ever hit, but that its value is irrelevant, because the actual checks are elsewhere

Mahmut Bulut (Jun 04 2019 at 16:23, on Zulip):

My understanding is same.

oli (Jun 04 2019 at 16:24, on Zulip):

oh, I have an idea, gimme a sec

oli (Jun 04 2019 at 16:28, on Zulip):

hm, no, didn't pan out

RalfJ (Jun 04 2019 at 17:51, on Zulip):

For both cases I used this test

#![feature(core_intrinsics)]
const FOO: &usize = &std::intrinsics::size_of::<u32>();

that's not promotion -- top-level & is special

RalfJ (Jun 04 2019 at 17:51, on Zulip):

if you want to test promotion, do it with something like let x: &'static _ = &intrinsics::...

eddyb (Jun 05 2019 at 11:02, on Zulip):

@Dylan MacKenzie I really don't understand your question - how can the Vec element type matter? Vec has a Drop impl regardless of the element type

eddyb (Jun 05 2019 at 11:56, on Zulip):

@RalfJ I still disagree with this FWIW but I don't have the energy today to give you a good explanation https://github.com/rust-lang/rust/pull/61492/files#r289985850

eddyb (Jun 05 2019 at 11:57, on Zulip):

if you really want we could do the on-side-effect checking, and disqualify a local from promotion that way

eddyb (Jun 05 2019 at 11:58, on Zulip):

at which point it could be its own self-encapsulated check

RalfJ (Jun 05 2019 at 11:58, on Zulip):

RalfJ I still disagree with this FWIW but I don't have the energy today to give you a good explanation https://github.com/rust-lang/rust/pull/61492/files#r289985850

what's your argument beside the historical coincidence about the order in which they were added to the code base?

eddyb (Jun 05 2019 at 11:58, on Zulip):

but you'd still need to do the other checks

eddyb (Jun 05 2019 at 11:59, on Zulip):

@RalfJ I think it's a coincidence that they both care about drops

eddyb (Jun 05 2019 at 12:00, on Zulip):

the const-checking one would need to eventually start allowing const impl Drop

eddyb (Jun 05 2019 at 12:00, on Zulip):

while promotion in fn's can never do that, since the respective drop is at runtime

eddyb (Jun 05 2019 at 12:00, on Zulip):

so they'd eventually use two flags instead of one NeedsDrop

eddyb (Jun 05 2019 at 12:01, on Zulip):

like, NeedsNonConstDrop and NeedsDrop

eddyb (Jun 05 2019 at 12:02, on Zulip):

one is about not executing implicit non-const fn Drop::drop calls, the other is about preserving observable behavior

eddyb (Jun 05 2019 at 12:04, on Zulip):

it gets weirder when there are multiple uses of a promoted local, and only some of them are promoted

eddyb (Jun 05 2019 at 12:04, on Zulip):

@oli tried to make some changes in this area

eddyb (Jun 05 2019 at 12:04, on Zulip):

even if they both checked Drop terminators, the checks would be quite different

eddyb (Jun 05 2019 at 12:05, on Zulip):

anyway, after https://github.com/rust-lang/rust/issues/61539, I really need a break

oli (Jun 05 2019 at 12:37, on Zulip):

I did some cleanups, but I don't think I went through with everything I wanted

oli (Jun 05 2019 at 12:37, on Zulip):

I really need a todo list

RalfJ (Jun 05 2019 at 14:28, on Zulip):

while promotion in fn's can never do that, since the respective drop is at runtime

well, If the drop is const impl we could allow promoting (that is assuming we are willing to promote arbitrary const fn calls -- which is less of a problem for calls that don't return anything, like drop)

RalfJ (Jun 05 2019 at 14:30, on Zulip):

one is about not executing implicit non-const fn Drop::drop calls, the other is about preserving observable behavior

but in that case then, wouldnt we want to ignore some "qualif"s in the promotion part because they are only relevant for const checking?

oli (Jun 05 2019 at 14:46, on Zulip):

well, If the drop is const impl we could allow promoting (that is assuming we are willing to promote arbitrary const fn calls -- which is less of a problem for calls that don't return anything, like drop)

not a good idea imo. User code will "randomly" (from the perspective of someone who doesn't know about promotion) start to stop running destructors

RalfJ (Jun 05 2019 at 15:59, on Zulip):

it won't stop running them, it'll run them at CTFE time

Dylan MacKenzie (Jun 05 2019 at 16:54, on Zulip):

@eddyb my point was merely that we could skip running dataflow as an optimization if the result type of the const block cannot have interior mutability behind a reference and is not needs drop.

eddyb (Jun 18 2019 at 16:46, on Zulip):

@Dylan MacKenzie IMO that's both a premature optimization and dangerous even if it's sound now (which I don't know if it is)

eddyb (Jun 18 2019 at 16:47, on Zulip):

because if we change some things in the "unoptimized" version and the condition for using the "optimized" version don't take that into account, they can be out of sync

eddyb (Jun 18 2019 at 16:47, on Zulip):

@Dylan MacKenzie also I don't understand why "reaching definition" might be needed

eddyb (Jun 18 2019 at 16:48, on Zulip):

(from the other thread)

eddyb (Jun 18 2019 at 16:48, on Zulip):

I feel bad because it seems like I'm just confusing everyone with something that should be otherwise not that complicated :(

Dylan MacKenzie (Jun 18 2019 at 16:50, on Zulip):

Unfortunately I have a pretty low threshold for confusion :) Reaching definitions lets us create a use-def chain, which we can use to enumerate all possible rvalues that could get assigned to the return place

eddyb (Jun 18 2019 at 16:51, on Zulip):

@Dylan MacKenzie that's implying a full rewrite of the pass though?

eddyb (Jun 18 2019 at 16:51, on Zulip):

as opposed to just computing the same bits as today, but using the dataflow framework

eddyb (Jun 18 2019 at 16:51, on Zulip):

which handles control-flow unlike the limitation of linear control-flow today

eddyb (Jun 18 2019 at 16:52, on Zulip):

not to mention it's hard to encode everything touching a local in an use-def chain

eddyb (Jun 18 2019 at 16:53, on Zulip):

we're reaching VSDG-level of complexity

Dylan MacKenzie (Jun 18 2019 at 16:54, on Zulip):

I think it would yes.

eddyb (Jun 18 2019 at 16:54, on Zulip):

also, what would you do for loops? forward dataflow can track properties of locals modified by a loop, but "pulling" the definition from an Use-Def chain would need to handle the fixpoint of loops which is where most of the complexity of VSDG comes from

eddyb (Jun 18 2019 at 16:54, on Zulip):

@Dylan MacKenzie okay but I never suggested any of this?

Dylan MacKenzie (Jun 18 2019 at 16:54, on Zulip):

I guess I was confused when you mentioned the existing dataflow framework above

eddyb (Jun 18 2019 at 16:55, on Zulip):

this is why I think that maybe I should do it myself because I don't seem to get across just how simple this is supposed to be

Dylan MacKenzie (Jun 18 2019 at 16:55, on Zulip):

I do not have a problem with that :)

eddyb (Jun 18 2019 at 16:55, on Zulip):

@Dylan MacKenzie "dataflow analysis" aka (forward) propagation of 1 bit of information per local variable, not "dataflow graph"

eddyb (Jun 18 2019 at 16:56, on Zulip):

yeah the only issue is that I keep accumulating TODO items faster than I can dispatch them :(

Dylan MacKenzie (Jun 18 2019 at 16:57, on Zulip):

So, reaching defs keeps a bit for each assignment in the program, not for each local

Dylan MacKenzie (Jun 18 2019 at 16:57, on Zulip):

(not sure if I'm repeating stuff you already know)

eddyb (Jun 18 2019 at 16:57, on Zulip):

yeah I know. I just have no idea why you'd switch to something incredibly more difficult

eddyb (Jun 18 2019 at 16:57, on Zulip):

(unless you don't want to handle loops?)

Dylan MacKenzie (Jun 18 2019 at 16:58, on Zulip):

Why does reaching definitions not handle loops?

eddyb (Jun 18 2019 at 16:58, on Zulip):

(even then it's more work unless you reconstruct a DAG version of the MIR, more VSDG-like. that would be useful for unification of "type-level const expressions" in the context of const generics :P)

Dylan MacKenzie (Jun 18 2019 at 16:58, on Zulip):

back-edges in the CFG are handled fine I believe

eddyb (Jun 18 2019 at 16:59, on Zulip):

@Dylan MacKenzie it's not really the "reaching definitions" but rather how you plan to use them

eddyb (Jun 18 2019 at 16:59, on Zulip):

to "pull" the definition of a value

eddyb (Jun 18 2019 at 16:59, on Zulip):

this is bad because in a loop, a value can depend on things in previous iterations

eddyb (Jun 18 2019 at 17:00, on Zulip):

forward dataflow can saturate a property, and we know that's sound (especially since it runs in the same direction the code would run at runtime :P), but if you do it by "pulling" things, you run into soundness concerns of cyclic analyses

eddyb (Jun 18 2019 at 17:01, on Zulip):

we'd have this problem if we wanted to, say, promote a value that was constructed by a loop

eddyb (Jun 18 2019 at 17:01, on Zulip):

we'd have to represent the cyclical control-flow around the assignments, too

Dylan MacKenzie (Jun 18 2019 at 17:01, on Zulip):

Rephrasing, I'll get the set of definitions which are live, but those definitions may depend on earlier values

Dylan MacKenzie (Jun 18 2019 at 17:01, on Zulip):

in the loop iteration

Dylan MacKenzie (Jun 18 2019 at 17:02, on Zulip):

?

eddyb (Jun 18 2019 at 17:02, on Zulip):

yes. and you need a fixpoint algorithm

eddyb (Jun 18 2019 at 17:02, on Zulip):

and I'm not sure I can think of one

eddyb (Jun 18 2019 at 17:02, on Zulip):

not to mention this is harder than doing the (straight)forward dataflow the rest of the compiler uses

eddyb (Jun 18 2019 at 17:04, on Zulip):

@Dylan MacKenzie so, today, we have one bitvec per Qualif implementor and we mutate them on every Assign/Call. and this works only for linear control-flow. if we switched to using the dataflow framework, we'd have one dataflow result for each Qualif implementor, that we could advance, and it gives us access to one bitvec each

eddyb (Jun 18 2019 at 17:04, on Zulip):

so for linear control-flow we'd just observe the same values in those bitvecs

eddyb (Jun 18 2019 at 17:04, on Zulip):

but more interesting control-flow would also "just work"

eddyb (Jun 18 2019 at 17:05, on Zulip):

and most of the code won't have to change at all to account for this

Dylan MacKenzie (Jun 18 2019 at 17:05, on Zulip):

Okay, so you were thinking of something like the strategy that I mentioned at the top of this (one bit per local representing qualif-state)

eddyb (Jun 18 2019 at 17:05, on Zulip):

yeah which is literally what we do today

Dylan MacKenzie (Jun 18 2019 at 17:05, on Zulip):

but I wasn't sure how to fit this into the "gen-kill" framework

eddyb (Jun 18 2019 at 17:06, on Zulip):

those are silly names for set/unset

eddyb (Jun 18 2019 at 17:06, on Zulip):

or I should say, "historical names"

Dylan MacKenzie (Jun 18 2019 at 17:06, on Zulip):

since the transfer function depends on whether another bit in the dataflow is set

Dylan MacKenzie (Jun 18 2019 at 17:06, on Zulip):

which might change as we're iterating

eddyb (Jun 18 2019 at 17:07, on Zulip):

what do you mean?

eddyb (Jun 18 2019 at 17:07, on Zulip):

oh

eddyb (Jun 18 2019 at 17:07, on Zulip):

OH

eddyb (Jun 18 2019 at 17:07, on Zulip):

/me facepalms

Dylan MacKenzie (Jun 18 2019 at 17:07, on Zulip):

So if I have _1 = _2, I only want to gen _1 if the qualif bit for _2 is set

eddyb (Jun 18 2019 at 17:08, on Zulip):

I'm sorry it took me so long to find out that this is was your issue :(

Dylan MacKenzie (Jun 18 2019 at 17:08, on Zulip):

that's okay, I'm maybe a bit too shy

eddyb (Jun 18 2019 at 17:09, on Zulip):

I kept wondering what am I missing, but this makes sense

Dylan MacKenzie (Jun 18 2019 at 17:09, on Zulip):

I can do this in a generic dataflow algorithm, but not in the BitDenotation-based one.

eddyb (Jun 18 2019 at 17:09, on Zulip):

I was so intent of removing dependencies between the various bits... that I forgot how dataflow analysis works

eddyb (Jun 18 2019 at 17:12, on Zulip):

@Dylan MacKenzie okay yeah it can still be bit-based, and it should reach fixpoint, you just can't compute the effects in isolation...

eddyb (Jun 18 2019 at 17:12, on Zulip):

I still think "pull" would be too much of a change :(

Dylan MacKenzie (Jun 18 2019 at 17:14, on Zulip):

"pull"?

eddyb (Jun 18 2019 at 17:14, on Zulip):

the thing you were talking about, to walk to the definition

eddyb (Jun 18 2019 at 17:14, on Zulip):

you're "pulling" the definition. whereas right now we're "pushing" properties to the use

eddyb (Jun 18 2019 at 17:16, on Zulip):

@Dylan MacKenzie so uhhh we technically could add bits[i] = bits[j] / bits[i] |= bits[j] on top of the bits[i] = constant that gen/kill represent, to the existing dataflow framework, but @pnkfelix might hate us :D

eddyb (Jun 18 2019 at 17:17, on Zulip):

we would have to change the Qualif methods to do something quite different than just -> bool, but it shouldn't be that bad (and we could do it today, with the linear controlflow restriction, before messing with the dataflow framework)

Dylan MacKenzie (Jun 18 2019 at 17:18, on Zulip):

@eddyb A custom dataflow algorithm isn't that hard, there's already one here for liveness (that I think could have been implemented on top of `BitDenotation?)

Dylan MacKenzie (Jun 18 2019 at 17:18, on Zulip):

https://github.com/rust-lang/rust/blob/9606f6fa64926a84d82e3c62dbdc57f5c10f756d/src/librustc_mir/util/liveness.rs#L59

eddyb (Jun 18 2019 at 17:19, on Zulip):

yeah I know I'd just hate myself and maybe risk making it unsound

eddyb (Jun 18 2019 at 17:19, on Zulip):

I think the liveness one is separate because liveness is backwards dataflow IIRC

eddyb (Jun 18 2019 at 17:20, on Zulip):

I have a branch on which I wrote a thing I called "eventflow" that was bidirectional and was focused on tracking "something may have happened in the past/future" as opposed to "stateful properties"

eddyb (Jun 18 2019 at 17:21, on Zulip):

(like, it was saturating, gen-only so to speak, whereas liveness uses kill)

Dylan MacKenzie (Jun 18 2019 at 17:21, on Zulip):

ah, that's right

eddyb (Jun 18 2019 at 17:23, on Zulip):

(https://github.com/eddyb/rust/blob/copy-elision/src/librustc_mir/analysis/eventflow.rs)

eddyb (Jun 18 2019 at 17:26, on Zulip):

so, I'm a bit conflicted now. I certainly think that some sort of sparse array of "where should the value for this output bit come from? unchanged, constant (gen/kill), or some other input bit?", generated the same way gen/kill sets are today, could be faster to compute with than re-doing all the per-statement logic on every propagation

eddyb (Jun 18 2019 at 17:27, on Zulip):

but I'm not sure what we should be doing. at least I'm still more confident in forward dataflow for reasoning about the state of locals at some point in the control-flow

Dylan MacKenzie (Jun 18 2019 at 17:32, on Zulip):

So we could keep the "gen/kill" set, then add an ancillary "steal" sparse map per CFG-edge

Dylan MacKenzie (Jun 18 2019 at 17:35, on Zulip):

The normal dataflow analyses would all have empty "steal" maps so I don't think it would slow things down too much in the normal case

Dylan MacKenzie (Jun 18 2019 at 17:51, on Zulip):

There's one other thing that I found confusing and that may be applicable: the current framework lets you inspect the initial state of each block (and optionally the initial state at each statement) when defining your transfer function. I'm not 100% sure why this is useful. I think it helps the rate of convergence since if we have_1 = Some(Cell::new()); _2 = _1;, we could immediately gen the bit for _2 instead of "steal"-ing from _1

Dylan MacKenzie (Jun 18 2019 at 17:54, on Zulip):

The only place that parameter is actually used is here: https://github.com/rust-lang/rust/blob/44fb88d25282d9362774536965f2455f677734f3/src/librustc_mir/dataflow/impls/borrows.rs#L207

eddyb (Jun 18 2019 at 18:05, on Zulip):

wait

eddyb (Jun 18 2019 at 18:05, on Zulip):

let me actually check the propagation :|

eddyb (Jun 18 2019 at 18:07, on Zulip):

@Dylan MacKenzie it's intra-block only <https://github.com/rust-lang/rust/blob/master/src/librustc_mir/dataflow/mod.rs#L574-L597>

eddyb (Jun 18 2019 at 18:08, on Zulip):

which makes sense, the gen/kill sets are only computed once per block

Dylan MacKenzie (Jun 18 2019 at 18:09, on Zulip):

So that flag changes whether you can see the effects of previous statements in the block applied to the initial entry set when defining transfer functions for later statements

Dylan MacKenzie (Jun 18 2019 at 18:11, on Zulip):

Otherwise every statement in the block sees only the effect from start_block_effect

eddyb (Jun 18 2019 at 18:12, on Zulip):

which is either all-ones or all-zeros, IIRC

Dylan MacKenzie (Jun 18 2019 at 18:13, on Zulip):

No, you can have custom start_block_effects. Like marking all parameters to a function as DefinitelyInitialized

eddyb (Jun 18 2019 at 18:13, on Zulip):

hmm but that's invalid except for the start block?

eddyb (Jun 18 2019 at 18:13, on Zulip):

everything else should assume the most conservative thing possible

eddyb (Jun 18 2019 at 18:14, on Zulip):

(e.g. you can move out of those argument locals)

Dylan MacKenzie (Jun 18 2019 at 18:14, on Zulip):

https://github.com/rust-lang/rust/blob/44fb88d25282d9362774536965f2455f677734f3/src/librustc_mir/dataflow/mod.rs#L201

Dylan MacKenzie (Jun 18 2019 at 18:15, on Zulip):

Right, so on_entry will have the value from start_block_effect

Dylan MacKenzie (Jun 18 2019 at 18:16, on Zulip):

while defining transfer functions in all basic blocks

Dylan MacKenzie (Jun 18 2019 at 18:16, on Zulip):

It's just not useful there

Dylan MacKenzie (Jun 18 2019 at 18:18, on Zulip):

Oh wait. I'm wrong, it just has those effects in theSTART_BLOCK

eddyb (Jun 18 2019 at 18:18, on Zulip):

phew

Dylan MacKenzie (Jun 18 2019 at 18:18, on Zulip):

All other blocks will have see the entry set be either all zeros or all ones like you said

Dylan MacKenzie (Jun 20 2019 at 15:59, on Zulip):

@eddyb, I did another readthrough of qualify_consts.rs in my free time yesterday. I think I want to make BitDenotation variant over transfer functions, so if/when #61787 lands, GenKillSet will become the canonical implementer but const-qualification will have one that adds the ability to copy bits from other locals (this operation will be pretty slow if there's a lot of assignments).

Dylan MacKenzie (Jun 20 2019 at 16:00, on Zulip):

I then want to split qualify_consts into two logical passes, the first checks for const correctness (e.g. calling non-const fns, accessing mutable statics, etc.). The second is where we run dataflow.

Dylan MacKenzie (Jun 20 2019 at 16:03, on Zulip):

@eddyb

Dylan MacKenzie (Jun 20 2019 at 16:03, on Zulip):

That is :)

eddyb (Jun 20 2019 at 16:04, on Zulip):

so, the way I imagined this would go

Dylan MacKenzie (Jun 20 2019 at 16:05, on Zulip):

(deleted)

eddyb (Jun 20 2019 at 16:05, on Zulip):

is that we would keep the current pass

eddyb (Jun 20 2019 at 16:05, on Zulip):

but run the dataflow before it, such that we can seek the results at any Location we need

eddyb (Jun 20 2019 at 16:06, on Zulip):

so most of the code would remain unchanged (risking little to no bugs :P)

Dylan MacKenzie (Jun 20 2019 at 16:07, on Zulip):

My next thought was "I'm worried about creating a massive, unreviewable diff since the control flow in qualify_consts is pretty hard to separate out" :)

eddyb (Jun 20 2019 at 16:07, on Zulip):

like, the way things are set up right now is that very little of the code actually writes the bitvecs, most of it reads them

Dylan MacKenzie (Jun 20 2019 at 16:08, on Zulip):

That makes things easier, so ConstCx will also store dataflow results, and in_local will fetch the results of dataflow analysis

eddyb (Jun 20 2019 at 16:08, on Zulip):

and if we have the dataflow results we should be able to seek them as we do the const-checking

eddyb (Jun 20 2019 at 16:08, on Zulip):

yupp :D

Dylan MacKenzie (Jun 20 2019 at 16:12, on Zulip):

I'm planning to work on this in the evenings (I'm UTC-8:00, so in 9 hours or so), so I'll keep doing progress reports and ask questions if I get stuck

eddyb (Jun 20 2019 at 16:13, on Zulip):

I haven't considered abstracting dataflow before at the level of "ahead-of-time computation for each whole block" but I really like the idea!

eddyb (Jun 20 2019 at 16:13, on Zulip):

@Dylan MacKenzie have you had a chance to discuss this with @pnkfelix or @nikomatsakis yet?

eddyb (Jun 20 2019 at 16:14, on Zulip):

if modular enough NLL borrowck might be able to use it to enhance some aspects of, say, error reporting (cc @Matthew Jasper I guess?)

Dylan MacKenzie (Jun 20 2019 at 16:15, on Zulip):

No, I think niko is unavailable, but i can try to have a sync talk with @pnkfelix if he's not too busy.

Dylan MacKenzie (Jun 26 2019 at 17:16, on Zulip):

Hey @eddyb, I got a bit side-tracked while trying to customize dataflow analysis (see #62010, #62062, #61787). I'm returning to const-qualification now. However, I think there's a problem with adding a general-purpose assign operator (bit[i] = bit[j]) to transfer functions. The reason the "gen-kill" paradigm is useful is that it composes nicely, allowing us to build up a single pair of bit-vectors which apply the transfer functions of all statements in the block. As soon as we add and assign operator, however, we can no longer coalesce transfer functions in this way, since the order of assignments relative to gens and kills matters. For example, I believe the following cannot be expressed by a single gen/kill set and a single, ordered list of assignments:

_2 = _1
_1 = false
_3 = _2
_2 = false
_1 = _2

This means that such a transfer function can't be nicely added to the existing dataflow execution engine. We would need a separate, slower one which goes statement-by-statement.

Dylan MacKenzie (Jun 26 2019 at 17:25, on Zulip):

I considered whether transfer functions could be split into three components, assign before gen-kill, gen-kill, and assign after gen-kill, but I think that's not possible for every basic block.

Dylan MacKenzie (Jun 26 2019 at 17:26, on Zulip):

Can't come up with a good example though

Dylan MacKenzie (Jun 26 2019 at 17:39, on Zulip):

Hmm, maybe I'm wrong

Dylan MacKenzie (Jun 26 2019 at 17:42, on Zulip):

If the RHS has been gen-ed or kill-ed earlier in the block, instead of assigning, you just gen/kill it. And assignments are always applied before gen and kill.

Dylan MacKenzie (Jun 26 2019 at 19:55, on Zulip):

The other issue here is that the dataflow at location APIs rely on transfer functions being idempotent (trans(trans(v)) == trans(v)). A transfer function like _1 = _2; _2 = _3 does not meet this assumption.

eddyb (Jun 27 2019 at 12:17, on Zulip):

sooo there's a less efficient but still per-block view of this: per-element (e.g. per-local) Unchanged | Clear | Set | CopyFrom(OtherElement)

eddyb (Jun 27 2019 at 12:17, on Zulip):

kill/gen is a 2-bit encoding of Unchanged | Clear | Set, split into two bit vectors for efficiency reasons

eddyb (Jun 27 2019 at 12:18, on Zulip):

if you want to use kill/gen bitvecs + another vec for "copy from", you'll have to clear the 2 you're not using, for any action

eddyb (Jun 27 2019 at 12:19, on Zulip):

gen/kill are already exclusive. this "two bitvecs and one regular vec" would still have to be exclusive

eddyb (Jun 27 2019 at 12:20, on Zulip):

although, come to think of it, A | B | C | ... | Z(Local) is the same size as Local, thanks to newtype_index! and @oli's attribute hacks for validity ranges

Dylan MacKenzie (Jun 27 2019 at 17:42, on Zulip):

How do you encode the relative order of assignments in that scheme (e.g. _1 = _2; _2 = _3)? I think you need a collection that maintains the order of CopyFrom(OtherElement) within a block.

Dylan MacKenzie (Jun 27 2019 at 17:44, on Zulip):

Does this make you reconsider the reaching definitions formulation of const-qualification at all? I'm starting to think that building a use-def chain would be simpler than supporting CopyFrom

eddyb (Jul 02 2019 at 08:21, on Zulip):

@Dylan MacKenzie sorry, I didn't see that question. I would assume you would just do the SSA thing?

eddyb (Jul 02 2019 at 08:22, on Zulip):

like, you would end up with an state array for which state[1] == CopyFrom(_2) and state[2] == CopyFrom(_3)

eddyb (Jul 02 2019 at 08:24, on Zulip):

but you can only get CopyFrom(x) if state[x] is Unchanged at that point in the block when CopyFrom would've been emitted

eddyb (Jul 02 2019 at 08:25, on Zulip):

that is, CopyFrom uses the on-entry value

eddyb (Jul 02 2019 at 08:25, on Zulip):

so you could imagine it in another way: AtBlockEntry(Local) | Clear | Set

eddyb (Jul 02 2019 at 08:26, on Zulip):

where everything is initialized to AtBlockEntry(Local)

eddyb (Jul 02 2019 at 08:26, on Zulip):

and on Assign(x, y) you literally just go state[x] = state[y], copying the intra-block state

eddyb (Jul 02 2019 at 08:31, on Zulip):

which might be AtBlockEntry(y) but could be anything else

eddyb (Jul 02 2019 at 08:43, on Zulip):

@Dylan MacKenzie wait, what about something like x = (y, z); - that needs to | two other locals' bits together, so you do need a list of |= operations instead...

eddyb (Jul 02 2019 at 08:47, on Zulip):

this is turning into an SSA-like DAG of operations isn't it :P

Dylan MacKenzie (Jul 02 2019 at 16:52, on Zulip):

Hmm, yes. While we could solve that particular case by tracking individual move paths (e.g. a.b.c) and doing deaggregation, we can't handle successive assignments to an array index (e.g. a[x]) without |. At that point I think there's no way to efficiently concatenate statement transfer functions; you have to step through each basic block statement-by-statement.

As the transfer function of the per-local approach becomes more expensive, I become more convinced that a reaching definitions formulation would be comparable in performance. While you have to store a bit for each definition of a local (instead of each local), you only have to run a single dataflow analysis for all 4 qualifications.

The big question is how to handle cycles in the use-def chain while propagating qualifications, but I believe a simple graph traversal is sufficient. To start, each definition of a local is unconditionally qualified, unconditionally not qualified, or qualified if any one of a set of locals may be qualified. When the last case happens, we add all definitions of the aforementioned set of locals that reach the current point to a work queue, and recursively process them until we find one that is definitely qualified or we run out of definitions in the queue. If we encounter a cyclical dependency (as would occur in loop { x = x + 1}), that definition will already be on the work queue. We should be able to simply skip these tautologies (qualif(x = x + 1) -> qualif(x = x+ 1)), since the only way the condition can be qualified is if one of the other definitions (in this case of x) on the work queue is qualified.

I have been continuing work on a reaching definitions pass a bit (I extended rustc_peek and wrote a test, and started on a use-def chain). I'm worried about the memory overhead of storing all reaching definitions for every use of a local in the body. It would be nice to compute these on-demand, but accessing intra-block dataflow state requires processing all previous statements in the block, so without processing them in order at the beginning, we would be quadratic in the worst case.

Dylan MacKenzie (Jul 02 2019 at 16:54, on Zulip):

Sorry for the brain dump @eddyb :silence:

eddyb (Jul 02 2019 at 17:19, on Zulip):

given what I've seen regarding VSDG, I'm really scared of relying on "reaching definitions" in an analysis

eddyb (Jul 02 2019 at 17:20, on Zulip):

at most I would create some sort of data structure to help do forward dataflow, with a restriction like single-dominating-definition

eddyb (Jul 02 2019 at 17:23, on Zulip):

you can even build the equivalent of "decision trees" except without conditions. so they're more like "merge trees"

eddyb (Jul 02 2019 at 17:23, on Zulip):

as long as you don't touch backedges

eddyb (Jul 02 2019 at 17:24, on Zulip):

and then you propagate along those merge edges until fixpoint, like the current dataflow algorithm

eddyb (Jul 02 2019 at 17:25, on Zulip):

this is what I get for assuming one could just slap dataflow onto qualify_consts....

Dylan MacKenzie (Jul 02 2019 at 17:50, on Zulip):

VSDG is value state dependence graph?

Christian Poveda (Jul 02 2019 at 18:33, on Zulip):

Yes it is

eddyb (Jul 04 2019 at 06:45, on Zulip):

yeah

eddyb (Jul 04 2019 at 06:46, on Zulip):

the soundness of converting a CFG to VSDG is pretty much all in the loops, you have to get them right - if you have a DAG (so no backedges, no looping) it's almost trivial to convert the branches at the splits to selects at the merges

eddyb (Jul 04 2019 at 06:47, on Zulip):

(and an entire function would be SESE, or at least very easy to turn SEME into SESE)

eddyb (Jul 04 2019 at 07:54, on Zulip):

@Dylan MacKenzie frankly, the sensible thing to do would probably be to convert each block into something like "reaching definition", but without needing the existing dataflow analysis to do (I guess "still-live-on-block-exit writes to this local" is more or less what I'd want there), and then do dataflow-like propagation of bits, but without caching the results or anything, just keep redoing each block's work (based on those "known writes") until fixpoint

eddyb (Jul 04 2019 at 07:55, on Zulip):

we could reuse some of the dataflow framework for "propagate until fixpoint" but the transfer function would be more involved than (bits - kill) | gen

Dylan MacKenzie (Jul 10 2019 at 18:43, on Zulip):

@eddyb I wrote up a summary of my thoughts on this on #53819. I still don't understand your concerns regarding VSDGs since I don't have the theoretical background. Basically, I think that the custom dataflow analysis which propagates qualif bits each iteration will be as sound/precise as one that computes the use-def chain ahead of time and then propagates qualification bits on that.

Dylan MacKenzie (Jul 10 2019 at 18:43, on Zulip):

There's some justification for this idea at the end of my post, but I'm not sure how convincing you'll find it :)

eddyb (Jul 11 2019 at 06:23, on Zulip):

I don't really understand how you even plan to use a def-use chain

eddyb (Jul 11 2019 at 06:24, on Zulip):

if you want to handle mutation

eddyb (Jul 11 2019 at 06:24, on Zulip):

anyway, I don't see how you can be correct around loops without a lot of painful work

eddyb (Jul 11 2019 at 06:25, on Zulip):

whereas dataflow sidesteps the problem by interpreting the loop

Dylan MacKenzie (Jul 11 2019 at 06:31, on Zulip):

It feels like we're talking past each other a bit. Do you think you could provide a more concrete example of how this falls apart around loops? A use-def chain is built via a dataflow analysis, so I'm not really sure what you mean with your last statement

Dylan MacKenzie (Jul 11 2019 at 06:32, on Zulip):

I guess that would be hard if you don't understand how I plan to use a use-def chain in the first place

eddyb (Jul 11 2019 at 06:33, on Zulip):

are you mapping locations of uses to definitions?

Dylan MacKenzie (Jul 11 2019 at 06:33, on Zulip):

Yes

eddyb (Jul 11 2019 at 06:33, on Zulip):

because that's already an improvement, and not really a def-use chain

Dylan MacKenzie (Jul 11 2019 at 06:33, on Zulip):

Uses of each local to definitions of that lovey

eddyb (Jul 11 2019 at 06:33, on Zulip):

that's "reaching definition"

Dylan MacKenzie (Jul 11 2019 at 06:33, on Zulip):

Local

eddyb (Jul 11 2019 at 06:34, on Zulip):

but not all definitions, surely? only the reaching ones?

Dylan MacKenzie (Jul 11 2019 at 06:34, on Zulip):

Correct

eddyb (Jul 11 2019 at 06:34, on Zulip):

yeah I think we should say "reaching defs" not "def-use chain"

eddyb (Jul 11 2019 at 06:35, on Zulip):

the latter makes more sense when you have one def and many uses and you can use an intrusive linked list or something

eddyb (Jul 11 2019 at 06:35, on Zulip):

like, for SSA

eddyb (Jul 11 2019 at 06:36, on Zulip):

it's useful for visiting all uses of a def, because use -> def in that sort if situation is already provided

eddyb (Jul 11 2019 at 06:36, on Zulip):

okay so what do you do for local.field = ...;?

eddyb (Jul 11 2019 at 06:37, on Zulip):

does it count as a "def"? I would probably switch to "reaching writes"

Dylan MacKenzie (Jul 11 2019 at 06:37, on Zulip):

My impression was that a UD chain is a data structure which one can build after doing a reaching definitions analysis

eddyb (Jul 11 2019 at 06:38, on Zulip):

it doesn't make sense when you have mutations though

Dylan MacKenzie (Jul 11 2019 at 06:38, on Zulip):

in SSA, you get a use-def chain for free (since each variable is only defined once)

eddyb (Jul 11 2019 at 06:38, on Zulip):

not exactly, you get use->def for free, but not def->uses

eddyb (Jul 11 2019 at 06:39, on Zulip):

anyway, regardless of terminology (which I'm not 100% sure of either, I'd defer to @nikomatsakis)

Dylan MacKenzie (Jul 11 2019 at 06:39, on Zulip):

Okay so when you have mutations, there are multiple points where any one local can be "defined"

eddyb (Jul 11 2019 at 06:39, on Zulip):

do you track both partial mutations and merging branches?

Dylan MacKenzie (Jul 11 2019 at 06:40, on Zulip):

(I'm using defined to mean: "any part of this variable can be written")

eddyb (Jul 11 2019 at 06:40, on Zulip):

yeah that's prone to confusion, I'd just call that a write or mutation

Dylan MacKenzie (Jul 11 2019 at 06:40, on Zulip):

Yes. Partial mutations are tracked. Any assignment to a projection of a local is a "partial definition"

eddyb (Jul 11 2019 at 06:41, on Zulip):

e.g. if c { x = a; } else { x = b; } x.f = y; use(x);

Dylan MacKenzie (Jul 11 2019 at 06:41, on Zulip):

re: merging branches, are you refering to the join operator for reaching defs dataflow ?

Dylan MacKenzie (Jul 11 2019 at 06:42, on Zulip):

use(x) -> ["x =a", "x =b", "x.f=y"]

Dylan MacKenzie (Jul 11 2019 at 06:42, on Zulip):

Would be the use-def chain that the current analysis builds

eddyb (Jul 11 2019 at 06:43, on Zulip):

okay so.....

Dylan MacKenzie (Jul 11 2019 at 06:43, on Zulip):

but if c { x.f = a } else {x.f = b} x = y; use(x); would result in ["x = y"]

eddyb (Jul 11 2019 at 06:43, on Zulip):

how do you evaluate that?

eddyb (Jul 11 2019 at 06:43, on Zulip):

that's basically my overall question: you're building the equivalent of a VSDG fragment, except less principled

Dylan MacKenzie (Jul 11 2019 at 06:44, on Zulip):

Reaching definitions analysis is what's doing the work here

eddyb (Jul 11 2019 at 06:44, on Zulip):

no, I mean, not how you compute it

eddyb (Jul 11 2019 at 06:45, on Zulip):

how do you evaluate a property of x on that chain

Dylan MacKenzie (Jul 11 2019 at 06:45, on Zulip):

Ah, you need to look at the rvalue of each assignment

eddyb (Jul 11 2019 at 06:46, on Zulip):

but in what order/

eddyb (Jul 11 2019 at 06:46, on Zulip):

in pseudo-VSDG it would be something like select(c, { x = a; }, { x = b; }) >>= { x.f = y; }

Dylan MacKenzie (Jul 11 2019 at 06:47, on Zulip):

What do you mean by "order"?

eddyb (Jul 11 2019 at 06:47, on Zulip):

how do you combine a, b and y into x?

eddyb (Jul 11 2019 at 06:47, on Zulip):

or more high-level: x = Foo { f: y, ..select(c, a, b) }

eddyb (Jul 11 2019 at 06:47, on Zulip):

γ is basically select or the C ?: operator

eddyb (Jul 11 2019 at 06:48, on Zulip):

okay it's actually indistinguishable from y, dammit

eddyb (Jul 11 2019 at 06:48, on Zulip):

I'll replace it with select

Dylan MacKenzie (Jul 11 2019 at 06:48, on Zulip):

the union of the qualification bits for each of a, b and y

eddyb (Jul 11 2019 at 06:49, on Zulip):

so you just union every write together?

Dylan MacKenzie (Jul 11 2019 at 06:49, on Zulip):

(If c is the condition, it's not considered)

Dylan MacKenzie (Jul 11 2019 at 06:49, on Zulip):

Every write which could possible reach that point in the program yes

eddyb (Jul 11 2019 at 06:49, on Zulip):

no matter the order.... hmmmmm

Dylan MacKenzie (Jul 11 2019 at 06:50, on Zulip):

I'm really confused why the order matters

eddyb (Jul 11 2019 at 06:50, on Zulip):

well, you're side-stepping it partially by mapping locations of uses

eddyb (Jul 11 2019 at 06:50, on Zulip):

so you at least have correct reaching writes for any use at the point of any other write

Dylan MacKenzie (Jul 11 2019 at 06:51, on Zulip):

If x has multiple reaching definitions, if any one of them is maybe qualified, we need to assume that x is maybe qualifed

eddyb (Jul 11 2019 at 06:51, on Zulip):

for const qualif, sure

eddyb (Jul 11 2019 at 06:53, on Zulip):

so this works specifically because the writes are order-independent

eddyb (Jul 11 2019 at 06:53, on Zulip):

@Dylan MacKenzie okay, so, let me know if you've written this down before

Dylan MacKenzie (Jul 11 2019 at 06:54, on Zulip):

The order of writes is handled by the dataflow analysis. If there's a whole definition of x followed by a partial definition of x, both will reach a "use" further down

Dylan MacKenzie (Jul 11 2019 at 06:54, on Zulip):

When there's multiple entries, the reaching definitions at exit of all predecessors are unioned

eddyb (Jul 11 2019 at 06:55, on Zulip):

for every local x, you're propagating a conservative approximation f(x) to every use of x, made by merging all the writes to x uniformly

eddyb (Jul 11 2019 at 06:55, on Zulip):

to do this, you're first propagating a writes(x) set, and f(x) at any given point is writes(x).map(f).fold(merge) at that point

Dylan MacKenzie (Jul 11 2019 at 06:56, on Zulip):

Yes. Can we change f(x) to qualif(x)

eddyb (Jul 11 2019 at 06:56, on Zulip):

propagating f directly is equivalent to propagating writes, but the latter is more reusable, for multiple functions

eddyb (Jul 11 2019 at 06:56, on Zulip):

yeah I just meant dataflow in general

Dylan MacKenzie (Jul 11 2019 at 06:56, on Zulip):

Yes!

eddyb (Jul 11 2019 at 06:57, on Zulip):

okay so I see why this is sound, it's because of the unordered merge (ignoring full writes)

Dylan MacKenzie (Jul 11 2019 at 06:57, on Zulip):

(and it can be done using only gens and kills

eddyb (Jul 11 2019 at 06:57, on Zulip):

you couldn't use this for constant folding, for example

eddyb (Jul 11 2019 at 06:58, on Zulip):

(I'm a bit worried that naively doing this with the gen/kill dataflow framework might be less efficient than other alternatives but that's unrelated to my other concerns)

Dylan MacKenzie (Jul 11 2019 at 06:58, on Zulip):

I'm pretty sure you can't, because then you need a lattice containing all possible values

Dylan MacKenzie (Jul 11 2019 at 06:58, on Zulip):

for the constant

eddyb (Jul 11 2019 at 06:59, on Zulip):

hmm, you could handle a += 1 because you'd be seeing a = a + 1 and it's like a@2 = a@1 + 1

eddyb (Jul 11 2019 at 06:59, on Zulip):

what you couldn't handle is the distinction between if/else and the entry vs backedge of a loop

eddyb (Jul 11 2019 at 07:01, on Zulip):

but even then, if your merge(a, b) is if a? == b? { a } else { None }... that's still order-independent

Dylan MacKenzie (Jul 11 2019 at 07:02, on Zulip):

The analysis I have in mind doesn't assume anything about conditionals, so the a? == b? is confusing to me

eddyb (Jul 11 2019 at 07:02, on Zulip):

@Dylan MacKenzie okay, so, full writes are SSA-like which is very good, and having multiple of them is always from a "CFG merge", so only partial writes can be order-dependent

eddyb (Jul 11 2019 at 07:03, on Zulip):

I meant for constant-folding: keep the value only if it's equal on all edges

eddyb (Jul 11 2019 at 07:04, on Zulip):

okay so if you were doing symbolic tracking of ADTs, or even using miri allocations, then my previous example with a, b and y flowing into x would be a problem

eddyb (Jul 11 2019 at 07:05, on Zulip):

pretty much because you don't have a merge, you have side-effects

eddyb (Jul 11 2019 at 07:05, on Zulip):

@Dylan MacKenzie okay... what else, how do you handle &mut x?

eddyb (Jul 11 2019 at 07:05, on Zulip):

or borrows in general, I guess, not just mutable ones

Dylan MacKenzie (Jul 11 2019 at 07:06, on Zulip):

It's what you proposed on nagisa's PR

eddyb (Jul 11 2019 at 07:06, on Zulip):

(since &Cell<T> could still mutate T)

eddyb (Jul 11 2019 at 07:06, on Zulip):

assume the worst, based on the type?

eddyb (Jul 11 2019 at 07:06, on Zulip):

do you track it in the chain though?

Dylan MacKenzie (Jul 11 2019 at 07:06, on Zulip):

I use HaveBeenBorrowedLocals to see if a pointer to x could possible exist at a given program point

eddyb (Jul 11 2019 at 07:07, on Zulip):

oh

Dylan MacKenzie (Jul 11 2019 at 07:07, on Zulip):

If so, I mark any indirect definitions as possibly writing to x

Dylan MacKenzie (Jul 11 2019 at 07:07, on Zulip):

(Indirect definitions are *p = ...)

eddyb (Jul 11 2019 at 07:07, on Zulip):

that's not enough, and it could be wrong qualif-wise

Dylan MacKenzie (Jul 11 2019 at 07:08, on Zulip):

counterexample?

eddyb (Jul 11 2019 at 07:08, on Zulip):

function calls

Dylan MacKenzie (Jul 11 2019 at 07:08, on Zulip):

Those are handled

eddyb (Jul 11 2019 at 07:08, on Zulip):

so what do you do then?

Dylan MacKenzie (Jul 11 2019 at 07:08, on Zulip):

(one moment

Dylan MacKenzie (Jul 11 2019 at 07:09, on Zulip):

https://github.com/rust-lang/rust/pull/62547/files#diff-f7260434bc49a5d0402c7cf7f08ea77cR58

Dylan MacKenzie (Jul 11 2019 at 07:09, on Zulip):

here

eddyb (Jul 11 2019 at 07:09, on Zulip):

I'm scared of opening that :D

Dylan MacKenzie (Jul 11 2019 at 07:10, on Zulip):

I mean your can look at the domain

Dylan MacKenzie (Jul 11 2019 at 07:10, on Zulip):

?

Dylan MacKenzie (Jul 11 2019 at 07:10, on Zulip):

or go to #62547

eddyb (Jul 11 2019 at 07:10, on Zulip):

I'm scared of looking at the PR right now

Dylan MacKenzie (Jul 11 2019 at 07:10, on Zulip):
        // We can't know (without MIR inlining or explicit annotation) whether a callee mutates
        // data behind a pointer. If the address of one of our locals is observable, it may be the
        // target of such a mutation. A type-based analysis (e.g. does this function take any type
        // containing a mutable reference as a parameter?) is insufficient, since raw pointers can
        // be laundered through any integral type.
Dylan MacKenzie (Jul 11 2019 at 07:11, on Zulip):

So we treat all function calls as "indirect definitions"

Dylan MacKenzie (Jul 11 2019 at 07:12, on Zulip):

This is not precise, but it's not clear to me how one would do any better

eddyb (Jul 11 2019 at 07:13, on Zulip):

okay, yeah, that sounds fine (I already wrote a similar analysis last year. ugh I really need to get all of that merged)

eddyb (Jul 11 2019 at 07:13, on Zulip):

but anyway, from all of this what I gather is:
1. "use-def" confused the hell out of me, "reaching writes" makes a lot more sense
2. "merge all writes", in an order-independent way, is what makes this work, and it should be somehow put up front (maybe make it a literal bool that gets |'d by the framework?)

Dylan MacKenzie (Jul 11 2019 at 07:15, on Zulip):

"use-def chain" and "reaching definitions" are terms of art so I'm loathe to change them. If more people chime in I will. AFAICT I'm using them correctly.

eddyb (Jul 11 2019 at 07:15, on Zulip):

and qualify_consts should have the 2 type-based qualif's simply ignore the qualification if the type isn't even susceptible to it

Dylan MacKenzie (Jul 11 2019 at 07:15, on Zulip):

The "def" part is confusing though

eddyb (Jul 11 2019 at 07:15, on Zulip):

they are terms of art that apply much better in more SSA-like situations :P

eddyb (Jul 11 2019 at 07:16, on Zulip):

actually, hmm, for the 2 non-type-based qualifs (the promotable stuff), you want borrows to just set them

eddyb (Jul 11 2019 at 07:16, on Zulip):

@Dylan MacKenzie so maybe just treat non-frozen borrows as resetting to the "worst case for that type"?

Dylan MacKenzie (Jul 11 2019 at 07:17, on Zulip):

I agree you can bail out for HasInteriorMut and NeedsDrop

Dylan MacKenzie (Jul 11 2019 at 07:17, on Zulip):

Well, why not wait until there's actually a write to the target of a pointer?

eddyb (Jul 11 2019 at 07:17, on Zulip):

which for the unpromotability qualifs, the worst case doesn't depend on the type and it's 1/true

eddyb (Jul 11 2019 at 07:18, on Zulip):

tracking indirect writes is neat but IMO too expensive, and it's not like we promote anything you can borrow more than once anyway?

Dylan MacKenzie (Jul 11 2019 at 07:18, on Zulip):

(I'm still not 100% on how this extends to promotability, my mental model has been exclusively HasInteriorMut and NeedsDrop)

eddyb (Jul 11 2019 at 07:19, on Zulip):

like, we don't allow user variables, only temps, in the MIR of promoted expressions, so anything you could borrow yourself and mutate doesn't need to be allowed

eddyb (Jul 11 2019 at 07:19, on Zulip):

yeah for those two it's just easier to assume that they're type-based once they're borrowed

eddyb (Jul 11 2019 at 07:21, on Zulip):

this would be easier, IMO, if you weren't restricted by using the existing dataflow framework, for creating the chains (or "sets", really)

eddyb (Jul 11 2019 at 07:21, on Zulip):

since you could have richer enums describing the kind of write

Dylan MacKenzie (Jul 11 2019 at 07:23, on Zulip):

I think it is likely to be more efficient, I think it would take more implementation work though.

Dylan MacKenzie (Jul 11 2019 at 07:23, on Zulip):

I did propose a custom dataflow framework at the top of this thread

eddyb (Jul 11 2019 at 07:23, on Zulip):

fair enough

eddyb (Jul 11 2019 at 07:23, on Zulip):

I'm a bit mad that we have to transfer information between locals. that's the only reason what I had in mind doesn't work :(

Dylan MacKenzie (Jul 11 2019 at 07:24, on Zulip):

I never intended for anyone to write a dataflow implementation themselves, I'm sorry if that was ever an interpretation of anything I said

XD

eddyb (Jul 11 2019 at 07:24, on Zulip):

oh well now I can pretend this took ages to happen because it's actually hard

eddyb (Jul 11 2019 at 07:25, on Zulip):

@Dylan MacKenzie maybe we should have a discussion with @oli and @nikomatsakis and @Santiago Pastorino to pick some terminology that we all agree on?

eddyb (Jul 11 2019 at 07:25, on Zulip):

I think "reaching writes set" is the most accurate

eddyb (Jul 11 2019 at 07:26, on Zulip):

and you could also use a different "ever borrowed" kill/gen dataflow (might already exist for generators)

eddyb (Jul 11 2019 at 07:26, on Zulip):

well, no, you need "ever mutably borrowed"

Dylan MacKenzie (Jul 11 2019 at 07:27, on Zulip):

Yes that would be nice. I'm fine with "reaching writes set", although I think the actual implementor of BitDenotation should be called ReachingDefinitions.

eddyb (Jul 11 2019 at 07:27, on Zulip):

ReachingDefs is okay I guess

Dylan MacKenzie (Jul 11 2019 at 07:27, on Zulip):

As long as it's not doing anything special

eddyb (Jul 11 2019 at 07:28, on Zulip):

the "ever mutably borrowed" would be to gate whether you're assuming the worst or actually merging the writes set

eddyb (Jul 11 2019 at 07:28, on Zulip):

@Dylan MacKenzie can we land a "reaching defs" that ignores indirect writes or borrows and has no infra for doing the "merge RHSes of writes" style of dataflow?

Dylan MacKenzie (Jul 11 2019 at 07:29, on Zulip):

How do you mean "ignores writes"?

eddyb (Jul 11 2019 at 07:29, on Zulip):

typo, sorry. anyway I think such a PR would be easier to review

eddyb (Jul 11 2019 at 07:29, on Zulip):

oh and I keep forgetting, I should ping @pnkfelix to look at this stuff

Dylan MacKenzie (Jul 11 2019 at 07:31, on Zulip):

So the "merge RHSes of writes" is seperate from the dataflow pass

Dylan MacKenzie (Jul 11 2019 at 07:31, on Zulip):

in the current PR

Dylan MacKenzie (Jul 11 2019 at 07:32, on Zulip):

I could take out all handling of indirect writes/borrows and only do dataflow on defs locals that are never borrowed

Dylan MacKenzie (Jul 11 2019 at 07:32, on Zulip):

but I don't know how much easier that would be to review.

eddyb (Jul 11 2019 at 07:32, on Zulip):

oh I guess you could use one bit for "was borrowed"

Dylan MacKenzie (Jul 11 2019 at 07:33, on Zulip):

That's already been implemented in theHaveBeenBorrowedLocals dataflow analysis

Dylan MacKenzie (Jul 11 2019 at 07:33, on Zulip):

(which was meant for generators originally)

eddyb (Jul 11 2019 at 07:33, on Zulip):

and &mut x would act as if it was a x = unknown() but not in the program

eddyb (Jul 11 2019 at 07:33, on Zulip):

well, in your case, frozen borrows are fine

Dylan MacKenzie (Jul 11 2019 at 07:34, on Zulip):

What if I have let p = &mut x; x = 2; *p = 4;

eddyb (Jul 11 2019 at 07:34, on Zulip):

ah drat I see

eddyb (Jul 11 2019 at 07:35, on Zulip):

okay, fine

Dylan MacKenzie (Jul 11 2019 at 07:51, on Zulip):

I'm off to bed. If you like you can take a look at the reaching definitions analysis itself

Dylan MacKenzie (Jul 11 2019 at 07:51, on Zulip):

https://github.com/rust-lang/rust/pull/62547/files#diff-f7260434bc49a5d0402c7cf7f08ea77c

Dylan MacKenzie (Jul 11 2019 at 07:52, on Zulip):

I think it's comprehensible (but then again I wrote it)

Dylan MacKenzie (Jul 11 2019 at 07:53, on Zulip):

This is separate from the "chain" data structure which determines precisely which subset of writes are reachable for a use of a given local

Dylan MacKenzie (Jul 11 2019 at 07:58, on Zulip):

the output of "reaching definitions" is all definitions of any local which reach a given statement

Dylan MacKenzie (Jul 14 2019 at 23:51, on Zulip):

I've rebased #62547 with some fixes and additional documentation in the hopes of clearing up some of the confusion. I've also left some comments on github justifying the naming of UseDefChain and ReachingDefinitions and discussing how this could be used to implement const propagation.

Dylan MacKenzie (Jul 14 2019 at 23:52, on Zulip):

@eddyb

Dylan MacKenzie (Jul 16 2019 at 18:29, on Zulip):

@eddyb Could I pick your brain for a little while about the current logic around const qualification?

Dylan MacKenzie (Jul 17 2019 at 16:51, on Zulip):

pinging @eddyb

Dylan MacKenzie (Jul 22 2019 at 17:09, on Zulip):

@eddyb this line in const_qualify.rs makes dataflow-based const qualification a bit harder, since it makes the IsNotPromotable pass dependent on HasMutInterior. Is there a better way to express this?

Dylan MacKenzie (Jul 22 2019 at 17:09, on Zulip):

https://github.com/rust-lang/rust/blob/4bc1ce7bdb7f5dc9ea07c0b630c087d8e11140e4/src/librustc_mir/transform/qualify_consts.rs#L768

Dylan MacKenzie (Jul 26 2019 at 12:37, on Zulip):

@eddyb Is there someone who can help clarify the current logic in qualify_consts.rs for me?

oli (Jul 26 2019 at 15:29, on Zulip):

I can help

oli (Jul 26 2019 at 15:33, on Zulip):

The comment in the link you posted suggests that the dependency exists just to reduce duplicate diagnostics. Maybe we can dedup another way

Dylan MacKenzie (Jul 27 2019 at 16:29, on Zulip):

Thanks @oli! So the basic idea is to replace Qualifs::in_local, which is currently only kept up to date within a block, with a flow-sensitive version derived from dataflow analysis (either reaching definitions based or using a not-yet-implemented custom dataflow analysis). I'm a bit confused because "const qualification" is also run in normal functions to mark candidates for promotion, but I can't really imagine what it means to track promotability across basic blocks.

oli (Jul 27 2019 at 16:32, on Zulip):

we only track promotability across asserts and uncoditional jump I believe

oli (Jul 27 2019 at 16:32, on Zulip):

we won't ever expand that

Dylan MacKenzie (Jul 27 2019 at 16:32, on Zulip):

I think I'm just trying to clarify what exactly the semantics are around Qualifs[Is{Implicitly,}Promotable] in this particular pass. If we have a = if x { b } else { c } where both b and c are promotable, we could never really promote a right?

oli (Jul 27 2019 at 16:32, on Zulip):

no, not ever

Dylan MacKenzie (Jul 27 2019 at 16:33, on Zulip):

WIthout doing const-prop/dead-code elimination first (but that's a ways off)

oli (Jul 27 2019 at 16:33, on Zulip):

may not even promote b or c if they depend on values created before the if

oli (Jul 27 2019 at 16:33, on Zulip):

const prop isn't happening before promotion

oli (Jul 27 2019 at 16:33, on Zulip):

promotion needs to happen in a guaranteed manner

oli (Jul 27 2019 at 16:33, on Zulip):

const prop is opportunistic

Dylan MacKenzie (Jul 27 2019 at 16:34, on Zulip):

So does it even make sense to trackIsPromotable in a flow-sensitive manner?

Dylan MacKenzie (Jul 27 2019 at 16:35, on Zulip):

I think we just need to do this for HasMutInterior and NeedsDrop

oli (Jul 27 2019 at 16:35, on Zulip):

I thought it was necessary for const eval?

oli (Jul 27 2019 at 16:35, on Zulip):

oh right

oli (Jul 27 2019 at 16:35, on Zulip):

those are enough

Dylan MacKenzie (Jul 27 2019 at 16:36, on Zulip):

Okay good! I think that simplifies things for me

Dylan MacKenzie (Jul 27 2019 at 16:38, on Zulip):

I'm going to try and refactor qualif_consts.rs to use a flow-sensitive backend to determine HasMutInterior and NeedsDrop for each local at each program point.

Dylan MacKenzie (Jul 27 2019 at 16:39, on Zulip):

And then try to get a proof of concept going that uses #62547 to build that flow-sensitive backend

Dylan MacKenzie (Jul 27 2019 at 16:45, on Zulip):

A few more clarifications around promotion; I believe the main use-case is R-value static promotion like let x = &5. We only need to promote things when their reference is taken. Otherwise things are just handled by const-eval?

oli (Jul 27 2019 at 18:53, on Zulip):

Yes

oli (Jul 27 2019 at 18:53, on Zulip):

Const prop to be exact

oli (Jul 27 2019 at 18:54, on Zulip):

Well, also there's repeat expression initializers

oli (Jul 27 2019 at 18:54, on Zulip):

And the hack for SIMD

oli (Jul 27 2019 at 18:54, on Zulip):

Where function arguments are promoted

Dylan MacKenzie (Jul 27 2019 at 20:18, on Zulip):

Okay. I just saw these in Candidate enum over in promote_consts.rs.

Dylan MacKenzie (Jul 29 2019 at 15:24, on Zulip):

@oli Just a heads up. I'm going to start on this this evening, but I will be pretty slow. I've tried to get started a few times before, but ground to a halt. I think a lack of understanding around promotion as well as the cyclomatic complexity of some functions were the problem. Hopefully the first one will be less of a problem now :)

Alexander Regueiro (Aug 10 2019 at 16:30, on Zulip):

@Dylan MacKenzie Hey, how's progress out of curiosity?

Dylan MacKenzie (Aug 11 2019 at 06:31, on Zulip):

@Alexander Regueiro Short answer: stalled.

Dylan MacKenzie (Aug 11 2019 at 06:37, on Zulip):

@Alexander Regueiro Long answer: I'm doubtful that I'm able to move this forward since whenever I've tried to extend the current qualification logic to more complex CFGs, it requires a pretty large re-architecting. This makes preserving the existing behavior of the const qualifier difficult, and makes me worry that my work won't get merged. I can't help but feel that @eddyb has a much simpler solution in mind that I am unable to see, which is a bit discouraging.

eddyb (Aug 11 2019 at 06:37, on Zulip):

ugh I fell behind this again :(

Dylan MacKenzie (Aug 11 2019 at 06:37, on Zulip):

If you're interested in working on this, either by yourself or in collaboration, let me know and we can have a synchronous discussion.

eddyb (Aug 11 2019 at 06:38, on Zulip):

(also, last week I was away on vacation)

eddyb (Aug 11 2019 at 06:38, on Zulip):

@Dylan MacKenzie so the problem you hit is that there are still interactions between the different bits, right?

Dylan MacKenzie (Aug 11 2019 at 06:38, on Zulip):

@eddyb No problem.

eddyb (Aug 11 2019 at 06:38, on Zulip):

is it that silly thing that has a comment on it saying it's to improve error quality?

eddyb (Aug 11 2019 at 06:38, on Zulip):

we could try turning it off in the current code and see what happens

Dylan MacKenzie (Aug 11 2019 at 06:38, on Zulip):

@eddyb Yes, that's the only explicit dependency

eddyb (Aug 11 2019 at 06:38, on Zulip):

maybe we can massage the diagnostics so they make sense

eddyb (Aug 11 2019 at 06:39, on Zulip):

actually, hmm

eddyb (Aug 11 2019 at 06:40, on Zulip):

I think that if we just clear one flag and not set the other, it might cause bugs

eddyb (Aug 11 2019 at 06:41, on Zulip):

e.g. &&Cell::new(0) might try to promote the outer reference even if the inner one isn't allowed to be promoted

eddyb (Aug 11 2019 at 06:42, on Zulip):

@Dylan MacKenzie okay, new idea: you know how the actual promotion works? the part that mutates?

Dylan MacKenzie (Aug 11 2019 at 06:43, on Zulip):

mutates the MIR? In promote_consts.rs?

eddyb (Aug 11 2019 at 06:43, on Zulip):

it traverses all the intermediary Rvalue's by using the "promotability state" information to know where the only assignment to a local is

eddyb (Aug 11 2019 at 06:43, on Zulip):

so basically it's like an SSA tree

eddyb (Aug 11 2019 at 06:44, on Zulip):

we could do that same traversal and check that every borrow is actually promotable

eddyb (Aug 11 2019 at 06:44, on Zulip):

after having computed everything locally

eddyb (Aug 11 2019 at 06:44, on Zulip):

so &expr would always have no bits set

Dylan MacKenzie (Aug 11 2019 at 06:44, on Zulip):

My understanding was that only temporaries which are assigned to exactly once are considered for promotion

Dylan MacKenzie (Aug 11 2019 at 06:44, on Zulip):

This is what you're saying?

eddyb (Aug 11 2019 at 06:45, on Zulip):

yeah, but you have to keep in mind that you can have e.g. &(1 + 2, false) which is several Rvalue's

eddyb (Aug 11 2019 at 06:45, on Zulip):

so they form a tree, similar to the AST for the expression

Dylan MacKenzie (Aug 11 2019 at 06:45, on Zulip):

That would be translated to _1 = 1+2; _2 = false; _3 = (_1, _2); _4 = &_3?

eddyb (Aug 11 2019 at 06:45, on Zulip):

something like that yeah

Dylan MacKenzie (Aug 11 2019 at 06:46, on Zulip):

give or take a panic on arithmetic overflow?

eddyb (Aug 11 2019 at 06:46, on Zulip):

yupp

eddyb (Aug 11 2019 at 06:46, on Zulip):

once we compute the bits for what locals contain then we can do a tentative promotion (maybe even in the mutating code, just make it failible? but that might be too hard)

eddyb (Aug 11 2019 at 06:47, on Zulip):

and bail out if we find anything fishy

eddyb (Aug 11 2019 at 06:47, on Zulip):

so &&Cell::new(0) would not mind the outer reference by itself but also look inside, and in the inner reference, it would find Cell::new(0) with interior mutability

eddyb (Aug 11 2019 at 06:47, on Zulip):

this could mean we might be able to even remove some of the uses of the qualification bits

eddyb (Aug 11 2019 at 06:48, on Zulip):

@Dylan MacKenzie wow I feel so dumb

Dylan MacKenzie (Aug 11 2019 at 06:48, on Zulip):

What does the MIR look like for &&Cell::new(0)? I'm not quite sure what problem you're solving here.

eddyb (Aug 11 2019 at 06:48, on Zulip):

yeah we only need to support the promotion usecases for "SSA tree equivalent to linear control-flow"

Dylan MacKenzie (Aug 11 2019 at 06:49, on Zulip):

Right, we don't need to do dataflow for promotion.

eddyb (Aug 11 2019 at 06:49, on Zulip):

the problem is that right now the inner &Cell::new(0) is marked as Unpromotable

eddyb (Aug 11 2019 at 06:49, on Zulip):

because the temp it borrows, holding a Cell<i32>, is InteriorMut

eddyb (Aug 11 2019 at 06:49, on Zulip):

and that's the dependency between two qualif bits that you hit

Dylan MacKenzie (Aug 11 2019 at 06:49, on Zulip):

Since we have an additional rule that "promotable temps can only be assigned to exactly once"

eddyb (Aug 11 2019 at 06:50, on Zulip):

wait so what do we need dataflow for? the interiormut/needsdrop?

Dylan MacKenzie (Aug 11 2019 at 06:50, on Zulip):

Yes. Exactly those two qualifs I believe.

eddyb (Aug 11 2019 at 06:52, on Zulip):

are you interested in changing the code on master to rely on the "SSA-like tree" for promotion checking? assuming we never write the qualifs again after initialization for promotable temps, we should already be able to use local_qualifs to do this second pass

Dylan MacKenzie (Aug 11 2019 at 06:52, on Zulip):

Okay, so this is the genesis behind the current code clearing HasMutInterior and replacing it with IsNotPromotable.

eddyb (Aug 11 2019 at 06:52, on Zulip):

I could also look into it since I have a bit more free time now (long story)

eddyb (Aug 11 2019 at 06:52, on Zulip):

how did I miss this lmao

Dylan MacKenzie (Aug 11 2019 at 06:52, on Zulip):

on assignment

eddyb (Aug 11 2019 at 06:53, on Zulip):

yeah that's done so anything containing the unpromotable reference isn't accidentally considered promotable itself

eddyb (Aug 11 2019 at 06:53, on Zulip):

for a second I thought you could just keep HasMutInterior but that's a really bad idea

eddyb (Aug 11 2019 at 06:53, on Zulip):

because things will clear it based on type

Dylan MacKenzie (Aug 11 2019 at 06:55, on Zulip):

Regarding your "SSA-like tree" framing, how is that different from the current code?

Dylan MacKenzie (Aug 11 2019 at 06:56, on Zulip):

It currently relies on a linear CFG because one always exists in const fns since jumps are disallowed, and it can assume that promotable temps exist in a subset of the CFG which is linear because promotable temps can only be assigned to once in the mir::Body

eddyb (Aug 11 2019 at 06:58, on Zulip):

what I mean is not relying on the bits to be propagated correctly

Dylan MacKenzie (Aug 11 2019 at 06:58, on Zulip):

Let me know how I can clarify that. (That sentence confuses me and I wrote it)

eddyb (Aug 11 2019 at 06:59, on Zulip):

hmm, actually...

eddyb (Aug 11 2019 at 06:59, on Zulip):

if we could somehow avoid code duplication (by making the step-wise stuff, like the current Qualif trait implementors, general enough)

eddyb (Aug 11 2019 at 07:00, on Zulip):

we might be able to just write a promotion-checking pass with no dataflow

eddyb (Aug 11 2019 at 07:00, on Zulip):

more like your def-use idea, but simpler because no branching

eddyb (Aug 11 2019 at 07:00, on Zulip):

and then we can figure out how to implement const-checking with the same primitives

eddyb (Aug 11 2019 at 07:01, on Zulip):

I think @RalfJ might like this direction?

eddyb (Aug 11 2019 at 07:02, on Zulip):

right now the code is a mess because it sort of does two (quite related) things at the same time

eddyb (Aug 11 2019 at 07:02, on Zulip):

the dataflow concerns are enough for me to want to split it up if possible

Dylan MacKenzie (Aug 11 2019 at 07:02, on Zulip):

right now the code is a mess because it sort of does two (quite related) things at the same time

This has been a big struggle for me :)

eddyb (Aug 11 2019 at 07:02, on Zulip):

I'm really sorry Q_Q

Dylan MacKenzie (Aug 11 2019 at 07:03, on Zulip):

Yes, my original thought was that we want a linear pass which does promotion and a dataflow-based pass which checks for HasMutInterior and NeedsDrop

eddyb (Aug 11 2019 at 07:04, on Zulip):

yeah, it's just that the linear pass can be nicer because it can look at the "SSA-like trees" instead of being stateful

eddyb (Aug 11 2019 at 07:05, on Zulip):

@Dylan MacKenzie I have... an evil idea...

eddyb (Aug 11 2019 at 07:05, on Zulip):

that I should've had ages ago :(

Dylan MacKenzie (Aug 11 2019 at 07:05, on Zulip):

but there's a line in the current code which replaces HasMutInterior with IsNotPromotable when an assignment like _1 = &_2 occurs

eddyb (Aug 11 2019 at 07:05, on Zulip):

we can do conservative approximations

Dylan MacKenzie (Aug 11 2019 at 07:05, on Zulip):

So the two seem linked in some way that I still don't fully understand

Dylan MacKenzie (Aug 11 2019 at 07:06, on Zulip):

we can do conservative approximations

?

eddyb (Aug 11 2019 at 07:06, on Zulip):

so, that line is irrelevant for const-checking: an error would've always been emitted for that assignment, so it doesn't matter what you do as long as you don't cause more errors

Dylan MacKenzie (Aug 11 2019 at 07:06, on Zulip):

so, that line is irrelevant for const-checking: an error would've always been emitted for that assignment, so it doesn't matter what you do as long as you don't cause more errors

So it really is just for diagnostics (as the comment suggests)

eddyb (Aug 11 2019 at 07:06, on Zulip):

so ideally you would just turn off the HasInteriorMut flag (&T is always Freeze anyway, regardless of &)

eddyb (Aug 11 2019 at 07:07, on Zulip):

but for promotion, you do need to keep track that you have an unpromotable reference so you don't promote something larger that contains it

eddyb (Aug 11 2019 at 07:07, on Zulip):

because e.g. &&0 is promoted as &'static &'static 0, at the outermost reference (the inner one isn't promoted separately)

eddyb (Aug 11 2019 at 07:09, on Zulip):

anyway, the conservative approx: we don't care about the order and just gather dependencies between locals, i.e. what may flow into what (note that this could be a cyclic graph because of loops)

eddyb (Aug 11 2019 at 07:10, on Zulip):

and then we saturate it to fixpoint

eddyb (Aug 11 2019 at 07:10, on Zulip):

we only need to specially handle one thing, which is initialization state

eddyb (Aug 11 2019 at 07:11, on Zulip):

if a local is definitely moved out of fully at a drop, then what the local contained in it when it was initialized doesn't matter

eddyb (Aug 11 2019 at 07:11, on Zulip):

but that can be done with an existing dataflow analysis pass

eddyb (Aug 11 2019 at 07:11, on Zulip):

@Dylan MacKenzie gah I wish I wasn't so stubborn to come up with this before you spent time working on a framework Q_Q

eddyb (Aug 11 2019 at 07:12, on Zulip):

or, wait, how close is this to what you have?

Dylan MacKenzie (Aug 11 2019 at 07:12, on Zulip):

anyway, the conservative approx: we don't care about the order and just gather dependencies between locals, i.e. what may flow into what (note that this could be a cyclic graph because of loops)

This part seems pretty simlar?

eddyb (Aug 11 2019 at 07:12, on Zulip):

because if I did another dumb thing and described the code you've already written,,, then we'll just use that

eddyb (Aug 11 2019 at 07:12, on Zulip):

I guess I'mm still waking up here

Dylan MacKenzie (Aug 11 2019 at 07:13, on Zulip):

I guess I'mm still waking up here

I'm falling asleep here, so at least it's fair XD

Dylan MacKenzie (Aug 11 2019 at 07:15, on Zulip):

Dylan MacKenzie gah I wish I wasn't so stubborn to come up with this before you spent time working on a framework Q_Q

I'm only interested in helping to get #49146 (and friends) implemented

eddyb (Aug 11 2019 at 07:16, on Zulip):

okay, so we have 2 tasks we could do on master:

1. remove the Operand::Move side-effect of clearning the NeedsDrop qualif bit, and use a dataflow initialization analysis to know when a drop is dead
2. compute promotion candidates in promote_consts instead of qualify_consts (i.e. all of them, without checking), and validate them by traversing the tree of temps

eddyb (Aug 11 2019 at 07:16, on Zulip):

and then I think we can "just" land your PR?!

eddyb (Aug 11 2019 at 07:16, on Zulip):

do you think you'd want to do one of these or should I take a stab at it?

Dylan MacKenzie (Aug 11 2019 at 07:18, on Zulip):

You should take a stab at it. I think both steps are productive. However, I think we should think about using a custom dataflow analysis and not my PR.

eddyb (Aug 11 2019 at 07:18, on Zulip):

if your PR is order-agnostic then it's not that different from my conservative idea

Dylan MacKenzie (Aug 11 2019 at 07:19, on Zulip):

In order to use it i was going to have to change each in_x method from Qualif to return an enum like

eddyb (Aug 11 2019 at 07:19, on Zulip):

like, you still need 1. with your PR, right? otherwise, you couldn't deal with the distinction between moved-out locals and still-initialized, drop-wise

Dylan MacKenzie (Aug 11 2019 at 07:19, on Zulip):
Definitely
Conditionally(Vec<Local>)
Not
eddyb (Aug 11 2019 at 07:19, on Zulip):

yeah that is reasonable

eddyb (Aug 11 2019 at 07:20, on Zulip):

I think we can make it more efficient, but that is sort of the thing we need to do in order to be generic enough to handle this

Dylan MacKenzie (Aug 11 2019 at 07:20, on Zulip):

Okay, in that case doing it on top of my PR would be fine. It just seemed really heavy on allocations.

eddyb (Aug 11 2019 at 07:20, on Zulip):

ahh, see

eddyb (Aug 11 2019 at 07:20, on Zulip):

that's where you have to get clever :P

Dylan MacKenzie (Aug 11 2019 at 07:21, on Zulip):

And my PR is already really heavy on allocations

eddyb (Aug 11 2019 at 07:21, on Zulip):

lemme show you an example of avoiding allocations

eddyb (Aug 11 2019 at 07:22, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc/mir/mod.rs#L1235

eddyb (Aug 11 2019 at 07:22, on Zulip):

wow this doesn't even use Cow anymore

eddyb (Aug 11 2019 at 07:23, on Zulip):

@Dylan MacKenzie frankly, once we get rid of the two promotability qualifs, things are going to be muuuch better

Dylan MacKenzie (Aug 11 2019 at 07:23, on Zulip):

Dylan MacKenzie frankly, once we get rid of the two promotability qualifs, things are going to be muuuch better

Agreed

oli (Aug 11 2019 at 07:23, on Zulip):

Needs more existential types

eddyb (Aug 11 2019 at 07:23, on Zulip):

because the other two are about value properties, not how you got a value. how many ways do you know to get one value from another?

eddyb (Aug 11 2019 at 07:24, on Zulip):

projection and construction

eddyb (Aug 11 2019 at 07:24, on Zulip):

that's... it

eddyb (Aug 11 2019 at 07:25, on Zulip):

@Dylan MacKenzie so what's going to be left is, uhh, a Ty -> bool and AggregateKind -> bool

eddyb (Aug 11 2019 at 07:25, on Zulip):

that's it I'm pretty sure

eddyb (Aug 11 2019 at 07:27, on Zulip):

types are an upper bound, aggregates are a lower bound, everything else is the same between !Freeze and NeedsDrop

eddyb (Aug 11 2019 at 07:33, on Zulip):

@Dylan MacKenzie we could probably even dedup the "promotable locals" by basically using your "where is this written to" information and limiting to one full initialization

Dylan MacKenzie (Aug 11 2019 at 07:34, on Zulip):

@eddyb I think I need to go to bed now, but hopefully you could try implementing your ideas and see what problems arise. When I tried implementing qualification on top of my PR I had to makeQualif::in_local become recursive thus required it be memoized. Perhaps your ideas are simpler and don't require as much storage. I think maybe I do better when looking at code than talking in the abstract (the exception being the current implementation of qualify_consts I guess :)

eddyb (Aug 11 2019 at 07:35, on Zulip):

@Dylan MacKenzie status: making sure that commenting this out results in a test failure https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/qualify_consts.rs#L771

Dylan MacKenzie (Aug 11 2019 at 07:37, on Zulip):

@eddyb Yes, one nice side-effect of the use-def pass is you can answer queries like "give me all the temporaries which have exactly one definition"

eddyb (Aug 11 2019 at 07:41, on Zulip):

wait, "def-use"... but, we don't care about the uses :P

Dylan MacKenzie (Aug 11 2019 at 07:43, on Zulip):

Well yes, basically anything that enumerates "def"s can give you that (for some definition of "def")

eddyb (Aug 11 2019 at 07:43, on Zulip):

can we just call it "find_accesses"? or "accesses"?

eddyb (Aug 11 2019 at 07:43, on Zulip):

or "find_writes" idk

Dylan MacKenzie (Aug 11 2019 at 07:44, on Zulip):

"find_writes" is fine with me.

Dylan MacKenzie (Aug 11 2019 at 07:45, on Zulip):

("access" would include both reads and writes, not sure if that's what you're going for)

Dylan MacKenzie (Aug 11 2019 at 07:46, on Zulip):

Totally unrelated, I have a branch which tried to split the propagation of qualifs between locals from the use of those qualifs into separate passes

Dylan MacKenzie (Aug 11 2019 at 07:46, on Zulip):

https://github.com/ecstatic-morse/rust/tree/qualifs

Dylan MacKenzie (Aug 11 2019 at 07:47, on Zulip):

If you find yourself doing something similar, maybe it would be useful to look at?

eddyb (Aug 11 2019 at 07:47, on Zulip):

thanks!

Dylan MacKenzie (Aug 11 2019 at 07:47, on Zulip):

The end goal was to replace QualifsAtLocation with something based on dataflow (at least for HasMutInterior and NeedsDrop)

Dylan MacKenzie (Aug 11 2019 at 07:48, on Zulip):

This was the least intrusive approach I could see

eddyb (Aug 11 2019 at 07:48, on Zulip):

error: internal compiler error: src/librustc_mir/interpret/intern.rs:183: const qualif failed to prevent mutable references

eddyb (Aug 11 2019 at 07:48, on Zulip):

I guess that's my confirmation and also assurance that even if we screw up, miri will catch us

RalfJ (Aug 11 2019 at 09:00, on Zulip):

I guess that's my confirmation and also assurance that even if we screw up, miri will catch us

well, sometimes. we started building that safety net with @oli 's refactoring of interning.

RalfJ (Aug 11 2019 at 09:01, on Zulip):

I am not sure yet if we catch all errors. but that is the goal.

RalfJ (Aug 11 2019 at 09:01, on Zulip):

this goes hand-in-hand with documenting in https://github.com/rust-rfcs/const-eval what exactly needs to be checked

RalfJ (Aug 11 2019 at 09:01, on Zulip):

help with that would be much appreciated, as UCG is taking up basically all of my Rust time these days

RalfJ (Aug 11 2019 at 09:02, on Zulip):

and please report a bug if you find something that miri does not catch :D

Alexander Regueiro (Aug 11 2019 at 15:45, on Zulip):

@Dylan MacKenzie I see. Thanks. Maybe worth another chat with him in that case... otherwise, a rearchitecting isn't necessarily a terrible thing.

Alexander Regueiro (Aug 11 2019 at 18:05, on Zulip):

Dylan MacKenzie I see. Thanks. Maybe worth another chat with him in that case... otherwise, a rearchitecting isn't necessarily a terrible thing.

Oh gosh... I see I missed a long conversation between the two of you before I even replied hah. Good to see anyway.

eddyb (Aug 12 2019 at 15:33, on Zulip):

@Dylan MacKenzie feel free to ping me daily or so - I haven't gotten to it because I decided to look into fixing miri's substs situation

eddyb (Aug 13 2019 at 12:30, on Zulip):

@Dylan MacKenzie one down! https://github.com/rust-lang/rust/pull/63518

eddyb (Aug 14 2019 at 06:24, on Zulip):

@oli on no :( https://github.com/rust-lang/rust/pull/63518#issuecomment-521117822

oli (Aug 14 2019 at 06:44, on Zulip):

Huh, idk what change caused this to become legal

eddyb (Aug 14 2019 at 06:44, on Zulip):

@oli it was always legal, I'm just dumb

eddyb (Aug 14 2019 at 06:45, on Zulip):

moving out of a variable clears its NeedsDrp

eddyb (Aug 14 2019 at 06:46, on Zulip):

and what's allowed on beta/nightly is reassigning locals

oli (Aug 14 2019 at 06:47, on Zulip):

Ah. Yea I remember a change there

eddyb (Aug 14 2019 at 09:43, on Zulip):

@Dylan MacKenzie I started on the second thing, I'm thinking I should do a crater run that ensures both strategies produce the same results

Dylan MacKenzie (Aug 18 2019 at 23:52, on Zulip):

@eddyb Sorry, I wasn't able to be at a computer for most of last week. I've been able to work on the refactor this weekend though.

Dylan MacKenzie (Aug 18 2019 at 23:56, on Zulip):

I'll try to get my branch to a more easily understandable state and publish it ASAP so I can get your feedback.

eddyb (Aug 21 2019 at 20:33, on Zulip):

@Dylan MacKenzie okay I finally got around to doing it: https://github.com/rust-lang/rust/compare/master...eddyb:promo-sanity

eddyb (Aug 21 2019 at 20:33, on Zulip):

gtg now but I will report tomorrow wiith test results

Dylan MacKenzie (Aug 22 2019 at 05:17, on Zulip):

@eddyb Awesome. I'm almost done with my branch. I'll publish tomorrow morning. Sorry for the lack of coordination. I've been a bit busy.

Dylan MacKenzie (Aug 22 2019 at 16:51, on Zulip):

@eddyb https://github.com/rust-lang/rust/compare/master...ecstatic-morse:qualifs-temp

Dylan MacKenzie (Aug 22 2019 at 16:51, on Zulip):

So I ended up doing a custom dataflow implementation, not the use-def chain based one

eddyb (Aug 22 2019 at 16:51, on Zulip):

heh, nice

eddyb (Aug 22 2019 at 16:52, on Zulip):

wait, I didn't realize that

Dylan MacKenzie (Aug 22 2019 at 16:52, on Zulip):

The reason was that I wanted to allow const-qualification to still run in the old, RPO mode

Dylan MacKenzie (Aug 22 2019 at 16:53, on Zulip):

Since we don't actually need to have dataflow-based qualification for non-const fns

eddyb (Aug 22 2019 at 16:53, on Zulip):

aaaah I see

Dylan MacKenzie (Aug 22 2019 at 16:53, on Zulip):

And the use-def chain required implementing Q::in_{operand,rvalue,...} differently for each mode

eddyb (Aug 22 2019 at 16:53, on Zulip):

right, and eventually we can rip out most of that and do it on-demand

Dylan MacKenzie (Aug 22 2019 at 16:54, on Zulip):

Whereas with a custom dataflow impl, all the Qualif methods stay pretty much the same

eddyb (Aug 22 2019 at 16:54, on Zulip):

while trying to share as much logic as possible (I went the other way, duplicating logic, for my initial test of the separate checker)

Dylan MacKenzie (Aug 22 2019 at 16:54, on Zulip):

and I can just implement another Resolver that runs in the old mode

Dylan MacKenzie (Aug 22 2019 at 16:55, on Zulip):

Yeah, so you're trying to separate promotion from qualification which I think is a good idea.

Dylan MacKenzie (Aug 22 2019 at 16:56, on Zulip):

Promotion never actually needs to run in dataflow mode, because only temporaries with exactly one assignment are promotable

eddyb (Aug 22 2019 at 16:56, on Zulip):

I kept running into confusing issues where if you have errors, promotion is limited one way or another

eddyb (Aug 22 2019 at 16:56, on Zulip):

like min_const_fn will promote nothing if there are min_const_fn check errors

eddyb (Aug 22 2019 at 16:57, on Zulip):

and then if you have non-trivial control-flow, the linear checker will just not see entire parts of it

eddyb (Aug 22 2019 at 16:58, on Zulip):

like my promoter chose to promote this https://github.com/rust-lang/rust/blob/master/src/test/ui/consts/const-eval/issue-52475.rs#L10

eddyb (Aug 22 2019 at 16:59, on Zulip):

because, well, it can promote in anything just as well as it can promote in runtime functions

eddyb (Aug 22 2019 at 16:59, on Zulip):

dataflow only needs to handle two bits, really, HasMutInterior and NeedsDrop

Dylan MacKenzie (Aug 22 2019 at 16:59, on Zulip):

Hmm, can we not promote &0 there?

eddyb (Aug 22 2019 at 17:00, on Zulip):

check_const never looks at the body of the while

eddyb (Aug 22 2019 at 17:00, on Zulip):

so it never picks up promotion candidates from within

eddyb (Aug 22 2019 at 17:00, on Zulip):

anyway this just means I need another special-case

Dylan MacKenzie (Aug 22 2019 at 17:01, on Zulip):

Oh wow. that's because the body gets rejected by min_const_fn

Dylan MacKenzie (Aug 22 2019 at 17:01, on Zulip):

?

eddyb (Aug 22 2019 at 17:01, on Zulip):

no, min_const_fn applies only to const fn

eddyb (Aug 22 2019 at 17:02, on Zulip):

this is an array length

Dylan MacKenzie (Aug 22 2019 at 17:02, on Zulip):

Ah yes, duh

Dylan MacKenzie (Aug 22 2019 at 17:03, on Zulip):

So I thought the const checking code does look at loop bodies

eddyb (Aug 22 2019 at 17:03, on Zulip):

it gives up the moment it sees a branch, I guess if it were an infinite loop it would have seen the body :P

Dylan MacKenzie (Aug 22 2019 at 17:03, on Zulip):

When it traverses basic blocks, if it sees one that was already visited (a back-edge in the CFG) it calls non_const

eddyb (Aug 22 2019 at 17:04, on Zulip):

this gets promoted btw https://github.com/rust-lang/rust/blob/master/src/test/ui/consts/const-eval/issue-52475.rs#L4

Dylan MacKenzie (Aug 22 2019 at 17:04, on Zulip):

Does the promotion logic give up as soon as it sees a branch

Dylan MacKenzie (Aug 22 2019 at 17:04, on Zulip):

?

eddyb (Aug 22 2019 at 17:04, on Zulip):

not just one that was visited. also note the None

eddyb (Aug 22 2019 at 17:04, on Zulip):

it takes a linear path, so it gives up if there are multiple targets

eddyb (Aug 22 2019 at 17:05, on Zulip):

wow Zulip is so laggy for me now

Dylan MacKenzie (Aug 22 2019 at 17:06, on Zulip):

Oh, SwitchInt is None here

Dylan MacKenzie (Aug 22 2019 at 17:06, on Zulip):

https://github.com/rust-lang/rust/blob/07ea2604075d6f896addce0e6949c7cf25dd3715/src/librustc_mir/transform/qualify_consts.rs#L323

Dylan MacKenzie (Aug 22 2019 at 17:06, on Zulip):

I understand now

eddyb (Aug 22 2019 at 17:07, on Zulip):

yeah all of these https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/qualify_consts.rs#L950-L956

eddyb (Aug 22 2019 at 17:08, on Zulip):

and, success!

Dylan MacKenzie (Aug 22 2019 at 17:17, on Zulip):

So there's still a ton to do on my branch, I need to implement the TempOnlyResolver, then check to see that the cursor is actually observing the proper set of effects. Basically, because qualif propagation was intertwined with const checking in the original code, some of the checks may be run before e.g. the return place of a call is given qualifs and some may be run after.

Dylan MacKenzie (Aug 22 2019 at 17:26, on Zulip):

Separating the existing pass into three: qualif propagation, const checking, and promotion is I think a noble goal. Splitting up the latter two (which your PR does) makes validating the separation of "qualif propagation" a lot easier.

eddyb (Aug 22 2019 at 17:41, on Zulip):

@Dylan MacKenzie here it is, sorry for the delays https://github.com/rust-lang/rust/pull/63812

Dylan MacKenzie (Aug 24 2019 at 18:13, on Zulip):

So I opened #63860, which contains a write-up of the changes in my branch. I'm working on splitting up assign in the current pass since its main purpose is obsoleted now that we don't actually propagate qualifs in Checker. I guess after implementing TempOnlyResolver I'll see if I can get the tests passing (the build/test cycle takes forever on my local machine, so I'll probably abuse CI for this).

Dylan MacKenzie (Aug 27 2019 at 00:30, on Zulip):

The latest version of #63860 results in an index out of bounds ICE here: https://github.com/rust-lang/rust/blob/9b91b9c10e3c87ed333a1e34c4f46ed68f1eee06/src/librustc_mir/borrow_check/nll/type_check/mod.rs#L427

Dylan MacKenzie (Aug 27 2019 at 00:32, on Zulip):

@eddyb Do you have any idea how this would get triggered?

oli (Aug 27 2019 at 05:56, on Zulip):

That can only happen on broken mir

oli (Aug 27 2019 at 05:57, on Zulip):

Maybe promotion does something only halfway?

Dylan MacKenzie (Aug 27 2019 at 06:00, on Zulip):

@oli like perhaps my PR is now incorrectly marking something for promotion something that the code that does promotion can't translate to a Body?

Dylan MacKenzie (Aug 27 2019 at 06:01, on Zulip):

@oli

oli (Aug 27 2019 at 06:11, on Zulip):

Sounds like it

oli (Aug 27 2019 at 06:11, on Zulip):

What code is causing the ICE?

Dylan MacKenzie (Aug 27 2019 at 06:14, on Zulip):

I don't have the error message in front of me now, but I'll have a look at it as well as the generated MIR tomorrow. Do you have any other tips for debugging promotion?

Dylan MacKenzie (Aug 27 2019 at 06:15, on Zulip):

(the name of the function being processed appears in the query stack)

oli (Aug 27 2019 at 06:43, on Zulip):

I believe we have some debug tracing in promote.rs

oli (Aug 27 2019 at 06:43, on Zulip):

I can have a look at your PR later, maybe I'll see sth

Dylan MacKenzie (Aug 27 2019 at 16:03, on Zulip):

https://github.com/rust-lang/rust/blob/5d20ff4d2718c820632b38c1e49d4de648a9810b/src/libcore/num/dec2flt/rawfp.rs#L260

Dylan MacKenzie (Aug 27 2019 at 16:03, on Zulip):

#0 [mir_borrowck] processing num::dec2flt::rawfp::round_normal

Dylan MacKenzie (Aug 27 2019 at 16:04, on Zulip):

Dylan MacKenzie (Aug 27 2019 at 16:07, on Zulip):

So there's the failing function body as well as a backtrace for good measure. Although there's no line number for the closure in `sanitize_place, I did verify what statement was failing

Dylan MacKenzie (Aug 27 2019 at 16:09, on Zulip):

Surprisingly, replacing the body of that function with unimplemented allows all of core to compile.

Dylan MacKenzie (Aug 27 2019 at 16:09, on Zulip):

Which implies that only that function triggers the problem.

oli (Aug 27 2019 at 16:54, on Zulip):

I'm assuming that removing the assert_eq will pass, too

oli (Aug 27 2019 at 16:55, on Zulip):

If so, you're somehow losing the argument marker for x.f

Dylan MacKenzie (Aug 27 2019 at 16:59, on Zulip):

Yes, commenting out assert_eq works as well

Dylan MacKenzie (Aug 27 2019 at 16:59, on Zulip):

Can you say more about argument markers @oli ?

Dylan MacKenzie (Aug 27 2019 at 17:02, on Zulip):

Do you mean that I'm trying to promote x.f even though it's a field of an argument?

Dylan MacKenzie (Aug 27 2019 at 17:04, on Zulip):

btw, I figured out -Zdump-mir=func_name so I have the MIR after promotion.

Dylan MacKenzie (Aug 27 2019 at 17:05, on Zulip):
        _63 = &(promoted[0]: (&'static str, u32, u32)); // bb7[4]: scope 4 at src/libcore/macros.rs:19:38: 19:92
        _62 = &(*_63);                   // bb7[5]: scope 4 at src/libcore/macros.rs:19:38: 19:92
        const panicking::panic_fmt(move _34, move _62) -> bb1; // bb7[6]: scope 4 at src/libcore/macros.rs:18:9: 19:93
Dylan MacKenzie (Aug 27 2019 at 17:06, on Zulip):

@oli do you think you could breifly explain the promoted[0] syntax to me?

Dylan MacKenzie (Aug 27 2019 at 17:07, on Zulip):

I'm guessing promoted[0] refers to the newly created mir::Body that computes the promoted value.

Dylan MacKenzie (Aug 27 2019 at 17:07, on Zulip):

And (&'static str, u32, u32) is the return type of that mir::Body?

Dylan MacKenzie (Aug 27 2019 at 17:12, on Zulip):

There's two promoteds, one holds a [&str; 3] with some strings for assert, and this one holds the filename, line number, and column number.

Dylan MacKenzie (Aug 27 2019 at 17:21, on Zulip):

The second promoted seems like it has almost the right number of locals to cause index is 7, len is 6

Dylan MacKenzie (Aug 27 2019 at 17:21, on Zulip):
promoted[1] in  num::dec2flt::rawfp::round_normal: [&str; 3] = {
    let mut _0: [&str; 3];               // return place in scope 0 at src/libcore/macros.rs:53:28: 55:17
    let mut _1: [&str; 3];               // in scope 0 at src/libcore/macros.rs:53:28: 55:17
    let mut _2: &str;                    // in scope 0 at src/libcore/macros.rs:53:28: 55:17
    let mut _3: &'static str;            // in scope 0 at src/libcore/macros.rs:53:28: 55:17
    let mut _4: &str;                    // in scope 0 at src/libcore/macros.rs:53:28: 55:17
    let mut _5: &'static str;            // in scope 0 at src/libcore/macros.rs:53:28: 55:17
    let mut _6: &str;                    // in scope 0 at src/libcore/macros.rs:53:28: 55:17
    let mut _7: &'static str;            // in scope 0 at src/libcore/macros.rs:53:28: 55:17
    scope 1 {
        scope 2 {
            scope 3 {
                scope 4 {
                    scope 5 {
                    }
                }
                scope 6 {
                }
            }
        }
    }

    bb0: {
        _3 = const "assertion failed: `(left == right)`\n  left: `"; // bb0[0]: scope 0 at src/libcore/macros.rs:53:28: 55:17
                                         // ty::Const
                                         // + ty: &'static str
                                         // + val: Slice { data: Allocation { bytes: [97, 115, 115, 101, 114, 116, 105, 111, 110, 32, 102, 97, 105, 108, 101, 100, 58, 32, 96, 40, 108, 101, 102, 116, 32, 61, 61, 32, 114, 105, 103, 104, 116, 41, 96, 10, 32, 32, 108, 101, 102, 116, 58, 32, 96], relocations: Relocations(SortedMap { data: [] }), undef_mask: UndefMask { blocks: [35184372088831], len: Size { raw: 45 } }, align: Align { pow2: 0 }, mutability: Immutable, extra: () }, start: 0, end: 45 }
                                         // mir::Constant
                                         // + span: src/libcore/macros.rs:53:28: 55:17
                                         // + ty: &'static str
                                         // + literal: Const { ty: &'static str, val: Slice { data: Allocation { bytes: [97, 115, 115, 101, 114, 116, 105, 111, 110, 32, 102, 97, 105, 108, 101, 100, 58, 32, 96, 40, 108, 101, 102, 116, 32, 61, 61, 32, 114, 105, 103, 104, 116, 41, 96, 10, 32, 32, 108, 101, 102, 116, 58, 32, 96], relocations: Relocations(SortedMap { data: [] }), undef_mask: UndefMask { blocks: [35184372088831], len: Size { raw: 45 } }, align: Align { pow2: 0 }, mutability: Immutable, extra: () }, start: 0, end: 45 } }
        _2 = &(*_3);                     // bb0[1]: scope 0 at src/libcore/macros.rs:53:28: 55:17
        _5 = const "`,\n right: `";      // bb0[2]: scope 0 at src/libcore/macros.rs:53:28: 55:17
                                         // ty::Const
                                         // + ty: &'static str
                                         // + val: Slice { data: Allocation { bytes: [96, 44, 10, 32, 114, 105, 103, 104, 116, 58, 32, 96], relocations: Relocations(SortedMap { data: [] }), undef_mask: UndefMask { blocks: [4095], len: Size { raw: 12 } }, align: Align { pow2: 0 }, mutability: Immutable, extra: () }, start: 0, end: 12 }
                                         // mir::Constant
                                         // + span: src/libcore/macros.rs:53:28: 55:17
                                         // + ty: &'static str
                                         // + literal: Const { ty: &'static str, val: Slice { data: Allocation { bytes: [96, 44, 10, 32, 114, 105, 103, 104, 116, 58, 32, 96], relocations: Relocations(SortedMap { data: [] }), undef_mask: UndefMask { blocks: [4095], len: Size { raw: 12 } }, align: Align { pow2: 0 }, mutability: Immutable, extra: () }, start: 0, end: 12 } }
        _4 = &(*_5);                     // bb0[3]: scope 0 at src/libcore/macros.rs:53:28: 55:17
        _7 = const "`";                  // bb0[4]: scope 0 at src/libcore/macros.rs:53:28: 55:17
                                         // ty::Const
                                         // + ty: &'static str
                                         // + val: Slice { data: Allocation { bytes: [96], relocations: Relocations(SortedMap { data: [] }), undef_mask: UndefMask { blocks: [1], len: Size { raw: 1 } }, align: Align { pow2: 0 }, mutability: Immutable, extra: () }, start: 0, end: 1 }
                                         // mir::Constant
                                         // + span: src/libcore/macros.rs:53:28: 55:17
                                         // + ty: &'static str
                                         // + literal: Const { ty: &'static str, val: Slice { data: Allocation { bytes: [96], relocations: Relocations(SortedMap { data: [] }), undef_mask: UndefMask { blocks: [1], len: Size { raw: 1 } }, align: Align { pow2: 0 }, mutability: Immutable, extra: () }, start: 0, end: 1 } }
        _6 = &(*_7);                     // bb0[5]: scope 0 at src/libcore/macros.rs:53:28: 55:17
        _1 = [move _2, move _4, move _6]; // bb0[6]: scope 0 at src/libcore/macros.rs:53:28: 55:17
        _0 = move _1;                    // bb0[7]: scope 0 at src/libcore/macros.rs:53:28: 55:17
        return;                          // bb0[8]: scope 0 at src/libcore/macros.rs:53:28: 55:17
    }
}
Dylan MacKenzie (Aug 27 2019 at 17:22, on Zulip):

Maybe something happens in a later pass?

Dylan MacKenzie (Aug 27 2019 at 17:29, on Zulip):

Also, after commenting out that line, building continues on to stage 1 std artifacts (the previous failure in core was from stage 0 std artifacts???) and fails on ptr::<impl cmp::Ord for *const T>::cmp

Dylan MacKenzie (Aug 27 2019 at 17:30, on Zulip):

I believe that is here https://github.com/rust-lang/rust/blob/0396aace27eea97c3603e9683e921807dff2a314/src/libcore/ptr/mod.rs#L2885

Dylan MacKenzie (Aug 27 2019 at 18:01, on Zulip):

No later pass removes a local AFAICT. I think I'll need more guidance to debug this.

RalfJ (Aug 27 2019 at 18:29, on Zulip):

this wouldnt have anything to do with the ICE I noted at https://github.com/rust-lang/rust/pull/63580#issuecomment-525164084, would it?

Wesley Wiser (Aug 27 2019 at 18:32, on Zulip):

I think that ICE is occurring because we're running the Inliner optimization pass. I need to investigate more though...

Dylan MacKenzie (Aug 27 2019 at 18:39, on Zulip):

@RalfJ Seems plausible. I can revert that PR and see if it helps

oli (Aug 27 2019 at 20:07, on Zulip):

We don't run mir opts on libcore I think

oli (Aug 27 2019 at 20:13, on Zulip):

Do you still have the mir of promoted 0 at hand?

Dylan MacKenzie (Aug 27 2019 at 20:29, on Zulip):

@oli I don't, and I'm having trouble getting x.py to resume from the right place.

Dylan MacKenzie (Aug 27 2019 at 20:30, on Zulip):

@oli Is there something specific you're looking for?

Dylan MacKenzie (Aug 27 2019 at 21:59, on Zulip):
// MIR for `num::dec2flt::rawfp::round_normal`
// source = MirSource { instance: Item(DefId(0:519 ~ core[b842]::num[0]::dec2flt[0]::rawfp[0]::round_normal[0])), promoted: Some(promoted[0]) }
// pass_name = QualifyAndPromoteConstants
// disambiguator = after

promoted[0] in  num::dec2flt::rawfp::round_normal: (&'static str, u32, u32) = {
    let mut _0: (&'static str, u32, u32); // return place in scope 0 at src/libcore/macros.rs:19:38: 19:92
    let mut _1: (&'static str, u32, u32); // in scope 0 at src/libcore/macros.rs:19:39: 19:92
    scope 1 {
        scope 2 {
            scope 3 {
                scope 4 {
                    scope 5 {
                    }
                }
                scope 6 {
                }
            }
        }
    }

    bb0: {
        _1 = (const "src/libcore/num/dec2flt/rawfp.rs", const 264u32, const 5u32); // bb0[0]: scope 0 at src/libcore/macros.rs:19:39: 19:92
                                         // ty::Const
                                         // + ty: &'static str
                                         // + val: Slice { data: Allocation { bytes: [115, 114, 99, 47, 108, 105, 98, 99, 111, 114, 101, 47, 110, 117, 109, 47, 100, 101, 99, 50, 102, 108, 116, 47, 114, 97, 119, 102, 112, 46, 114, 115], relocations: Relocations(SortedMap { data: [] }), undef_mask: UndefMask { blocks: [4294967295], len: Size { raw: 32 } }, align: Align { pow2: 0 }, mutability: Immutable, extra: () }, start: 0, end: 32 }
                                         // mir::Constant
                                         // + span: src/libcore/num/dec2flt/rawfp.rs:264:5: 264:40
                                         // + ty: &'static str
                                         // + literal: Const { ty: &'static str, val: Slice { data: Allocation { bytes: [115, 114, 99, 47, 108, 105, 98, 99, 111, 114, 101, 47, 110, 117, 109, 47, 100, 101, 99, 50, 102, 108, 116, 47, 114, 97, 119, 102, 112, 46, 114, 115], relocations: Relocations(SortedMap { data: [] }), undef_mask: UndefMask { blocks: [4294967295], len: Size { raw: 32 } }, align: Align { pow2: 0 }, mutability: Immutable, extra: () }, start: 0, end: 32 } }
                                         // ty::Const
                                         // + ty: u32
                                         // + val: Scalar(0x00000108)
                                         // mir::Constant
                                         // + span: src/libcore/num/dec2flt/rawfp.rs:264:5: 264:40
                                         // + ty: u32
                                         // + literal: Const { ty: u32, val: Scalar(0x00000108) }
                                         // ty::Const
                                         // + ty: u32
                                         // + val: Scalar(0x00000005)
                                         // mir::Constant
                                         // + span: src/libcore/num/dec2flt/rawfp.rs:264:5: 264:40
                                         // + ty: u32
                                         // + literal: Const { ty: u32, val: Scalar(0x00000005) }
        _0 = move _1;                    // bb0[1]: scope 0 at src/libcore/macros.rs:19:38: 19:92
        return;                          // bb0[2]: scope 0 at src/libcore/macros.rs:19:38: 19:92
    }
}
Dylan MacKenzie (Aug 27 2019 at 22:00, on Zulip):
// MIR for `num::dec2flt::rawfp::round_normal`
// source = MirSource { instance: Item(DefId(0:519 ~ core[b842]::num[0]::dec2flt[0]::rawfp[0]::round_normal[0])), promoted: Some(promoted[1]) }
// pass_name = QualifyAndPromoteConstants
// disambiguator = after

promoted[1] in  num::dec2flt::rawfp::round_normal: [&str; 3] = {
    let mut _0: [&str; 3];               // return place in scope 0 at src/libcore/macros.rs:53:28: 55:17
    let mut _1: [&str; 3];               // in scope 0 at src/libcore/macros.rs:53:28: 55:17
    let mut _2: &str;                    // in scope 0 at src/libcore/macros.rs:53:28: 55:17
    let mut _3: &'static str;            // in scope 0 at src/libcore/macros.rs:53:28: 55:17
    let mut _4: &str;                    // in scope 0 at src/libcore/macros.rs:53:28: 55:17
    let mut _5: &'static str;            // in scope 0 at src/libcore/macros.rs:53:28: 55:17
    let mut _6: &str;                    // in scope 0 at src/libcore/macros.rs:53:28: 55:17
    let mut _7: &'static str;            // in scope 0 at src/libcore/macros.rs:53:28: 55:17
    scope 1 {
        scope 2 {
            scope 3 {
                scope 4 {
                    scope 5 {
                    }
                }
                scope 6 {
                }
            }
        }
    }

    bb0: {
        _3 = const "assertion failed: `(left == right)`\n  left: `"; // bb0[0]: scope 0 at src/libcore/macros.rs:53:28: 55:17
                                         // ty::Const
                                         // + ty: &'static str
                                         // + val: Slice { data: Allocation { bytes: [97, 115, 115, 101, 114, 116, 105, 111, 110, 32, 102, 97, 105, 108, 101, 100, 58, 32, 96, 40, 108, 101, 102, 116, 32, 61, 61, 32, 114, 105, 103, 104, 116, 41, 96, 10, 32, 32, 108, 101, 102, 116, 58, 32, 96], relocations: Relocations(SortedMap { data: [] }), undef_mask: UndefMask { blocks: [35184372088831], len: Size { raw: 45 } }, align: Align { pow2: 0 }, mutability: Immutable, extra: () }, start: 0, end: 45 }
                                         // mir::Constant
                                         // + span: src/libcore/macros.rs:53:28: 55:17
                                         // + ty: &'static str
                                         // + literal: Const { ty: &'static str, val: Slice { data: Allocation { bytes: [97, 115, 115, 101, 114, 116, 105, 111, 110, 32, 102, 97, 105, 108, 101, 100, 58, 32, 96, 40, 108, 101, 102, 116, 32, 61, 61, 32, 114, 105, 103, 104, 116, 41, 96, 10, 32, 32, 108, 101, 102, 116, 58, 32, 96], relocations: Relocations(SortedMap { data: [] }), undef_mask: UndefMask { blocks: [35184372088831], len: Size { raw: 45 } }, align: Align { pow2: 0 }, mutability: Immutable, extra: () }, start: 0, end: 45 } }
        _2 = &(*_3);                     // bb0[1]: scope 0 at src/libcore/macros.rs:53:28: 55:17
        _5 = const "`,\n right: `";      // bb0[2]: scope 0 at src/libcore/macros.rs:53:28: 55:17
                                         // ty::Const
                                         // + ty: &'static str
                                         // + val: Slice { data: Allocation { bytes: [96, 44, 10, 32, 114, 105, 103, 104, 116, 58, 32, 96], relocations: Relocations(SortedMap { data: [] }), undef_mask: UndefMask { blocks: [4095], len: Size { raw: 12 } }, align: Align { pow2: 0 }, mutability: Immutable, extra: () }, start: 0, end: 12 }
                                         // mir::Constant
                                         // + span: src/libcore/macros.rs:53:28: 55:17
                                         // + ty: &'static str
                                         // + literal: Const { ty: &'static str, val: Slice { data: Allocation { bytes: [96, 44, 10, 32, 114, 105, 103, 104, 116, 58, 32, 96], relocations: Relocations(SortedMap { data: [] }), undef_mask: UndefMask { blocks: [4095], len: Size { raw: 12 } }, align: Align { pow2: 0 }, mutability: Immutable, extra: () }, start: 0, end: 12 } }
        _4 = &(*_5);                     // bb0[3]: scope 0 at src/libcore/macros.rs:53:28: 55:17
        _7 = const "`";                  // bb0[4]: scope 0 at src/libcore/macros.rs:53:28: 55:17
                                         // ty::Const
                                         // + ty: &'static str
                                         // + val: Slice { data: Allocation { bytes: [96], relocations: Relocations(SortedMap { data: [] }), undef_mask: UndefMask { blocks: [1], len: Size { raw: 1 } }, align: Align { pow2: 0 }, mutability: Immutable, extra: () }, start: 0, end: 1 }
                                         // mir::Constant
                                         // + span: src/libcore/macros.rs:53:28: 55:17
                                         // + ty: &'static str
                                         // + literal: Const { ty: &'static str, val: Slice { data: Allocation { bytes: [96], relocations: Relocations(SortedMap { data: [] }), undef_mask: UndefMask { blocks: [1], len: Size { raw: 1 } }, align: Align { pow2: 0 }, mutability: Immutable, extra: () }, start: 0, end: 1 } }
        _6 = &(*_7);                     // bb0[5]: scope 0 at src/libcore/macros.rs:53:28: 55:17
        _1 = [move _2, move _4, move _6]; // bb0[6]: scope 0 at src/libcore/macros.rs:53:28: 55:17
        _0 = move _1;                    // bb0[7]: scope 0 at src/libcore/macros.rs:53:28: 55:17
        return;                          // bb0[8]: scope 0 at src/libcore/macros.rs:53:28: 55:17
    }
}
Dylan MacKenzie (Aug 27 2019 at 22:01, on Zulip):

This is right after promotion

Dylan MacKenzie (Aug 27 2019 at 23:15, on Zulip):

@oli I have the mir dumps from all passes again, let me know what you want me to look for. I didn't see anything super obvious

Dylan MacKenzie (Aug 28 2019 at 00:17, on Zulip):

Same error occurs with Wesley Wiser's PR reverted

oli (Aug 28 2019 at 06:43, on Zulip):

this is super odd. if the len is 6, that means the max index is 5, so it can't be promoted[1]

oli (Aug 28 2019 at 06:44, on Zulip):

how many locals does the mir::Body of the function itself have?

oli (Aug 28 2019 at 06:44, on Zulip):

maybe we're not inside a promoted?

eddyb (Aug 28 2019 at 10:05, on Zulip):

@Dylan MacKenzie it would be a good idea to compare the two computation methods and ensure they always agree, like I do in my PR

Dylan MacKenzie (Aug 28 2019 at 15:14, on Zulip):

(deleted)

Dylan MacKenzie (Aug 28 2019 at 15:14, on Zulip):

@oli about 60 XD

oli (Aug 28 2019 at 15:14, on Zulip):

oh

oli (Aug 28 2019 at 15:15, on Zulip):

nevermind then

oli (Aug 28 2019 at 15:15, on Zulip):

no clue what's going on, this is very odd

Dylan MacKenzie (Aug 28 2019 at 15:50, on Zulip):

Okay, I'm gonna have a go at compare mode

Dylan MacKenzie (Aug 28 2019 at 15:51, on Zulip):

It will take some time though, so if you have ideas in the meantime I'm all ears

eddyb (Aug 28 2019 at 16:03, on Zulip):

@Dylan MacKenzie FWIW I meant just for the promotion_candidates list

Dylan MacKenzie (Aug 28 2019 at 16:10, on Zulip):

@eddyb I was planning on putting the old code into a module and calling old::Checker::check_const, then comparing the output

Dylan MacKenzie (Aug 28 2019 at 16:10, on Zulip):

This is about what you had in mind?

eddyb (Aug 28 2019 at 16:10, on Zulip):

I guess? I'm not sure how involved your changes are

Dylan MacKenzie (Aug 28 2019 at 16:13, on Zulip):

I guess I could try to run them as part of the same code path

Dylan MacKenzie (Aug 28 2019 at 16:13, on Zulip):

The whole hog approach might be quicker, I'll take a look today

Dylan MacKenzie (Aug 30 2019 at 03:45, on Zulip):

I've implemented a compare mode. It appears that promotion qualifs are not being properly detected when there's a Deref involved. I'll look into the underlying causes tomorrow.

Dylan MacKenzie (Aug 30 2019 at 18:37, on Zulip):

It turned out to be a simple bug: I wasn't initializing the dataflow state for the start block properly. As a result, I believe I was trying to promote some temporaries whose value came from an argument. I thought I would have seen this in the dumped MIR however (perhaps I was looking at stale output?). I'll make sure to check timestamps in the future.

Dylan MacKenzie (Aug 30 2019 at 18:38, on Zulip):

The dataflow-based code is still missing some candidates for promotion however. It appears to be static slices in complex CFGs. I'll keep debugging.

Dylan MacKenzie (Aug 30 2019 at 20:06, on Zulip):

I've figured out the source of the missed promotions.

Dylan MacKenzie (Aug 30 2019 at 20:09, on Zulip):

The flow-sensitive qualification code checks to see if the address of a local has been observed at a given program point. Slices in loops will always have their address observed at the definition site. The flow-sensitive code is very conservative, and if it sees a pointer write or a function call, it assumes that any local whose address is observable may have been mutated, and thus may now contain any qualif.

Dylan MacKenzie (Aug 30 2019 at 20:10, on Zulip):

https://github.com/rust-lang/rust/blob/ecca4b8c4bea678615175e1673ffcb647a94eb4a/src/libsyntax/attr/builtin.rs#L371

Dylan MacKenzie (Aug 30 2019 at 20:19, on Zulip):

@eddyb Since we know that the type &[&str] is always Freeze, we can assume that indirect writes don't mutate it. And a temporary like the RHS of let x: &'static [Cell<u32>] = &[] is not promotable anyway since it HasInteriorMut. Is this correct?

Dylan MacKenzie (Aug 30 2019 at 20:25, on Zulip):

If so, I guess we need to track whether a local has been mutably borrowed, or has been borrowed and is not Freeze.

oli (Aug 30 2019 at 21:38, on Zulip):

iirc we even promote &mut [], but that may just be inside static mut

oli (Aug 30 2019 at 21:41, on Zulip):

But in general: this is awesome news!

Dylan MacKenzie (Aug 30 2019 at 21:44, on Zulip):

@oli &mut [] is indeed promoted in non-const fns. We'll need to figure out a way to justify this inside CFGs with back-edges to be backwards compatible.

Dylan MacKenzie (Aug 30 2019 at 21:46, on Zulip):

Perhaps the answer is to wait for @eddyb s PR to get merged that separates promotion and const-qualification entirely

Dylan MacKenzie (Aug 30 2019 at 21:47, on Zulip):

I'm also running flow-based qualification for IsNotPromotable and IsNotImplicitlyPromotable, which is not necessary, so implementing a special TempOnlyQualifier that preserves the current behavior could make this go away.

Dylan MacKenzie (Aug 30 2019 at 21:59, on Zulip):

The tests should be running on CI now. We should see some regressions since I removed the code that replaces HasMutInterior with IsNotPromotable

Dylan MacKenzie (Aug 31 2019 at 06:10, on Zulip):

There's 18 failing tests, reflecting about 4-5 issues with my PR. I'm going to fix the trivial ones, then implementTempOnlyQualifier.

Dylan MacKenzie (Aug 31 2019 at 06:15, on Zulip):

I think the failure in https://github.com/rust-lang/rust/blob/master/src/test/ui/consts/const_arg_promotable.rs may be caused because we no longer swap HasMutInterior for IsNotPromotable. I don't know this for sure though. It's pretty easy to fix this particular case: simply check for HasMutInterior as well as IsNotPromotable when making sure args are promotable, but I suspect there's a deeper reason to exchange those two qualifs.

RalfJ (Aug 31 2019 at 07:09, on Zulip):

@Dylan MacKenzie btw, since you are working on const-qualif, your input on https://github.com/rust-rfcs/const-eval/pull/27 (and on the information in that repo in general) would be much appreciated :)

RalfJ (Aug 31 2019 at 07:10, on Zulip):

if there's things you learned that you think the next person working on const qualif should be able to just read somewhere, please add them there

lqd (Aug 31 2019 at 07:24, on Zulip):

(@RalfJ in case you don’t know, dylan is ecstatic-morse)

RalfJ (Aug 31 2019 at 09:03, on Zulip):

(I did not know that, thanks!)

eddyb (Aug 31 2019 at 12:31, on Zulip):

@Dylan MacKenzie yeaaaaaah crater takes forever though

ecstatic-morse (Sep 01 2019 at 00:28, on Zulip):

I changed my Zulip name to be consistent with my github. Someday I'll go full name everywhere, but not today XD

ecstatic-morse (Sep 01 2019 at 00:29, on Zulip):

I won't have a lot of time over the weekend to work on this, so expect some more progress on Tuesday

ecstatic-morse (Sep 04 2019 at 01:22, on Zulip):

After fixing some minor bugs, CI is now green for #63860. This version of the PR still includes my hack to enable &mut [] promotion within loops (I'm currently trying to devise a test which demonstrates its unsoundness), which can be removed once I stop using a flow-sensitive for promotion and go back to the temp-only one. I'll post an update here once that is implemented

ecstatic-morse (Sep 04 2019 at 19:59, on Zulip):

Okay, so the latest commit on #63860 contains a TempOnlyResolver that doesn't use dataflow, but instead applies the transfer function which underlies the dataflow analysis (expressed in QualifPropagator) linearly, similar to how the existing qualification scheme works.

ecstatic-morse (Sep 04 2019 at 20:00, on Zulip):

I've also removed the hack that enabled &mut [], so any lingering soundness holes are unknown to me.

ecstatic-morse (Sep 04 2019 at 20:01, on Zulip):

The latest version of this PR leaves intact the existing mutation of qualifs, and uses the compare function to log when their results differ from the new Resolver-based ones.

ecstatic-morse (Sep 04 2019 at 20:01, on Zulip):

However, only the Resolver based ones are actually used

ecstatic-morse (Sep 04 2019 at 20:03, on Zulip):

There are occasionally different results, but any difference I observe now is benign (usually a type being IsNotImplicitlyPromotable in addition to IsNotPromotable)

ecstatic-morse (Sep 04 2019 at 20:05, on Zulip):

It looks like no rustc tests fail due to the absence of the HasMutInterior -> IsNotPromotable replacement. I still have no idea what form a test case for this would take.

ecstatic-morse (Sep 04 2019 at 21:33, on Zulip):

@eddyb So I think we should have a sync discussion about the next steps here. There's a way to implement if and (eventually) loop in const contexts with minimal changes.Basically, we continue running the old-style propagation logic but overriding it with the results from the FlowSensitiveResolver if we're in a const context. I'm a bit worried about this, because it will exercise new, untested code paths in the existing checker now by traversing the entire mir::Body instead of stopping at SwitchInt.

Alternatively, we could also work towards a greenfield system that separates promotion and const-checking. I envision something roughly like this:
- Compute the dataflow results for HasMutInterior and NeedsDrop (if in a const context).
- Visit the mir::Body looking for violations of const safety (if in a const context). This pass would be much simpler than the existing const checking logic because it is no longer stateful: it doesn't need to keep track of promotable values and can simply use the dataflow cursor when it needs to query for HasMutInterior and NeedsDrop. The end-goal would be to unify this with the logic in qualify_min_const_fn.
- Compute the structural promotability of temps. This is the logic in promote_consts where we count the number of definitions of each temporary to see if they are eligible for promotion.
- Promote temps. If we are running in a const context, we can reuse dataflow results for HasMutInterior and NeedsDrop. Otherwise, we'll need to compute them on-demand using TempOnlyResolver. Then we propagate IsNotPromotable and IsNotImplicitlyPromotable, also using TempOnlyResolver.
- Finally, validate the arguments passed to functions with #[rustc_args_required_const(...)]. This could maybe be folded into the previous step.

Most of the work here would be in separating const-checking and promotion logic. You've already done some of this in #63812. We could run the newly separated pass in a compare-mode with the old one until we are happy with its correctness.

ecstatic-morse (Sep 14 2019 at 21:59, on Zulip):

I implemented this in #64470

ecstatic-morse (Sep 14 2019 at 21:59, on Zulip):

@eddyb ^

ecstatic-morse (Sep 15 2019 at 23:23, on Zulip):

@oli @RalfJ @eddyb I switched to a more conservative analysis for doing qualification in the presence of indirect writes (*p = x) that is closer to the pre-dataflow approach. However, it's kind of hacky as I explain in the most recent comment on #64470. Perhaps you'd like to weigh in on what we should do here long term to enable mutable borrows?

eddyb (Sep 16 2019 at 05:18, on Zulip):

ugh I'm going to lose my notifications if I jump right in here

eddyb (Sep 16 2019 at 05:18, on Zulip):

I can't spend too much time on this until the crater run finishes (I wish I never bothered with crater, it's so bad nowadays...)

eddyb (Sep 16 2019 at 05:19, on Zulip):

@ecstatic-morse but, what I think is that a non-frozen borrow should assume the worst about the contents of the local being borrowed

eddyb (Sep 16 2019 at 05:19, on Zulip):

that is, indirect writes can be ignored because they can't make it worse than what the borrow already assumed

ecstatic-morse (Sep 16 2019 at 05:21, on Zulip):

@eddyb Specifically, how do you want to handle const fn ident<T>(x: T) -> T { x }?

ecstatic-morse (Sep 16 2019 at 05:21, on Zulip):

x has to have HasMutInterior

ecstatic-morse (Sep 16 2019 at 05:23, on Zulip):

One way is to separate "can be mutated through a reference" from "is not Freeze".

ecstatic-morse (Sep 16 2019 at 05:23, on Zulip):

(right now const validation uses HasMutInterior for both)

ecstatic-morse (Sep 16 2019 at 05:29, on Zulip):

Anyways, I'll ping you once your crater run is done

Christian Poveda (Sep 16 2019 at 05:30, on Zulip):

hey, sorry to bother you? but what are you working on? I've been seeing the notifications for a couple weeks and I've grown curious about this :P

ecstatic-morse (Sep 16 2019 at 05:32, on Zulip):

@Christian Poveda The first comment on #63860 gives some background

Christian Poveda (Sep 16 2019 at 05:34, on Zulip):

Ohh ok thanks!

eddyb (Sep 16 2019 at 05:36, on Zulip):

I don't understand why that function is interesting at all

eddyb (Sep 16 2019 at 05:36, on Zulip):

it's just _0 = _1

eddyb (Sep 16 2019 at 05:37, on Zulip):

x has to have HasMutInterior

yes... and?

eddyb (Sep 16 2019 at 05:37, on Zulip):

if you take a reference to it, it could be used to mutate x

eddyb (Sep 16 2019 at 05:38, on Zulip):

One way is to separate "can be mutated through a reference" from "is not Freeze".

they... are the same thing tho

eddyb (Sep 16 2019 at 05:39, on Zulip):

I really don't know why the identity function is significant

ecstatic-morse (Sep 16 2019 at 05:40, on Zulip):

I mean within that body, no references exist to it, so it cannot be mutated through a reference

eddyb (Sep 16 2019 at 05:41, on Zulip):

yes it can!

eddyb (Sep 16 2019 at 05:41, on Zulip):

if you borrow it and pass it to some function even without any bounds it can use specialization to access some Cell inside

eddyb (Sep 16 2019 at 05:41, on Zulip):

but x could be runtime so this is largely irrelevant - it's unpromotable anyway

ecstatic-morse (Sep 16 2019 at 05:43, on Zulip):

Promotion is irrelevant here. The issue is NeedsDrop

eddyb (Sep 16 2019 at 05:44, on Zulip):

the risk of reinitialization?

ecstatic-morse (Sep 16 2019 at 05:44, on Zulip):

There's a drop Terminator at the end of ident

ecstatic-morse (Sep 16 2019 at 05:45, on Zulip):

So when we do _0 = _1, we clear NeedsDrop but not HasMutInterior

eddyb (Sep 16 2019 at 05:46, on Zulip):

wait but you can clear both

eddyb (Sep 16 2019 at 05:46, on Zulip):

wait are we worried about indirect accesses reinitializing something?

ecstatic-morse (Sep 16 2019 at 05:46, on Zulip):

Yes

eddyb (Sep 16 2019 at 05:47, on Zulip):

aaaaaaaah

eddyb (Sep 16 2019 at 05:47, on Zulip):

okay yeah for that you need "was ever borrowed" or "may have ever been borrowed before this point"

eddyb (Sep 16 2019 at 05:47, on Zulip):

the former is a decent approximation

eddyb (Sep 16 2019 at 05:48, on Zulip):

sorry, when you said "can be mutated through a reference" I thought there was implied that you'd know whether there was a reference

eddyb (Sep 16 2019 at 05:48, on Zulip):

HasMutInterior tells you that if you have a reference, it can be used to mutate even if it's not a mutable reference

ecstatic-morse (Sep 16 2019 at 05:48, on Zulip):

Yep sounds good. Btw this is not a blocker until we wanna unlock mutable references in const bodies

eddyb (Sep 16 2019 at 05:50, on Zulip):

this is confusing me a bit

eddyb (Sep 16 2019 at 05:50, on Zulip):

since I can't easily tell what depends on what

eddyb (Sep 16 2019 at 05:51, on Zulip):

@ecstatic-morse I guess the conservative thing to do for indirect accesses, function calls and InlineAsm, is to assume that anything that was borrowed mutably or sharedly with a !Frozen type, may be mutated

eddyb (Sep 16 2019 at 05:52, on Zulip):

you can't really rely on HasInteriorMut easily because HasInteriorMut can be affected by this... I think?

eddyb (Sep 16 2019 at 05:52, on Zulip):

or maybe NeedsDrop can

eddyb (Sep 16 2019 at 05:52, on Zulip):

cc @RalfJ I guess

eddyb (Sep 16 2019 at 05:53, on Zulip):

can a &Option<UnsafeCell<T>> ever go from None to Some, under any model?

eddyb (Sep 16 2019 at 05:54, on Zulip):

if not then I guess you can make anything borrow-related depend on HasInteriorMut

ecstatic-morse (Sep 16 2019 at 05:55, on Zulip):

Wow, Zulip is awful on Android :)

eddyb (Sep 16 2019 at 05:56, on Zulip):

be careful, it also drains my battery really fast

Christian Poveda (Sep 16 2019 at 05:56, on Zulip):

Wow, Zulip is awful on Android :)

yes... your messages arrive after 30 mins or so...

ecstatic-morse (Sep 16 2019 at 05:56, on Zulip):

There was an example that made me uneasy about using HasInteriorMut to track whether a local could be mutated indirectly, but I don't remember it off the top of my head.

eddyb (Sep 16 2019 at 05:56, on Zulip):

I have to hold the back button to kill the app and remove lag (which was noticeably in other apps) and not risk depleting my battery in a few minutes

ecstatic-morse (Sep 16 2019 at 05:59, on Zulip):

ecstatic-morse I guess the conservative thing to do for indirect accesses, function calls and InlineAsm, is to assume that anything that was borrowed mutably or sharedly with a !Frozen type, may be mutated

I think I will implement a dataflow pass that detects this, then check it along with NeedsDrop at Drop terminators.

ecstatic-morse (Sep 16 2019 at 06:00, on Zulip):

There's already HaveBeenBorrowedLocals; I just need a slight variation on this.

ecstatic-morse (Sep 16 2019 at 06:04, on Zulip):

if not then I guess you can make anything borrow-related depend on HasInteriorMut

ecstatic-morse (Sep 16 2019 at 06:06, on Zulip):

(This isn't an issue now, but may be in the future if we get more aggressive about clearing qualifs when locals are assigned to or moved out of)

RalfJ (Sep 16 2019 at 10:20, on Zulip):

can a &Option<UnsafeCell<T>> ever go from None to Some, under any model?

well... with &Option<UnsafeCell<bool>> I think Stacked Borrows lets it go from Some to None, yes. I also see no way (that Miri could implement) to forbid that.
enum layout optimizations are really annoying that way...

RalfJ (Sep 16 2019 at 10:31, on Zulip):

@eddyb yeah, see https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=2932bbee72bee2461e104410964f925d. interior mutability in Stacked Borrows is a per-location thing, so if the discriminant shares locations with the interior, it can be mutated...

oli (Sep 16 2019 at 11:14, on Zulip):

miri can overwrite the layout_of query and remove all layout optimizations :D

RalfJ (Sep 16 2019 at 11:20, on Zulip):

lol

RalfJ (Sep 16 2019 at 11:20, on Zulip):

that doesn't help for defining UB though ;)

oli (Sep 16 2019 at 11:32, on Zulip):

could miri record what value ranges may be written to an interior mut location?

RalfJ (Sep 16 2019 at 11:49, on Zulip):

maybe. in my nightmares.^^

oli (Sep 16 2019 at 11:55, on Zulip):

I assumed as much

ecstatic-morse (Sep 16 2019 at 23:56, on Zulip):

I'm now happy with the analysis in #64470. I'd like to split that PR up a bit, so that the generic dataflow engine and the new IndirectlyMutableLocals analysis can get a proper review. However, the code in the check_consts directory is ready to be reviewed in its current state.

ecstatic-morse (Sep 17 2019 at 00:01, on Zulip):

I also wanted to float the idea of putting the new, dataflow-based validation pass behind a feature gate. In other words, only enable it if #![feature(const_if)] or #![feature(const_loop)] is enabled. We can use the existing pass for promotion if we add a flag to it to disable the validation errors (the ones that are currently commented out on my PR). This would mean that #64470 could advance independently of #63812.

ecstatic-morse (Sep 17 2019 at 00:06, on Zulip):

I want to put the new qualification pass behind a feature flag because I'm worried about accidentally allowing a const operation that was previously banned. Such a change will be insta-stable unless the new pass is behind a flag. While a crater run will be able to tell us if my PR regresses currently valid code, there's not a corpus of currently broken const code beyond the test suite. Letting people play around with #![feature(const_if)] should give us some more data.

centril (Sep 17 2019 at 01:03, on Zulip):

sounds like a good idea to proceed carefully

ecstatic-morse (Sep 17 2019 at 05:27, on Zulip):

Oh btw let mut x: Option<Cell<i32>> = None; let p = &mut x; *p = Some(Cell::new(4)); is enough to require that we track HasMutInterior separate from IsIndirectlyMutable with the current rules. It wouldn't be too hard to extend them, but I think it's premature to try to combine the two concepts.

ecstatic-morse (Sep 17 2019 at 20:01, on Zulip):

@oli I'm gonna start splitting up my PR now. I'm also gonna rebase changes in check_consts since there's currently 3 different forms of the analysis in the git history (I'll keep the original series of commits on my fork)

ecstatic-morse (Sep 18 2019 at 00:58, on Zulip):

@oli @eddyb I rebased #64470 so that it can be reviewed commit-by-commit. Specifically the changes in qualifs.rs should be easier to review now. I also removed some of the unused code in resolver.rs that will be required to combine with #63812. Finally, I split out the dataflow portion of the PR in #64566 since there's a lot of design space there.

ecstatic-morse (Sep 18 2019 at 01:00, on Zulip):

I don't think I'm going to split out the addition of the IndirectlyMutableLocals dataflow pass into its own PR, since it's not very big and pretty closely coupled to this PR, so comment away if you see problems.

RalfJ (Sep 18 2019 at 21:00, on Zulip):

I want to put the new qualification pass behind a feature flag because I'm worried about accidentally allowing a const operation that was previously banned. Such a change will be insta-stable unless the new pass is behind a flag.

or we could have migration mode like we did for the borrow checker: run both, warn/ICE/whatever if they disagree.

centril (Sep 19 2019 at 00:43, on Zulip):

@RalfJ I both love (T-lang) and hate (T-release) that idea -- it's gonna be fun for rollups :P

RalfJ (Sep 19 2019 at 06:55, on Zulip):

yeah

RalfJ (Sep 19 2019 at 06:58, on Zulip):

well actually, I was not suggesting the thing with two runs of the test suite

RalfJ (Sep 19 2019 at 06:58, on Zulip):

I was suggesting the thing we do right now where we run both AST and NLL borrowck in production

ecstatic-morse (Sep 19 2019 at 20:47, on Zulip):

I was suggesting the thing we do right now where we run both AST and NLL borrowck in production

The current code always runs the new, dataflow-based validator but prevents it from emitting any errors. Then it compares the errors (Span plus fmt::Debug for the error struct) and ICEs if the sequence of errors is not identical with the old validator.

ecstatic-morse (Sep 19 2019 at 20:48, on Zulip):

I'm not really sure how to present this to the user. I guess we could emit a warning to nightly users saying "we messed something up, could you create an issue with your code so we can debug it"?

ecstatic-morse (Sep 19 2019 at 20:52, on Zulip):

Btw, I have a proof-of-concept branch that enables if and match in consts behind a feature flag so I can write better tests for the dataflow-based validator.

ecstatic-morse (Sep 19 2019 at 20:52, on Zulip):

https://github.com/ecstatic-morse/rust/tree/const-if

centril (Sep 19 2019 at 21:37, on Zulip):

I'm not really sure how to present this to the user. I guess we could emit a warning to nightly users saying "we messed something up, could you create an issue with your code so we can debug it"?

Isn't that just an ICE?

ecstatic-morse (Sep 19 2019 at 21:42, on Zulip):

I guess an ICE seems a little extreme to me, but y'all have more experience with this stuff than I do

centril (Sep 19 2019 at 21:48, on Zulip):

ICEs are a perfect fit for "we messed something up"

centril (Sep 19 2019 at 21:48, on Zulip):

that's why they exist :D

oli (Sep 20 2019 at 06:06, on Zulip):

not quite, in this case I think we'd want it as a future incompat warning

oli (Sep 20 2019 at 06:06, on Zulip):

if it's an ICE we'd be breaking user code

oli (Sep 20 2019 at 06:06, on Zulip):

we want to be able to crater it and to wait for ppl to tell us about their woes

centril (Sep 21 2019 at 13:00, on Zulip):

@oli sure, but after cratering we should make it an ICE

oli (Sep 21 2019 at 13:01, on Zulip):

crater takes too long

oli (Sep 21 2019 at 13:01, on Zulip):

I have made a different suggestion on the PR

oli (Sep 21 2019 at 13:01, on Zulip):

just only ICE on nightly for now :D

ecstatic-morse (Oct 01 2019 at 17:32, on Zulip):

I'm working on a fix for #64945 now. My local git repo for rust was corrupted somehow, so I'm trying to fix those concurrently.

ecstatic-morse (Oct 01 2019 at 19:24, on Zulip):

@oli @eddyb I opened #64967 to fix #64945. I'll continue investigating whether similar mismatches are possible.

ecstatic-morse (Oct 01 2019 at 19:31, on Zulip):

Also, I think IndirectlyMutableLocals is currently unsound in the presence of unsafe code and aggregates.

#[derive(Default)]
#[repr(C)]
struct PartialInteriorMut {
    a: i32,
    b: Cell<i32>,
}

fn main() {
    let x = PartialInteriorMut::default();
    let pa: *const i32 = &x.a ; // Doesn't cause `x` to get marked as indirectly mutable
    let pb: &Cell<i32> = unsafe { &*(pa.offset(1) as *const Cell<i32>) };
    pb.set(42);  // Mutates `x` indirectly even though `x` is not marked indirectly mutable!!!
}

Basically, we need to look at the type of the place base (not the type of the place after projections) to see whether to mark it as indirectly mutable.

RalfJ (Oct 09 2019 at 12:46, on Zulip):

that code is UB though according to current Stacked Borrows

RalfJ (Oct 09 2019 at 12:47, on Zulip):

but then this might or might not be what our final aliasing model says

RalfJ (Oct 09 2019 at 12:47, on Zulip):

mixed interior mut is a contentious case

RalfJ (Oct 09 2019 at 12:48, on Zulip):

oh I guess this is https://github.com/rust-lang/unsafe-code-guidelines/issues/134#issuecomment-538678208

RalfJ (Oct 09 2019 at 12:50, on Zulip):

in any case, good catch!

RalfJ (Oct 09 2019 at 12:50, on Zulip):

that comment makes me wonder if this is more about using raw pointers outside their original range, or partial interior mutability

RalfJ (Oct 09 2019 at 12:50, on Zulip):

it also raises some policy questions: if const-code has UB, do we care if our const-qualification analyses are incorrect?

Alexander Regueiro (Oct 12 2019 at 18:00, on Zulip):

what's the latest on this, out of curiosity? do we have some form of dataflow-based CTFE in rustc nightly yet?

ecstatic-morse (Oct 12 2019 at 18:29, on Zulip):

It was merged in #67440. See https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/Next.20steps.20for.20promotion.20and.20validation for the latest news.

Last update: Nov 15 2019 at 21:15UTC