Add some documentation for const eval and related topics #45

oli-obk · 2018-02-06T18:10:45Z

fixes #30

eddyb · 2018-02-06T18:48:41Z

src/param_env.md

@@ -0,0 +1,17 @@
+# Parameter Environment
+
+When working with associated constants or generic types it is often relevant to


This should be more general, maybe "generic and/or associated items (types, constants, functions/methods)"?

eddyb · 2018-02-06T18:50:34Z

src/param_env.md

+things.
+
+Another great thing about `ParamEnv` is that you can use it to eliminate the
+generic parameters from a `Ty` by calling `param_env.and(ty)`.


That eliminates irrelevant bounds from the param_env based on ty (or anything else with types in it, like Substs or Instance), without changing the ty. cc @nikomatsakis

Yes, this phrasing was confusing to me. Probably it makes sense to explain ParamEnvAnv a bit more -- basically, saying that often we need to pair up the environment with the thing we are operating on. When doing that, we use param_env.and(foo). If, however, foo doesn't refer to generic parameters, we'll swap in an empty environment since the where clauses shouldn't matter (I'm not sure if this behavior is worth mentioning though; it's kind of a detail and anyhow I expect it to change at some point).

mark-i-m

@oli-obk Thanks for the PR! This is a great addition to the book. I left a bunch of comments below. The content is great, but a little hard on the uninitiated (like me) :)

Let me know what you think!

mark-i-m · 2018-02-06T18:33:32Z

src/SUMMARY.md

@@ -19,5 +19,6 @@
    - [MIR construction](./mir-construction.md)
    - [MIR borrowck](./mir-borrowck.md)
    - [MIR optimizations](./mir-optimizations.md)
+- [Constant evaluation](./const-eval.md)


Could you also add ./miri and ./param_env to this file?

mark-i-m · 2018-02-06T18:34:51Z

src/glossary.md

@@ -17,6 +17,7 @@ ICE                     |  internal compiler error. When the compiler crashes.
 ICH                     |  incremental compilation hash. ICHs are used as fingerprints for things such as HIR and crate metadata, to check if changes have been made. This is useful in incremental compilation to see if part of a crate has changed and should be recompiled.
 infcx                   |  the inference context (see `librustc/infer`)
 MIR                     |  the Mid-level IR that is created after type-checking for use by borrowck and trans ([see more](./mir.html))
+miri                    |  an interpreter for MIR used for constant evaluation ([see more](./miri.html))


Perhaps add that "MIRI" is "MIR Interpreter"?

Also, nit: could you capitalize "MIRI"?

We've never capitalized it so far, just like rustc is never capitalized, but maybe we should?
@nikomatsakis @solson @oli-obk Opinions?

I've always used "miri" and "Miri", never "MIRI", though I guess that's an arbitrary aesthetic choice.

It particularly makes sense to go with lowercase miri when you're talking about a binary (like rustc).

It originated as an acronym though, so probably makes sense to capitalise it, even if we write it like an ordinary word in informal contexts.

It doubles as an acronym but was also named because it coincides with a human name. Plus writing it in all caps would be inconsistent with all existing documentation.

There's also that MIRI collides with another thing entirely

I prefer "miri" to MIRI. I'm hard put to justify this, but I guess that to me it doesn't feel like an "acronym", more like a "thing".

mark-i-m · 2018-02-06T18:35:44Z

src/miri.md

+Miri (**MIR** **I**nterpreter) is a virtual machine for executing MIR without
+compiling to machine code. It is usually invoked via `tcx.const_eval`.
+
+Its core datastructures can be found in `src/librustc/mir/interpret`. This is


Could you make this a live link to the source tree on github? That way, we can check if links break.

mark-i-m · 2018-02-06T18:41:57Z

src/miri.md

+## Miri interpretation
+
+Although the main entry point to constant evaluation is the `tcx.const_eval`
+query, there are additional functions in `src/librustc_mir/interpret/const_eval`


Could you make this a live link to the source tree on github? That way, we can check if links break.

mark-i-m · 2018-02-06T18:42:04Z

src/miri.md

+variables at the time of writing this guide.
+
+A stack frame is defined by the `Frame` type in
+`src/librustc_mir/interpret/eval_context.rs` and contains all the local


Could you make this a live link to the source tree on github? That way, we can check if links break.

mark-i-m · 2018-02-06T18:45:41Z

src/miri.md

@@ -0,0 +1,47 @@
+# Miri
+
+Miri (**MIR** **I**nterpreter) is a virtual machine for executing MIR without


It might be good to define what the model of the virtual machine is. For example, is it a stack-machine? Or does it have a flat byte-addressable memory? This would provide helpful context when discussing allocations and virtual memory below.

Those two are not exclusive, as many stack machines are also RAM machines (and if you're a RAM machine, adding a stack is easy).

However, miri isn't either of those, as each virtual allocation forms its own "address space", and the stack frames have fixed shapes (determined by the MIR of the fn body/const initializer being evaluated), instead of having more general data stack manipulation primitives.

So the stack is implemented more as a linked list of Frame in some sense (where each Frame is created based on what you are miri-ing)?

Well it's a stack of Frames within one evaluation - I don't believe nested Frames are ever created except for const fn calls - if you need the value of a different const, it would be evaluated separately, with an entirely different stack.

Got it :)

It would be good to add this info to the chapter.

mark-i-m · 2018-02-06T18:48:25Z

src/miri.md

+never have to access an `Allocation` directly except for translating it to the
+compilation target (atm just LLVM).
+
+### Details


I think we can remove this heading.

mark-i-m · 2018-02-06T18:50:28Z

src/miri.md

+anything that can't be representad as a `u64`) or `to_raw_bits` which results
+in an `Option<u128>` yielding the `ByVal` if possible.
+
+## Miri allocations


It wasn't 100% clear to me what role allocations play. IIUC, an allocation is just a virtual memory location that gets modified by step, right?

mark-i-m · 2018-02-06T21:58:55Z

src/miri.md

@@ -0,0 +1,47 @@
+# Miri


This is a great start!

One thing I would find helpful is an example. Something like,

const MY_CONST: usize = 1 << 12; let x = [0; MY_CONST]

would, I think, be sufficient. Maybe a walk through of evaluating the MIR to get the length of x would be good?

What do you think?

Nit: type Foo = [u8; MY_CONST]; may add less confusion, as nesting const inside a fn body can be misleading (even if it has no semantic effects).

Heck yes to an example! =) Maybe use a const fn to explain?

mark-i-m · 2018-02-06T22:00:07Z

src/const-eval.md

@@ -0,0 +1,32 @@
+# Constant Evaluation


It would be good to mention where in the compiler "pipeline" const eval happens. IIUC, it is after MIR is constructed and borrowck?

Note that the "pipeline" model is no longer entirely accurate. We care more about dependencies - and yes, miri requires MIR to be constructed to run on, but e.g. the type-checking, MIR construction and MIR evaluation of a constant might happen before any of those ever happen on a different constant/function/etc.

We have to to do all that to understand even the type [T; 8] - type-checking and working MIR is therefore reentrant.

Note that the "pipeline" model is no longer entirely accurate.

Yep, that's why I put it in "quotes" 😛 What I was getting at more was what dependencies const eval has, as you noted.

alexreg · 2018-02-07T00:00:12Z

It originated as an acronym though. I mean, that must have been how it was invented. Sure, the fact it sounds like a human name helps, but it’s worth mentioning where the acronym came from, even we use the ordinary name in all other places.

…

Sent from my iPhone

On 6 Feb 2018, at 23:56, Scott Olson ***@***.***> wrote: @solson commented on this pull request. In src/glossary.md: > @@ -17,6 +17,7 @@ ICE | internal compiler error. When the compiler crashes. ICH | incremental compilation hash. ICHs are used as fingerprints for things such as HIR and crate metadata, to check if changes have been made. This is useful in incremental compilation to see if part of a crate has changed and should be recompiled. infcx | the inference context (see `librustc/infer`) MIR | the Mid-level IR that is created after type-checking for use by borrowck and trans ([see more](./mir.html)) +miri | an interpreter for MIR used for constant evaluation ([see more](./miri.html)) It doubles as an acronym but was also named because it coincides with a human name. Plus writing it in all caps would be inconsistent with all existing documentation. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

alexreg · 2018-02-07T01:34:22Z

That has a completely different context. No likelihood of being confused...

…

On 7 Feb 2018, at 01:33, eternaleye ***@***.***> wrote: @eternaleye commented on this pull request. In src/glossary.md <#45 (comment)>: > @@ -17,6 +17,7 @@ ICE | internal compiler error. When the compiler crashes. ICH | incremental compilation hash. ICHs are used as fingerprints for things such as HIR and crate metadata, to check if changes have been made. This is useful in incremental compilation to see if part of a crate has changed and should be recompiled. infcx | the inference context (see `librustc/infer`) MIR | the Mid-level IR that is created after type-checking for use by borrowck and trans ([see more](./mir.html)) +miri | an interpreter for MIR used for constant evaluation ([see more](./miri.html)) There's also that MIRI collides with another thing entirely <https://intelligence.org/> — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#45 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEF3IOsIS6ME7jr0PPMK46zjRuWMqqwks5tSP1ggaJpZM4R7fcr>.

nikomatsakis · 2018-02-07T13:42:35Z

src/param_env.md

+# Parameter Environment
+
+When working with associated constants or generic types it is often relevant to
+have more information about the `Self` or generic parameters. Trait bounds and


I think the easiest way to explain the parameter environment is that it represents the set of where clauses in scope. I would also always try to tie to an example:

fn foo<T: Copy>(t: T) { }

when you request tcx.param_env(def_id_for_foo) you get back [T: Copy], basically.

nikomatsakis · 2018-02-07T13:43:20Z

src/param_env.md

+Although you can obtain a valid `ParamEnv` for any item via
+`tcx.param_env(def_id)`, this `ParamEnv` can be too generic for your use case.
+Using the `ParamEnv` from the surrounding context can allow you to evaluate more
+things.


I don't understand this sentence. It seems like you should use the param-env that is correct, and no other. =) Maybe the question is more about determining what is correct? Can you give a specific example of what you had in mind here?

The place I ran into issues with this was when evaluating promoted constants. If I ran tcx.param_env(promoted_const_def_id) and then evaluated the constant, I got type resolution problems left and right. If I used tcx.param_env(function_def_id) during the evaluation of the promoted, everything was good.

promoted_const_def_id doesn't exist though?

hmm... maybe it was associated constants and not promoteds.

I'm definitely using the param_env of the surrounding function when evaluating constants, not just when calling Instance::resolve.

The rule is that you use the ParamEnv for the body you're monomorphizing. So a function referring to a constant or anything else, will use whatever is in the MIR, substituted with the Substs and the ParamEnv (if needed) for the function.

That is, if you got the MIR for a def_id, typesystem things you get from that MIR are valid wrt tcx.param_env(def_id).subst(tcx, substs), after applying .subst(tcx, substs).

nikomatsakis

This is great @oli-obk -- I'll check back in when more of @mark-i-m's comments are addressed, since I think they said most of what I would say.

mark-i-m

This is great! Thanks :)

I left a few minor nits. Also, this chapter borders a bit on too much detail, but I think it is ok.

Besides the nits, LGTM 👍

mark-i-m · 2018-02-09T00:57:23Z

src/miri.md

+create items that use the type (locals, constants, function arguments, ...).
+
+To obtain the (in this case empty) parameter environment, one can call
+`let param_env = tcx.param_env(def_id);`. The `GlobalId` needed is


Here the def_id is that of FOO not Foo, right?

mark-i-m · 2018-02-09T00:58:51Z

src/miri.md

+        return;
+    }
+}
+```


Hmm... is the exact MIR likely to change over time?

this is already simplified, and unlikely to change much or in relevant ways.

mark-i-m · 2018-02-09T01:03:52Z

src/miri.md

+```
+
+Before the evaluation, a virtual memory location is created for storing the
+evaluation result, in this case a `vec![u8; size_of::<usize>()]`.


This sentence is a bit ambiguous. Does "in this case a vec..." refer to the result or the virtual memory location?

mark-i-m · 2018-02-09T01:20:45Z

src/miri.md

+fails, its error message is used for reporting a compile-time error.
+
+Since it does not fail, `Value::ByVal(PrimVal::Bytes(4054))` is stored in the
+virtual memory was allocated before th evaluation. `_0` always refers to that


s/th /the /

mark-i-m · 2018-02-09T01:22:05Z

src/miri.md

+virtual memory was allocated before th evaluation. `_0` always refers to that
+location directly.
+
+After the evaluation is done, the virtual memory allocation is interned into the


What does "interned" mean? Is that basically the memoization of a query?

yea... except in this case it's not strongly tied to a query

mark-i-m · 2018-02-09T01:24:18Z

src/miri.md

+## Datastructures
+
+Miri's core datastructures can be found in
+https://github.com/rust-lang/rust/blob/master/src/librustc/mir/interpret . This


I don't feel very strongly, but I would prefer something of the format [librustc/mir/interpret](https://github.com/rust-lang/rust/blob/master/src/librustc/mir/interpret)

@mark-i-m I think the convention in the rest of the guide is just to do e.g. src/librustc/mir/interpret (with the src/ prefix, note), and not link... I don't mind this way, but just thinking we should be consistent. :-)

src/librustc/mir/interpret (with the src/ prefix, note), and not link..

Generally, we have been trying to make links. That way, travis can warn us if they break.

alexreg · 2018-02-09T14:28:59Z

Okay. A find & replace is in order then!

…

Sent from my iPhone

On 9 Feb 2018, at 08:12, Oliver Schneider ***@***.***> wrote: @oli-obk commented on this pull request. In src/miri.md: > +`Value::ByVal(PrimVal::Undef)`. When the initialization of `_1` is invoked, the +value of the `FOO` constant is required, and triggers another call to +`tcx.const_eval`, which will not be shown here. If the evaluation of FOO is +successful, 42 will be subtracted by its value `4096` and the result stored in +`_1` as `Value::ByValPair(PrimVal::Bytes(4054), PrimVal::Bytes(0))`. The first +part of the pair is the computed value, the second part is a bool that's true if +an overflow happened. + +The next statement asserts that said boolean is `0`. In case the assertion +fails, its error message is used for reporting a compile-time error. + +Since it does not fail, `Value::ByVal(PrimVal::Bytes(4054))` is stored in the +virtual memory was allocated before th evaluation. `_0` always refers to that +location directly. + +After the evaluation is done, the virtual memory allocation is interned into the yea... except in this case it's not strongly tied to a query — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

oli-obk · 2018-02-09T14:34:51Z

replacing "interned" with "memoizated"? If we do that, we should probably be changing it in the source, too. There's a function called intern and various fields having interned in their name.

alexreg · 2018-02-09T14:45:31Z

replacing "interned" with "memoizated"? If we do that, we should probably be changing it in the source, too. There's a function called intern and various fields having interned in their name.

No, replacing src/foo.rs with [`src/foo.rs`](https://...).

eddyb · 2018-02-09T14:45:52Z

@oli-obk @alexreg Note that we have both memoization (of functions) and interning (of data).
We shouldn't really mix the two IMO.

oli-obk · 2018-02-09T14:55:21Z

Changed all the links to actual hyperlinks and tested them.

mark-i-m · 2018-02-12T18:32:09Z

Ooops. Sorry, I didn't realize the ball was in my court 😛

@nikomatsakis Did you also want to re-review this PR, or should I merge (#45 (review))?

@oli-obk It looks like a rebase is needed?

mark-i-m · 2018-02-17T19:59:21Z

Travis failures:

trait-resolution.md#436: Found "./param_env.md", did you mean "./param_env.html"?
const-eval.md#24: Found "./param_env.md", did you mean "./param_env.html"?
const-eval.md#33: Found "./miri.md", did you mean "./miri.html"?

oli-obk · 2018-02-20T12:20:59Z

Rebased and fixed links

mark-i-m · 2018-02-20T15:45:17Z

Ping @nikomatsakis did you want to rereview (as per your content above)?

mark-i-m · 2018-02-23T18:36:09Z

I think Niko is busy. Let's go ahead and merge this because I really want this chapter in the book. We can iterate on it later...

eddyb reviewed Feb 6, 2018

View reviewed changes

mark-i-m suggested changes Feb 6, 2018

View reviewed changes

mark-i-m mentioned this pull request Feb 6, 2018

Chapter on MIRI? #30

Closed

nikomatsakis reviewed Feb 7, 2018

View reviewed changes

mark-i-m mentioned this pull request Feb 7, 2018

document the test infrastructure #47

Merged

mark-i-m suggested changes Feb 9, 2018

View reviewed changes

mark-i-m approved these changes Feb 12, 2018

View reviewed changes

nikomatsakis mentioned this pull request Feb 12, 2018

master tracking issue #6

Closed

20 tasks

mark-i-m added the S-waiting-on-author Status: this PR is waiting for additional action by the OP label Feb 15, 2018

mark-i-m mentioned this pull request Feb 17, 2018

Clean up and reorganize traits chapter #52

Merged

Add some documentation for const eval and related topics

62cd099

oli-obk force-pushed the miri branch from 57224c5 to 62cd099 Compare February 20, 2018 12:20

mark-i-m added S-waiting-on-review Status: this PR is waiting for a reviewer to verify its content and removed S-waiting-on-author Status: this PR is waiting for additional action by the OP labels Feb 23, 2018

mark-i-m merged commit 6e1eccd into master Feb 23, 2018

mark-i-m deleted the miri branch February 28, 2018 20:48

		@@ -0,0 +1,17 @@
		# Parameter Environment

		When working with associated constants or generic types it is often relevant to

		@@ -0,0 +1,47 @@
		# Miri

		Miri (MIR Interpreter) is a virtual machine for executing MIR without

Add some documentation for const eval and related topics #45

Add some documentation for const eval and related topics #45

Conversation

oli-obk commented Feb 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-i-m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexreg commented Feb 7, 2018 via email

alexreg commented Feb 7, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

mark-i-m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexreg commented Feb 9, 2018 via email

oli-obk commented Feb 9, 2018

alexreg commented Feb 9, 2018 • edited Loading

eddyb commented Feb 9, 2018

oli-obk commented Feb 9, 2018

mark-i-m commented Feb 12, 2018

mark-i-m commented Feb 17, 2018

oli-obk commented Feb 20, 2018

mark-i-m commented Feb 20, 2018

mark-i-m commented Feb 23, 2018

alexreg commented Feb 9, 2018 •

edited

Loading