Rollup merge of #95320 - JakobDegen:mir-docs, r=oli-obk

Document the current MIR semantics that are clear from existing code

This PR adds documentation to places, operands, rvalues, statementkinds, and terminatorkinds that describes their existing semantics and requirements. In many places the semantics depend on the Rust memory model or other T-Lang decisions - when this is the case, it is just noted as such with links to UCG issues where possible. I'm hopeful that none of the documentation added here can be used to justify optimizations that depend on the memory model. The documentation for places and operands probably comes closest to running afoul of this - if people think that it cannot be merged as is, it can definitely also be taken out.

The goal here is to only document parts of MIR that seem to be decided already, or are at least depended on by existing code. That leaves quite a number of open questions - those are marked as "needs clarification." I'm not sure what to do with those in this PR - we obviously can't decide all these questions here. Should I just leave them in as is? Take them out? Keep them in but as `//` instead of `///` comments?

If this is too big to review at once, I can split this up.

r? rust-lang/mir-opt

7 files changed, 633 insertions, 145 deletions

diff --git a/compiler/rustc_const_eval/src/transform/validate.rs b/compiler/rustc_const_eval/src/transform/validate.rs
index 58a7f6d1be0..01af9585135 100644
--- a/compiler/rustc_const_eval/src/transform/validate.rs
+++ b/compiler/rustc_const_eval/src/transform/validate.rs
@@ -3,15 +3,14 @@
 use rustc_index::bit_set::BitSet;
 use rustc_infer::infer::TyCtxtInferExt;
 use rustc_middle::mir::interpret::Scalar;
-use rustc_middle::mir::traversal;
 use rustc_middle::mir::visit::{PlaceContext, Visitor};
 use rustc_middle::mir::{
-    AggregateKind, BasicBlock, Body, BorrowKind, Local, Location, MirPass, MirPhase, Operand,
-    PlaceElem, PlaceRef, ProjectionElem, Rvalue, SourceScope, Statement, StatementKind, Terminator,
-    TerminatorKind, START_BLOCK,
+    traversal, AggregateKind, BasicBlock, BinOp, Body, BorrowKind, Local, Location, MirPass,
+    MirPhase, Operand, Place, PlaceElem, PlaceRef, ProjectionElem, Rvalue, SourceScope, Statement,
+    StatementKind, Terminator, TerminatorKind, UnOp, START_BLOCK,
 };
 use rustc_middle::ty::fold::BottomUpFolder;
-use rustc_middle::ty::{self, ParamEnv, Ty, TyCtxt, TypeFoldable};
+use rustc_middle::ty::{self, InstanceDef, ParamEnv, Ty, TyCtxt, TypeFoldable};
 use rustc_mir_dataflow::impls::MaybeStorageLive;
 use rustc_mir_dataflow::storage::AlwaysLiveLocals;
 use rustc_mir_dataflow::{Analysis, ResultsCursor};
@@ -36,6 +35,13 @@ pub struct Validator {
 
 impl<'tcx> MirPass<'tcx> for Validator {
     fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
+        // FIXME(JakobDegen): These bodies never instantiated in codegend anyway, so it's not
+        // terribly important that they pass the validator. However, I think other passes might
+        // still see them, in which case they might be surprised. It would probably be better if we
+        // didn't put this through the MIR pipeline at all.
+        if matches!(body.source.instance, InstanceDef::Intrinsic(..) | InstanceDef::Virtual(..)) {
+            return;
+        }
         let def_id = body.source.def_id();
         let param_env = tcx.param_env(def_id);
         let mir_phase = self.mir_phase;
@@ -240,58 +246,179 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
         self.super_projection_elem(local, proj_base, elem, context, location);
     }
 
-    fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) {
-        match &statement.kind {
-            StatementKind::Assign(box (dest, rvalue)) => {
-                // LHS and RHS of the assignment must have the same type.
-                let left_ty = dest.ty(&self.body.local_decls, self.tcx).ty;
-                let right_ty = rvalue.ty(&self.body.local_decls, self.tcx);
-                if !self.mir_assign_valid_types(right_ty, left_ty) {
+    fn visit_place(&mut self, place: &Place<'tcx>, _: PlaceContext, _: Location) {
+        // Set off any `bug!`s in the type computation code
+        let _ = place.ty(&self.body.local_decls, self.tcx);
+    }
+
+    fn visit_rvalue(&mut self, rvalue: &Rvalue<'tcx>, location: Location) {
+        macro_rules! check_kinds {
+            ($t:expr, $text:literal, $($patterns:tt)*) => {
+                if !matches!(($t).kind(), $($patterns)*) {
+                    self.fail(location, format!($text, $t));
+                }
+            };
+        }
+        match rvalue {
+            Rvalue::Use(_) => {}
+            Rvalue::Aggregate(agg_kind, _) => {
+                let disallowed = match **agg_kind {
+                    AggregateKind::Array(..) => false,
+                    AggregateKind::Generator(..) => self.mir_phase >= MirPhase::GeneratorsLowered,
+                    _ => self.mir_phase >= MirPhase::Deaggregated,
+                };
+                if disallowed {
                     self.fail(
                         location,
-                        format!(
-                            "encountered `{:?}` with incompatible types:\n\
-                            left-hand side has type: {}\n\
-                            right-hand side has type: {}",
-                            statement.kind, left_ty, right_ty,
-                        ),
+                        format!("{:?} have been lowered to field assignments", rvalue),
+                    )
+                }
+            }
+            Rvalue::Ref(_, BorrowKind::Shallow, _) => {
+                if self.mir_phase >= MirPhase::DropsLowered {
+                    self.fail(
+                        location,
+                        "`Assign` statement with a `Shallow` borrow should have been removed after drop lowering phase",
                     );
                 }
-                match rvalue {
-                    // The sides of an assignment must not alias. Currently this just checks whether the places
-                    // are identical.
-                    Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) => {
-                        if dest == src {
+            }
+            Rvalue::Len(p) => {
+                let pty = p.ty(&self.body.local_decls, self.tcx).ty;
+                check_kinds!(
+                    pty,
+                    "Cannot compute length of non-array type {:?}",
+                    ty::Array(..) | ty::Slice(..)
+                );
+            }
+            Rvalue::BinaryOp(op, vals) | Rvalue::CheckedBinaryOp(op, vals) => {
+                use BinOp::*;
+                let a = vals.0.ty(&self.body.local_decls, self.tcx);
+                let b = vals.1.ty(&self.body.local_decls, self.tcx);
+                match op {
+                    Offset => {
+                        check_kinds!(a, "Cannot offset non-pointer type {:?}", ty::RawPtr(..));
+                        if b != self.tcx.types.isize && b != self.tcx.types.usize {
+                            self.fail(location, format!("Cannot offset by non-isize type {:?}", b));
+                        }
+                    }
+                    Eq | Lt | Le | Ne | Ge | Gt => {
+                        for x in [a, b] {
+                            check_kinds!(
+                                x,
+                                "Cannot compare type {:?}",
+                                ty::Bool
+                                    | ty::Char
+                                    | ty::Int(..)
+                                    | ty::Uint(..)
+                                    | ty::Float(..)
+                                    | ty::RawPtr(..)
+                                    | ty::FnPtr(..)
+                            )
+                        }
+                        // None of the possible types have lifetimes, so we can just compare
+                        // directly
+                        if a != b {
                             self.fail(
                                 location,
-                                "encountered `Assign` statement with overlapping memory",
+                                format!("Cannot compare unequal types {:?} and {:?}", a, b),
                             );
                         }
                     }
-                    Rvalue::Aggregate(agg_kind, _) => {
-                        let disallowed = match **agg_kind {
-                            AggregateKind::Array(..) => false,
-                            AggregateKind::Generator(..) => {
-                                self.mir_phase >= MirPhase::GeneratorsLowered
-                            }
-                            _ => self.mir_phase >= MirPhase::Deaggregated,
-                        };
-                        if disallowed {
+                    Shl | Shr => {
+                        for x in [a, b] {
+                            check_kinds!(
+                                x,
+                                "Cannot shift non-integer type {:?}",
+                                ty::Uint(..) | ty::Int(..)
+                            )
+                        }
+                    }
+                    BitAnd | BitOr | BitXor => {
+                        for x in [a, b] {
+                            check_kinds!(
+                                x,
+                                "Cannot perform bitwise op on type {:?}",
+                                ty::Uint(..) | ty::Int(..) | ty::Bool
+                            )
+                        }
+                        if a != b {
                             self.fail(
                                 location,
-                                format!("{:?} have been lowered to field assignments", rvalue),
-                            )
+                                format!(
+                                    "Cannot perform bitwise op on unequal types {:?} and {:?}",
+                                    a, b
+                                ),
+                            );
                         }
                     }
-                    Rvalue::Ref(_, BorrowKind::Shallow, _) => {
-                        if self.mir_phase >= MirPhase::DropsLowered {
+                    Add | Sub | Mul | Div | Rem => {
+                        for x in [a, b] {
+                            check_kinds!(
+                                x,
+                                "Cannot perform op on type {:?}",
+                                ty::Uint(..) | ty::Int(..) | ty::Float(..)
+                            )
+                        }
+                        if a != b {
                             self.fail(
                                 location,
-                                "`Assign` statement with a `Shallow` borrow should have been removed after drop lowering phase",
+                                format!("Cannot perform op on unequal types {:?} and {:?}", a, b),
                             );
                         }
                     }
-                    _ => {}
+                }
+            }
+            Rvalue::UnaryOp(op, operand) => {
+                let a = operand.ty(&self.body.local_decls, self.tcx);
+                match op {
+                    UnOp::Neg => {
+                        check_kinds!(a, "Cannot negate type {:?}", ty::Int(..) | ty::Float(..))
+                    }
+                    UnOp::Not => {
+                        check_kinds!(
+                            a,
+                            "Cannot binary not type {:?}",
+                            ty::Int(..) | ty::Uint(..) | ty::Bool
+                        );
+                    }
+                }
+            }
+            Rvalue::ShallowInitBox(operand, _) => {
+                let a = operand.ty(&self.body.local_decls, self.tcx);
+                check_kinds!(a, "Cannot shallow init type {:?}", ty::RawPtr(..));
+            }
+            _ => {}
+        }
+        self.super_rvalue(rvalue, location);
+    }
+
+    fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) {
+        match &statement.kind {
+            StatementKind::Assign(box (dest, rvalue)) => {
+                // LHS and RHS of the assignment must have the same type.
+                let left_ty = dest.ty(&self.body.local_decls, self.tcx).ty;
+                let right_ty = rvalue.ty(&self.body.local_decls, self.tcx);
+                if !self.mir_assign_valid_types(right_ty, left_ty) {
+                    self.fail(
+                        location,
+                        format!(
+                            "encountered `{:?}` with incompatible types:\n\
+                            left-hand side has type: {}\n\
+                            right-hand side has type: {}",
+                            statement.kind, left_ty, right_ty,
+                        ),
+                    );
+                }
+                // FIXME(JakobDegen): Check this for all rvalues, not just this one.
+                if let Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) = rvalue {
+                    // The sides of an assignment must not alias. Currently this just checks whether
+                    // the places are identical.
+                    if dest == src {
+                        self.fail(
+                            location,
+                            "encountered `Assign` statement with overlapping memory",
+                        );
+                    }
                 }
             }
             StatementKind::AscribeUserType(..) => {
@@ -512,6 +639,9 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
                 }
             }
             TerminatorKind::Yield { resume, drop, .. } => {
+                if self.body.generator.is_none() {
+                    self.fail(location, "`Yield` cannot appear outside generator bodies");
+                }
                 if self.mir_phase >= MirPhase::GeneratorsLowered {
                     self.fail(location, "`Yield` should have been replaced by generator lowering");
                 }
@@ -551,6 +681,9 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
                 }
             }
             TerminatorKind::GeneratorDrop => {
+                if self.body.generator.is_none() {
+                    self.fail(location, "`GeneratorDrop` cannot appear outside generator bodies");
+                }
                 if self.mir_phase >= MirPhase::GeneratorsLowered {
                     self.fail(
                         location,
@@ -558,11 +691,19 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
                     );
                 }
             }
-            // Nothing to validate for these.
-            TerminatorKind::Resume
-            | TerminatorKind::Abort
-            | TerminatorKind::Return
-            | TerminatorKind::Unreachable => {}
+            TerminatorKind::Resume | TerminatorKind::Abort => {
+                let bb = location.block;
+                if !self.body.basic_blocks()[bb].is_cleanup {
+                    self.fail(location, "Cannot `Resume` or `Abort` from non-cleanup basic block")
+                }
+            }
+            TerminatorKind::Return => {
+                let bb = location.block;
+                if self.body.basic_blocks()[bb].is_cleanup {
+                    self.fail(location, "Cannot `Return` from cleanup basic block")
+                }
+            }
+            TerminatorKind::Unreachable => {}
         }
 
         self.super_terminator(terminator, location);
diff --git a/compiler/rustc_middle/src/lib.rs b/compiler/rustc_middle/src/lib.rs
index fa2dad5ce25..fd2b5f5335f 100644
--- a/compiler/rustc_middle/src/lib.rs
+++ b/compiler/rustc_middle/src/lib.rs
@@ -59,6 +59,7 @@
 #![feature(unwrap_infallible)]
 #![feature(decl_macro)]
 #![feature(drain_filter)]
+#![feature(intra_doc_pointers)]
 #![recursion_limit = "512"]
 #![allow(rustc::potential_query_instability)]
 
diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs
index 578fcd82ad6..9f7832c8a64 100644
--- a/compiler/rustc_middle/src/mir/mod.rs
+++ b/compiler/rustc_middle/src/mir/mod.rs
@@ -127,12 +127,24 @@ pub trait MirPass<'tcx> {
 /// The various "big phases" that MIR goes through.
 ///
 /// These phases all describe dialects of MIR. Since all MIR uses the same datastructures, the
-/// dialects forbid certain variants or values in certain phases.
+/// dialects forbid certain variants or values in certain phases. The sections below summarize the
+/// changes, but do not document them thoroughly. The full documentation is found in the appropriate
+/// documentation for the thing the change is affecting.
 ///
 /// Warning: ordering of variants is significant.
 #[derive(Copy, Clone, TyEncodable, TyDecodable, Debug, PartialEq, Eq, PartialOrd, Ord)]
 #[derive(HashStable)]
 pub enum MirPhase {
+    /// The dialect of MIR used during all phases before `DropsLowered` is the same. This is also
+    /// the MIR that analysis such as borrowck uses.
+    ///
+    /// One important thing to remember about the behavior of this section of MIR is that drop terminators
+    /// (including drop and replace) are *conditional*. The elaborate drops pass will then replace each
+    /// instance of a drop terminator with a nop, an unconditional drop, or a drop conditioned on a drop
+    /// flag. Of course, this means that it is important that the drop elaboration can accurately recognize
+    /// when things are initialized and when things are de-initialized. That means any code running on this
+    /// version of MIR must be sure to produce output that drop elaboration can reason about. See the
+    /// section on the drop terminatorss for more details.
     Built = 0,
     // FIXME(oli-obk): it's unclear whether we still need this phase (and its corresponding query).
     // We used to have this for pre-miri MIR based const eval.
@@ -162,6 +174,16 @@ pub enum MirPhase {
     /// And the following variant is allowed:
     /// * [`StatementKind::SetDiscriminant`]
     Deaggregated = 4,
+    /// Before this phase, generators are in the "source code" form, featuring `yield` statements
+    /// and such. With this phase change, they are transformed into a proper state machine. Running
+    /// optimizations before this change can be potentially dangerous because the source code is to
+    /// some extent a "lie." In particular, `yield` terminators effectively make the value of all
+    /// locals visible to the caller. This means that dead store elimination before them, or code
+    /// motion across them, is not correct in general. This is also exasperated by type checking
+    /// having pre-computed a list of the types that it thinks are ok to be live across a yield
+    /// point - this is necessary to decide eg whether autotraits are implemented. Introducing new
+    /// types across a yield point will lead to ICEs becaues of this.
+    ///
     /// Beginning with this phase, the following variants are disallowed:
     /// * [`TerminatorKind::Yield`](terminator::TerminatorKind::Yield)
     /// * [`TerminatorKind::GeneratorDrop](terminator::TerminatorKind::GeneratorDrop)
@@ -1573,18 +1595,45 @@ impl Statement<'_> {
 /// causing an ICE if they are violated.
 #[derive(Clone, Debug, PartialEq, TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable)]
 pub enum StatementKind<'tcx> {
-    /// Write the RHS Rvalue to the LHS Place.
+    /// Assign statements roughly correspond to an assignment in Rust proper (`x = ...`) except
+    /// without the possibility of dropping the previous value (that must be done separately, if at
+    /// all). The *exact* way this works is undecided. It probably does something like evaluating
+    /// the LHS to a place and the RHS to a value, and then storing the value to the place. Various
+    /// parts of this may do type specific things that are more complicated than simply copying
+    /// bytes.
+    ///
+    /// **Needs clarification**: The implication of the above idea would be that assignment implies
+    /// that the resulting value is initialized. I believe we could commit to this separately from
+    /// committing to whatever part of the memory model we would need to decide on to make the above
+    /// paragragh precise. Do we want to?
+    ///
+    /// Assignments in which the types of the place and rvalue differ are not well-formed.
+    ///
+    /// **Needs clarification**: Do we ever want to worry about non-free (in the body) lifetimes for
+    /// the typing requirement in post drop-elaboration MIR? I think probably not - I'm not sure we
+    /// could meaningfully require this anyway. How about free lifetimes? Is ignoring this
+    /// interesting for optimizations? Do we want to allow such optimizations?
     ///
-    /// The LHS place may not overlap with any memory accessed on the RHS.
+    /// **Needs clarification**: We currently require that the LHS place not overlap with any place
+    /// read as part of computation of the RHS for some rvalues (generally those not producing
+    /// primitives). This requirement is under discussion in [#68364]. As a part of this discussion,
+    /// it is also unclear in what order the components are evaluated.
+    ///
+    /// [#68364]: https://github.com/rust-lang/rust/issues/68364
+    ///
+    /// See [`Rvalue`] documentation for details on each of those.
     Assign(Box<(Place<'tcx>, Rvalue<'tcx>)>),
 
-    /// This represents all the reading that a pattern match may do
-    /// (e.g., inspecting constants and discriminant values), and the
-    /// kind of pattern it comes from. This is in order to adapt potential
-    /// error messages to these specific patterns.
+    /// This represents all the reading that a pattern match may do (e.g., inspecting constants and
+    /// discriminant values), and the kind of pattern it comes from. This is in order to adapt
+    /// potential error messages to these specific patterns.
     ///
     /// Note that this also is emitted for regular `let` bindings to ensure that locals that are
     /// never accessed still get some sanity checks for, e.g., `let x: ! = ..;`
+    ///
+    /// When executed at runtime this is a nop.
+    ///
+    /// Disallowed after drop elaboration.
     FakeRead(Box<(FakeReadCause, Place<'tcx>)>),
 
     /// Write the discriminant for a variant to the enum Place.
@@ -1599,17 +1648,35 @@ pub enum StatementKind<'tcx> {
     /// This writes `uninit` bytes to the entire place.
     Deinit(Box<Place<'tcx>>),
 
-    /// Start a live range for the storage of the local.
+    /// `StorageLive` and `StorageDead` statements mark the live range of a local.
+    ///
+    /// Using a local before a `StorageLive` or after a `StorageDead` is not well-formed. These
+    /// statements are not required. If the entire MIR body contains no `StorageLive`/`StorageDead`
+    /// statements for a particular local, the local is always considered live.
+    ///
+    /// More precisely, the MIR validator currently does a `MaybeStorageLiveLocals` analysis to
+    /// check validity of each use of a local. I believe this is equivalent to requiring for every
+    /// use of a local, there exist at least one path from the root to that use that contains a
+    /// `StorageLive` more recently than a `StorageDead`.
+    ///
+    /// **Needs clarification**: Is it permitted to have two `StorageLive`s without an intervening
+    /// `StorageDead`? Two `StorageDead`s without an intervening `StorageLive`? LLVM says poison,
+    /// yes. If the answer to any of these is "no," is breaking that rule UB or is it an error to
+    /// have a path in the CFG that might do this?
     StorageLive(Local),
 
-    /// End the current live range for the storage of the local.
+    /// See `StorageLive` above.
     StorageDead(Local),
 
-    /// Retag references in the given place, ensuring they got fresh tags. This is
-    /// part of the Stacked Borrows model. These statements are currently only interpreted
-    /// by miri and only generated when "-Z mir-emit-retag" is passed.
-    /// See <https://internals.rust-lang.org/t/stacked-borrows-an-aliasing-model-for-rust/8153/>
-    /// for more details.
+    /// Retag references in the given place, ensuring they got fresh tags.
+    ///
+    /// This is part of the Stacked Borrows model. These statements are currently only interpreted
+    /// by miri and only generated when `-Z mir-emit-retag` is passed. See
+    /// <https://internals.rust-lang.org/t/stacked-borrows-an-aliasing-model-for-rust/8153/> for
+    /// more details.
+    ///
+    /// For code that is not specific to stacked borrows, you should consider retags to read
+    /// and modify the place in an opaque way.
     Retag(RetagKind, Box<Place<'tcx>>),
 
     /// Encodes a user's type ascription. These need to be preserved
@@ -1624,6 +1691,10 @@ pub enum StatementKind<'tcx> {
     /// - `Contravariant` -- requires that `T_y :> T`
     /// - `Invariant` -- requires that `T_y == T`
     /// - `Bivariant` -- no effect
+    ///
+    /// When executed at runtime this is a nop.
+    ///
+    /// Disallowed after drop elaboration.
     AscribeUserType(Box<(Place<'tcx>, UserTypeProjection)>, ty::Variance),
 
     /// Marks the start of a "coverage region", injected with '-Cinstrument-coverage'. A
@@ -1633,9 +1704,19 @@ pub enum StatementKind<'tcx> {
     /// executed.
     Coverage(Box<Coverage>),
 
-    /// Denotes a call to the intrinsic function copy_overlapping, where `src_dst` denotes the
-    /// memory being read from and written to(one field to save memory), and size
-    /// indicates how many bytes are being copied over.
+    /// Denotes a call to the intrinsic function `copy_nonoverlapping`.
+    ///
+    /// First, all three operands are evaluated. `src` and `dest` must each be a reference, pointer,
+    /// or `Box` pointing to the same type `T`. `count` must evaluate to a `usize`. Then, `src` and
+    /// `dest` are dereferenced, and `count * size_of::<T>()` bytes beginning with the first byte of
+    /// the `src` place are copied to the continguous range of bytes beginning with the first byte
+    /// of `dest`.
+    ///
+    /// **Needs clarification**: In what order are operands computed and dereferenced? It should
+    /// probably match the order for assignment, but that is also undecided.
+    ///
+    /// **Needs clarification**: Is this typed or not, ie is there a typed load and store involved?
+    /// I vaguely remember Ralf saying somewhere that he thought it should not be.
     CopyNonOverlapping(Box<CopyNonOverlapping<'tcx>>),
 
     /// No-op. Useful for deleting instructions without affecting statement indices.
@@ -1785,8 +1866,82 @@ pub struct CopyNonOverlapping<'tcx> {
 ///////////////////////////////////////////////////////////////////////////
 // Places
 
-/// A path to a value; something that can be evaluated without
-/// changing or disturbing program state.
+/// Places roughly correspond to a "location in memory." Places in MIR are the same mathematical
+/// object as places in Rust. This of course means that what exactly they are is undecided and part
+/// of the Rust memory model. However, they will likely contain at least the following pieces of
+/// information in some form:
+///
+///  1. The address in memory that the place refers to.
+///  2. The provenance with which the place is being accessed.
+///  3. The type of the place and an optional variant index. See [`PlaceTy`][tcx::PlaceTy].
+///  4. Optionally, some metadata. This exists if and only if the type of the place is not `Sized`.
+///
+/// We'll give a description below of how all pieces of the place except for the provenance are
+/// calculated. We cannot give a description of the provenance, because that is part of the
+/// undecided aliasing model - we only include it here at all to acknowledge its existence.
+///
+/// Each local naturally corresponds to the place `Place { local, projection: [] }`. This place has
+/// the address of the local's allocation and the type of the local.
+///
+/// **Needs clarification:** Unsized locals seem to present a bit of an issue. Their allocation
+/// can't actually be created on `StorageLive`, because it's unclear how big to make the allocation.
+/// Furthermore, MIR produces assignments to unsized locals, although that is not permitted under
+/// `#![feature(unsized_locals)]` in Rust. Besides just putting "unsized locals are special and
+/// different" in a bunch of places, I (JakobDegen) don't know how to incorporate this behavior into
+/// the current MIR semantics in a clean way - possibly this needs some design work first.
+///
+/// For places that are not locals, ie they have a non-empty list of projections, we define the
+/// values as a function of the parent place, that is the place with its last [`ProjectionElem`]
+/// stripped. The way this is computed of course depends on the kind of that last projection
+/// element:
+///
+///  - [`Downcast`](ProjectionElem::Downcast): This projection sets the place's variant index to the
+///    given one, and makes no other changes. A `Downcast` projection on a place with its variant
+///    index already set is not well-formed.
+///  - [`Field`](ProjectionElem::Field): `Field` projections take their parent place and create a
+///    place referring to one of the fields of the type. The resulting address is the parent
+///    address, plus the offset of the field. The type becomes the type of the field. If the parent
+///    was unsized and so had metadata associated with it, then the metadata is retained if the
+///    field is unsized and thrown out if it is sized.
+///
+///    These projections are only legal for tuples, ADTs, closures, and generators. If the ADT or
+///    generator has more than one variant, the parent place's variant index must be set, indicating
+///    which variant is being used. If it has just one variant, the variant index may or may not be
+///    included - the single possible variant is inferred if it is not included.
+///  - [`ConstantIndex`](ProjectionElem::ConstantIndex): Computes an offset in units of `T` into the
+///    place as described in the documentation for the `ProjectionElem`. The resulting address is
+///    the parent's address plus that offset, and the type is `T`. This is only legal if the parent
+///    place has type `[T;  N]` or `[T]` (*not* `&[T]`). Since such a `T` is always sized, any
+///    resulting metadata is thrown out.
+///  - [`Subslice`](ProjectionElem::Subslice): This projection calculates an offset and a new
+///    address in a similar manner as `ConstantIndex`. It is also only legal on `[T; N]` and `[T]`.
+///    However, this yields a `Place` of type `[T]`, and additionally sets the metadata to be the
+///    length of the subslice.
+///  - [`Index`](ProjectionElem::Index): Like `ConstantIndex`, only legal on `[T; N]` or `[T]`.
+///    However, `Index` additionally takes a local from which the value of the index is computed at
+///    runtime. Computing the value of the index involves interpreting the `Local` as a
+///    `Place { local, projection: [] }`, and then computing its value as if done via
+///    [`Operand::Copy`]. The array/slice is then indexed with the resulting value. The local must
+///    have type `usize`.
+///  - [`Deref`](ProjectionElem::Deref): Derefs are the last type of projection, and the most
+///    complicated. They are only legal on parent places that are references, pointers, or `Box`. A
+///    `Deref` projection begins by loading a value from the parent place, as if by
+///    [`Operand::Copy`]. It then dereferences the resulting pointer, creating a place of the
+///    pointee's type. The resulting address is the address that was stored in the pointer. If the
+///    pointee type is unsized, the pointer additionally stored the value of the metadata.
+///
+/// Computing a place may cause UB. One possibility is that the pointer used for a `Deref` may not
+/// be suitably aligned. Another possibility is that the place is not in bounds, meaning it does not
+/// point to an actual allocation.
+///
+/// However, if this is actually UB and when the UB kicks in is undecided. This is being discussed
+/// in [UCG#319]. The options include that every place must obey those rules, that only some places
+/// must obey them, or that places impose no rules of their own.
+///
+/// [UCG#319]: https://github.com/rust-lang/unsafe-code-guidelines/issues/319
+///
+/// Rust currently requires that every place obey those two rules. This is checked by MIRI and taken
+/// advantage of by codegen (via `gep inbounds`). That is possibly subject to change.
 #[derive(Copy, Clone, PartialEq, Eq, Hash, TyEncodable, HashStable)]
 pub struct Place<'tcx> {
     pub local: Local,
@@ -2155,24 +2310,39 @@ pub struct SourceScopeLocalData {
 ///////////////////////////////////////////////////////////////////////////
 // Operands
 
-/// These are values that can appear inside an rvalue. They are intentionally
-/// limited to prevent rvalues from being nested in one another.
+/// An operand in MIR represents a "value" in Rust, the definition of which is undecided and part of
+/// the memory model. One proposal for a definition of values can be found [on UCG][value-def].
+///
+/// [value-def]: https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/value-domain.md
+///
+/// The most common way to create values is via loading a place. Loading a place is an operation
+/// which reads the memory of the place and converts it to a value. This is a fundamentally *typed*
+/// operation. The nature of the value produced depends on the type of the conversion. Furthermore,
+/// there may be other effects: if the type has a validity constraint loading the place might be UB
+/// if the validity constraint is not met.
+///
+/// **Needs clarification:** Ralf proposes that loading a place not have side-effects.
+/// This is what is implemented in miri today. Are these the semantics we want for MIR? Is this
+/// something we can even decide without knowing more about Rust's memory model?
+///
+/// **Needs clarifiation:** Is loading a place that has its variant index set well-formed? Miri
+/// currently implements it, but it seems like this may be something to check against in the
+/// validator.
 #[derive(Clone, PartialEq, TyEncodable, TyDecodable, Hash, HashStable)]
 pub enum Operand<'tcx> {
-    /// Copy: The value must be available for use afterwards.
-    ///
-    /// This implies that the type of the place must be `Copy`; this is true
-    /// by construction during build, but also checked by the MIR type checker.
+    /// Creates a value by loading the given place. The type of the place must be `Copy`
     Copy(Place<'tcx>),
 
-    /// Move: The value (including old borrows of it) will not be used again.
+    /// Creates a value by performing loading the place, just like the `Copy` operand.
+    ///
+    /// This *may* additionally overwrite the place with `uninit` bytes, depending on how we decide
+    /// in [UCG#188]. You should not emit MIR that may attempt a subsequent second load of this
+    /// place without first re-initializing it.
     ///
-    /// Safe for values of all types (modulo future developments towards `?Move`).
-    /// Correct usage patterns are enforced by the borrow checker for safe code.
-    /// `Copy` may be converted to `Move` to enable "last-use" optimizations.
+    /// [UCG#188]: https://github.com/rust-lang/unsafe-code-guidelines/issues/188
     Move(Place<'tcx>),
 
-    /// Synthesizes a constant value.
+    /// Constants are already semantically values, and remain unchanged.
     Constant(Box<Constant<'tcx>>),
 }
 
@@ -2280,57 +2450,134 @@ impl<'tcx> Operand<'tcx> {
 #[derive(Clone, TyEncodable, TyDecodable, Hash, HashStable, PartialEq)]
 /// The various kinds of rvalues that can appear in MIR.
 ///
-/// Not all of these are allowed at every [`MirPhase`]. Check the documentation there to see which
-/// ones you do not have to worry about. The MIR validator will generally enforce such restrictions,
-/// causing an ICE if they are violated.
+/// Not all of these are allowed at every [`MirPhase`] - when this is the case, it's stated below.
+///
+/// Computing any rvalue begins by evaluating the places and operands in some order (**Needs
+/// clarification**: Which order?). These are then used to produce a "value" - the same kind of
+/// value that an [`Operand`] produces.
 pub enum Rvalue<'tcx> {
-    /// x (either a move or copy, depending on type of x)
+    /// Yields the operand unchanged
     Use(Operand<'tcx>),
 
-    /// [x; 32]
+    /// Creates an array where each element is the value of the operand.
+    ///
+    /// This is the cause of a bug in the case where the repetition count is zero because the value
+    /// is not dropped, see [#74836].
+    ///
+    /// Corresponds to source code like `[x; 32]`.
+    ///
+    /// [#74836]: https://github.com/rust-lang/rust/issues/74836
     Repeat(Operand<'tcx>, ty::Const<'tcx>),
 
-    /// &x or &mut x
+    /// Creates a reference of the indicated kind to the place.
+    ///
+    /// There is not much to document here, because besides the obvious parts the semantics of this
+    /// are essentially entirely a part of the aliasing model. There are many UCG issues discussing
+    /// exactly what the behavior of this operation should be.
+    ///
+    /// `Shallow` borrows are disallowed after drop lowering.
     Ref(Region<'tcx>, BorrowKind, Place<'tcx>),
 
-    /// Accessing a thread local static. This is inherently a runtime operation, even if llvm
-    /// treats it as an access to a static. This `Rvalue` yields a reference to the thread local
-    /// static.
+    /// Creates a pointer/reference to the given thread local.
+    ///
+    /// The yielded type is a `*mut T` if the static is mutable, otherwise if the static is extern a
+    /// `*const T`, and if neither of those apply a `&T`.
+    ///
+    /// **Note:** This is a runtime operation that actually executes code and is in this sense more
+    /// like a function call. Also, eliminating dead stores of this rvalue causes `fn main() {}` to
+    /// SIGILL for some reason that I (JakobDegen) never got a chance to look into.
+    ///
+    /// **Needs clarification**: Are there weird additional semantics here related to the runtime
+    /// nature of this operation?
     ThreadLocalRef(DefId),
 
-    /// Create a raw pointer to the given place
-    /// Can be generated by raw address of expressions (`&raw const x`),
-    /// or when casting a reference to a raw pointer.
+    /// Creates a pointer with the indicated mutability to the place.
+    ///
+    /// This is generated by pointer casts like `&v as *const _` or raw address of expressions like
+    /// `&raw v` or `addr_of!(v)`.
+    ///
+    /// Like with references, the semantics of this operation are heavily dependent on the aliasing
+    /// model.
     AddressOf(Mutability, Place<'tcx>),
 
-    /// length of a `[X]` or `[X;n]` value
+    /// Yields the length of the place, as a `usize`.
+    ///
+    /// If the type of the place is an array, this is the array length. For slices (`[T]`, not
+    /// `&[T]`) this accesses the place's metadata to determine the length. This rvalue is
+    /// ill-formed for places of other types.
     Len(Place<'tcx>),
 
+    /// Performs essentially all of the casts that can be performed via `as`.
+    ///
+    /// This allows for casts from/to a variety of types.
+    ///
+    /// **FIXME**: Document exactly which `CastKind`s allow which types of casts. Figure out why
+    /// `ArrayToPointer` and `MutToConstPointer` are special.
     Cast(CastKind, Operand<'tcx>, Ty<'tcx>),
 
+    /// * `Offset` has the same semantics as [`offset`](pointer::offset), except that the second
+    ///   parameter may be a `usize` as well.
+    /// * The comparison operations accept `bool`s, `char`s, signed or unsigned integers, floats,
+    ///   raw pointers, or function pointers of matching types and return a `bool`.
+    /// * Left and right shift operations accept signed or unsigned integers not necessarily of the
+    ///   same type and return a value of the same type as their LHS. Like in Rust, the RHS is
+    ///   truncated as needed.
+    /// * The `Bit*` operations accept signed integers, unsigned integers, or bools with matching
+    ///   types and return a value of that type.
+    /// * The remaining operations accept signed integers, unsigned integers, or floats with
+    ///   matching types and return a value of that type.
     BinaryOp(BinOp, Box<(Operand<'tcx>, Operand<'tcx>)>),
+
+    /// Same as `BinaryOp`, but yields `(T, bool)` instead of `T`. In addition to performing the
+    /// same computation as the matching `BinaryOp`, checks if the infinite precison result would be
+    /// unequal to the actual result and sets the `bool` if this is the case.
+    ///
+    /// This only supports addition, subtraction, multiplication, and shift operations on integers.
     CheckedBinaryOp(BinOp, Box<(Operand<'tcx>, Operand<'tcx>)>),
 
+    /// Computes a value as described by the operation.
     NullaryOp(NullOp, Ty<'tcx>),
+
+    /// Exactly like `BinaryOp`, but less operands.
+    ///
+    /// Also does two's-complement arithmetic. Negation requires a signed integer or a float;
+    /// bitwise not requires a signed integer, unsigned integer, or bool. Both operation kinds
+    /// return a value with the same type as their operand.
     UnaryOp(UnOp, Operand<'tcx>),
 
-    /// Read the discriminant of an ADT.
+    /// Computes the discriminant of the place, returning it as an integer of type
+    /// [`discriminant_ty`].
+    ///
+    /// The validity requirements for the underlying value are undecided for this rvalue, see
+    /// [#91095]. Note too that the value of the discriminant is not the same thing as the
+    /// variant index; use [`discriminant_for_variant`] to convert.
+    ///
+    /// For types defined in the source code as enums, this is well behaved. This is also well
+    /// formed for other types, but yields no particular value - there is no reason it couldn't be
+    /// defined to yield eg zero though.
     ///
-    /// Undefined (i.e., no effort is made to make it defined, but there’s no reason why it cannot
-    /// be defined to return, say, a 0) if ADT is not an enum.
+    /// [`discriminant_ty`]: crate::ty::Ty::discriminant_ty
+    /// [#91095]: https://github.com/rust-lang/rust/issues/91095
+    /// [`discriminant_for_variant`]: crate::ty::Ty::discriminant_for_variant
     Discriminant(Place<'tcx>),
 
-    /// Creates an aggregate value, like a tuple or struct. This is
-    /// only needed because we want to distinguish `dest = Foo { x:
-    /// ..., y: ... }` from `dest.x = ...; dest.y = ...;` in the case
-    /// that `Foo` has a destructor. These rvalues can be optimized
-    /// away after type-checking and before lowering.
+    /// Creates an aggregate value, like a tuple or struct.
+    ///
+    /// This is needed because dataflow analysis needs to distinguish
+    /// `dest = Foo { x: ..., y: ... }` from `dest.x = ...; dest.y = ...;` in the case that `Foo`
+    /// has a destructor.
+    ///
+    /// Disallowed after deaggregation for all aggregate kinds except `Array` and `Generator`. After
+    /// generator lowering, `Generator` aggregate kinds are disallowed too.
     Aggregate(Box<AggregateKind<'tcx>>, Vec<Operand<'tcx>>),
 
     /// Transmutes a `*mut u8` into shallow-initialized `Box<T>`.
     ///
-    /// This is different a normal transmute because dataflow analysis will treat the box
-    /// as initialized but its content as uninitialized.
+    /// This is different from a normal transmute because dataflow analysis will treat the box as
+    /// initialized but its content as uninitialized. Like other pointer casts, this in general
+    /// affects alias analysis.
+    ///
+    /// Disallowed after drop elaboration.
     ShallowInitBox(Operand<'tcx>, Ty<'tcx>),
 }
 
diff --git a/compiler/rustc_middle/src/mir/tcx.rs b/compiler/rustc_middle/src/mir/tcx.rs
index 51d8113840a..597ade42236 100644
--- a/compiler/rustc_middle/src/mir/tcx.rs
+++ b/compiler/rustc_middle/src/mir/tcx.rs
@@ -76,6 +76,9 @@ impl<'tcx> PlaceTy<'tcx> {
         V: ::std::fmt::Debug,
         T: ::std::fmt::Debug + Copy,
     {
+        if self.variant_index.is_some() && !matches!(elem, ProjectionElem::Field(..)) {
+            bug!("cannot use non field projection on downcasted place")
+        }
         let answer = match *elem {
             ProjectionElem::Deref => {
                 let ty = self
diff --git a/compiler/rustc_middle/src/mir/terminator.rs b/compiler/rustc_middle/src/mir/terminator.rs
index ae94bd121f9..cc08857463d 100644
--- a/compiler/rustc_middle/src/mir/terminator.rs
+++ b/compiler/rustc_middle/src/mir/terminator.rs
@@ -105,13 +105,34 @@ impl<'a> Iterator for SwitchTargetsIter<'a> {
 
 impl<'a> ExactSizeIterator for SwitchTargetsIter<'a> {}
 
+/// A note on unwinding: Panics may occur during the execution of some terminators. Depending on the
+/// `-C panic` flag, this may either cause the program to abort or the call stack to unwind. Such
+/// terminators have a `cleanup: Option<BasicBlock>` field on them. If stack unwinding occurs, then
+/// once the current function is reached, execution continues at the given basic block, if any. If
+/// `cleanup` is `None` then no cleanup is performed, and the stack continues unwinding. This is
+/// equivalent to the execution of a `Resume` terminator.
+///
+/// The basic block pointed to by a `cleanup` field must have its `cleanup` flag set. `cleanup`
+/// basic blocks have a couple restrictions:
+///  1. All `cleanup` fields in them must be `None`.
+///  2. `Return` terminators are not allowed in them. `Abort` and `Unwind` terminators are.
+///  3. All other basic blocks (in the current body) that are reachable from `cleanup` basic blocks
+///     must also be `cleanup`. This is a part of the type system and checked statically, so it is
+///     still an error to have such an edge in the CFG even if it's known that it won't be taken at
+///     runtime.
 #[derive(Clone, TyEncodable, TyDecodable, Hash, HashStable, PartialEq)]
 pub enum TerminatorKind<'tcx> {
-    /// Block should have one successor in the graph; we jump there.
+    /// Block has one successor; we continue execution there.
     Goto { target: BasicBlock },
 
-    /// Operand evaluates to an integer; jump depending on its value
-    /// to one of the targets, and otherwise fallback to `otherwise`.
+    /// Switches based on the computed value.
+    ///
+    /// First, evaluates the `discr` operand. The type of the operand must be a signed or unsigned
+    /// integer, char, or bool, and must match the given type. Then, if the list of switch targets
+    /// contains the computed value, continues execution at the associated basic block. Otherwise,
+    /// continues execution at the "otherwise" basic block.
+    ///
+    /// Target values may not appear more than once.
     SwitchInt {
         /// The discriminant value being tested.
         discr: Operand<'tcx>,
@@ -124,29 +145,62 @@ pub enum TerminatorKind<'tcx> {
         targets: SwitchTargets,
     },
 
-    /// Indicates that the landing pad is finished and unwinding should
-    /// continue. Emitted by `build::scope::diverge_cleanup`.
+    /// Indicates that the landing pad is finished and that the process should continue unwinding.
+    ///
+    /// Like a return, this marks the end of this invocation of the function.
+    ///
+    /// Only permitted in cleanup blocks. `Resume` is not permitted with `-C unwind=abort` after
+    /// deaggregation runs.
     Resume,
 
-    /// Indicates that the landing pad is finished and that the process
-    /// should abort. Used to prevent unwinding for foreign items.
+    /// Indicates that the landing pad is finished and that the process should abort.
+    ///
+    /// Used to prevent unwinding for foreign items or with `-C unwind=abort`. Only permitted in
+    /// cleanup blocks.
     Abort,
 
-    /// Indicates a normal return. The return place should have
-    /// been filled in before this executes. This can occur multiple times
-    /// in different basic blocks.
+    /// Returns from the function.
+    ///
+    /// Like function calls, the exact semantics of returns in Rust are unclear. Returning very
+    /// likely at least assigns the value currently in the return place (`_0`) to the place
+    /// specified in the associated `Call` terminator in the calling function, as if assigned via
+    /// `dest = move _0`. It might additionally do other things, like have side-effects in the
+    /// aliasing model.
+    ///
+    /// If the body is a generator body, this has slightly different semantics; it instead causes a
+    /// `GeneratorState::Returned(_0)` to be created (as if by an `Aggregate` rvalue) and assigned
+    /// to the return place.
     Return,
 
     /// Indicates a terminator that can never be reached.
+    ///
+    /// Executing this terminator is UB.
     Unreachable,
 
-    /// Drop the `Place`.
+    /// The behavior of this statement differs significantly before and after drop elaboration.
+    /// After drop elaboration, `Drop` executes the drop glue for the specified place, after which
+    /// it continues execution/unwinds at the given basic blocks. It is possible that executing drop
+    /// glue is special - this would be part of Rust's memory model. (**FIXME**: due we have an
+    /// issue tracking if drop glue has any interesting semantics in addition to those of a function
+    /// call?)
+    ///
+    /// `Drop` before drop elaboration is a *conditional* execution of the drop glue. Specifically, the
+    /// `Drop` will be executed if...
+    ///
+    /// **Needs clarification**: End of that sentence. This in effect should document the exact
+    /// behavior of drop elaboration. The following sounds vaguely right, but I'm not quite sure:
+    ///
+    /// > The drop glue is executed if, among all statements executed within this `Body`, an assignment to
+    /// > the place or one of its "parents" occurred more recently than a move out of it. This does not
+    /// > consider indirect assignments.
     Drop { place: Place<'tcx>, target: BasicBlock, unwind: Option<BasicBlock> },
 
-    /// Drop the `Place` and assign the new value over it. This ensures
-    /// that the assignment to `P` occurs *even if* the destructor for
-    /// place unwinds. Its semantics are best explained by the
-    /// elaboration:
+    /// Drops the place and assigns a new value to it.
+    ///
+    /// This first performs the exact same operation as the pre drop-elaboration `Drop` terminator;
+    /// it then additionally assigns the `value` to the `place` as if by an assignment statement.
+    /// This assignment occurs both in the unwind and the regular code paths. The semantics are best
+    /// explained by the elaboration:
     ///
     /// ```
     /// BB0 {
@@ -170,7 +224,7 @@ pub enum TerminatorKind<'tcx> {
     /// }
     /// ```
     ///
-    /// Note that DropAndReplace is eliminated as part of the `ElaborateDrops` pass.
+    /// Disallowed after drop elaboration.
     DropAndReplace {
         place: Place<'tcx>,
         value: Operand<'tcx>,
@@ -178,7 +232,16 @@ pub enum TerminatorKind<'tcx> {
         unwind: Option<BasicBlock>,
     },
 
-    /// Block ends with a call of a function.
+    /// Roughly speaking, evaluates the `func` operand and the arguments, and starts execution of
+    /// the referred to function. The operand types must match the argument types of the function.
+    /// The return place type must match the return type. The type of the `func` operand must be
+    /// callable, meaning either a function pointer, a function type, or a closure type.
+    ///
+    /// **Needs clarification**: The exact semantics of this. Current backends rely on `move`
+    /// operands not aliasing the return place. It is unclear how this is justified in MIR, see
+    /// [#71117].
+    ///
+    /// [#71117]: https://github.com/rust-lang/rust/issues/71117
     Call {
         /// The function that’s being called.
         func: Operand<'tcx>,
@@ -187,7 +250,7 @@ pub enum TerminatorKind<'tcx> {
         /// This allows the memory occupied by "by-value" arguments to be
         /// reused across function calls without duplicating the contents.
         args: Vec<Operand<'tcx>>,
-        /// Destination for the return value. If some, the call is converging.
+        /// Destination for the return value. If none, the call necessarily diverges.
         destination: Option<(Place<'tcx>, BasicBlock)>,
         /// Cleanups to be done if the call unwinds.
         cleanup: Option<BasicBlock>,
@@ -199,8 +262,12 @@ pub enum TerminatorKind<'tcx> {
         fn_span: Span,
     },
 
-    /// Jump to the target if the condition has the expected value,
-    /// otherwise panic with a message and a cleanup target.
+    /// Evaluates the operand, which must have type `bool`. If it is not equal to `expected`,
+    /// initiates a panic. Initiating a panic corresponds to a `Call` terminator with some
+    /// unspecified constant as the function to call, all the operands stored in the `AssertMessage`
+    /// as parameters, and `None` for the destination. Keep in mind that the `cleanup` path is not
+    /// necessarily executed even in the case of a panic, for example in `-C panic=abort`. If the
+    /// assertion does not fail, execution continues at the specified basic block.
     Assert {
         cond: Operand<'tcx>,
         expected: bool,
@@ -209,7 +276,18 @@ pub enum TerminatorKind<'tcx> {
         cleanup: Option<BasicBlock>,
     },
 
-    /// A suspend point.
+    /// Marks a suspend point.
+    ///
+    /// Like `Return` terminators in generator bodies, this computes `value` and then a
+    /// `GeneratorState::Yielded(value)` as if by `Aggregate` rvalue. That value is then assigned to
+    /// the return place of the function calling this one, and execution continues in the calling
+    /// function. When next invoked with the same first argument, execution of this function
+    /// continues at the `resume` basic block, with the second argument written to the `resume_arg`
+    /// place. If the generator is dropped before then, the `drop` basic block is invoked.
+    ///
+    /// Not permitted in bodies that are not generator bodies, or after generator lowering.
+    ///
+    /// **Needs clarification**: What about the evaluation order of the `resume_arg` and `value`?
     Yield {
         /// The value to return.
         value: Operand<'tcx>,
@@ -221,11 +299,24 @@ pub enum TerminatorKind<'tcx> {
         drop: Option<BasicBlock>,
     },
 
-    /// Indicates the end of the dropping of a generator.
+    /// Indicates the end of dropping a generator.
+    ///
+    /// Semantically just a `return` (from the generators drop glue). Only permitted in the same situations
+    /// as `yield`.
+    ///
+    /// **Needs clarification**: Is that even correct? The generator drop code is always confusing
+    /// to me, because it's not even really in the current body.
+    ///
+    /// **Needs clarification**: Are there type system constraints on these terminators? Should
+    /// there be a "block type" like `cleanup` blocks for them?
     GeneratorDrop,
 
-    /// A block where control flow only ever takes one real path, but borrowck
-    /// needs to be more conservative.
+    /// A block where control flow only ever takes one real path, but borrowck needs to be more
+    /// conservative.
+    ///
+    /// At runtime this is semantically just a goto.
+    ///
+    /// Disallowed after drop elaboration.
     FalseEdge {
         /// The target normal control flow will take.
         real_target: BasicBlock,
@@ -233,9 +324,14 @@ pub enum TerminatorKind<'tcx> {
         /// practice.
         imaginary_target: BasicBlock,
     },
-    /// A terminator for blocks that only take one path in reality, but where we
-    /// reserve the right to unwind in borrowck, even if it won't happen in practice.
-    /// This can arise in infinite loops with no function calls for example.
+
+    /// A terminator for blocks that only take one path in reality, but where we reserve the right
+    /// to unwind in borrowck, even if it won't happen in practice. This can arise in infinite loops
+    /// with no function calls for example.
+    ///
+    /// At runtime this is semantically just a goto.
+    ///
+    /// Disallowed after drop elaboration.
     FalseUnwind {
         /// The target normal control flow will take.
         real_target: BasicBlock,
diff --git a/src/test/mir-opt/lower_intrinsics.rs b/src/test/mir-opt/lower_intrinsics.rs
index 8a8880dad02..eab51b65f1a 100644
--- a/src/test/mir-opt/lower_intrinsics.rs
+++ b/src/test/mir-opt/lower_intrinsics.rs
@@ -3,7 +3,7 @@
 #![crate_type = "lib"]
 
 // EMIT_MIR lower_intrinsics.wrapping.LowerIntrinsics.diff
-pub fn wrapping<T: Copy>(a: T, b: T) {
+pub fn wrapping(a: i32, b: i32) {
     let _x = core::intrinsics::wrapping_add(a, b);
     let _y = core::intrinsics::wrapping_sub(a, b);
     let _z = core::intrinsics::wrapping_mul(a, b);
diff --git a/src/test/mir-opt/lower_intrinsics.wrapping.LowerIntrinsics.diff b/src/test/mir-opt/lower_intrinsics.wrapping.LowerIntrinsics.diff
index a531a19bd78..5a0286bad2f 100644
--- a/src/test/mir-opt/lower_intrinsics.wrapping.LowerIntrinsics.diff
+++ b/src/test/mir-opt/lower_intrinsics.wrapping.LowerIntrinsics.diff
@@ -1,23 +1,23 @@
 - // MIR for `wrapping` before LowerIntrinsics
 + // MIR for `wrapping` after LowerIntrinsics
   
-  fn wrapping(_1: T, _2: T) -> () {
-      debug a => _1;                       // in scope 0 at $DIR/lower_intrinsics.rs:6:26: 6:27
-      debug b => _2;                       // in scope 0 at $DIR/lower_intrinsics.rs:6:32: 6:33
-      let mut _0: ();                      // return place in scope 0 at $DIR/lower_intrinsics.rs:6:38: 6:38
-      let _3: T;                           // in scope 0 at $DIR/lower_intrinsics.rs:7:9: 7:11
-      let mut _4: T;                       // in scope 0 at $DIR/lower_intrinsics.rs:7:45: 7:46
-      let mut _5: T;                       // in scope 0 at $DIR/lower_intrinsics.rs:7:48: 7:49
-      let mut _7: T;                       // in scope 0 at $DIR/lower_intrinsics.rs:8:45: 8:46
-      let mut _8: T;                       // in scope 0 at $DIR/lower_intrinsics.rs:8:48: 8:49
-      let mut _10: T;                      // in scope 0 at $DIR/lower_intrinsics.rs:9:45: 9:46
-      let mut _11: T;                      // in scope 0 at $DIR/lower_intrinsics.rs:9:48: 9:49
+  fn wrapping(_1: i32, _2: i32) -> () {
+      debug a => _1;                       // in scope 0 at $DIR/lower_intrinsics.rs:6:17: 6:18
+      debug b => _2;                       // in scope 0 at $DIR/lower_intrinsics.rs:6:25: 6:26
+      let mut _0: ();                      // return place in scope 0 at $DIR/lower_intrinsics.rs:6:33: 6:33
+      let _3: i32;                         // in scope 0 at $DIR/lower_intrinsics.rs:7:9: 7:11
+      let mut _4: i32;                     // in scope 0 at $DIR/lower_intrinsics.rs:7:45: 7:46
+      let mut _5: i32;                     // in scope 0 at $DIR/lower_intrinsics.rs:7:48: 7:49
+      let mut _7: i32;                     // in scope 0 at $DIR/lower_intrinsics.rs:8:45: 8:46
+      let mut _8: i32;                     // in scope 0 at $DIR/lower_intrinsics.rs:8:48: 8:49
+      let mut _10: i32;                    // in scope 0 at $DIR/lower_intrinsics.rs:9:45: 9:46
+      let mut _11: i32;                    // in scope 0 at $DIR/lower_intrinsics.rs:9:48: 9:49
       scope 1 {
           debug _x => _3;                  // in scope 1 at $DIR/lower_intrinsics.rs:7:9: 7:11
-          let _6: T;                       // in scope 1 at $DIR/lower_intrinsics.rs:8:9: 8:11
+          let _6: i32;                     // in scope 1 at $DIR/lower_intrinsics.rs:8:9: 8:11
           scope 2 {
               debug _y => _6;              // in scope 2 at $DIR/lower_intrinsics.rs:8:9: 8:11
-              let _9: T;                   // in scope 2 at $DIR/lower_intrinsics.rs:9:9: 9:11
+              let _9: i32;                 // in scope 2 at $DIR/lower_intrinsics.rs:9:9: 9:11
               scope 3 {
                   debug _z => _9;          // in scope 3 at $DIR/lower_intrinsics.rs:9:9: 9:11
               }
@@ -30,10 +30,10 @@
           _4 = _1;                         // scope 0 at $DIR/lower_intrinsics.rs:7:45: 7:46
           StorageLive(_5);                 // scope 0 at $DIR/lower_intrinsics.rs:7:48: 7:49
           _5 = _2;                         // scope 0 at $DIR/lower_intrinsics.rs:7:48: 7:49
--         _3 = wrapping_add::<T>(move _4, move _5) -> bb1; // scope 0 at $DIR/lower_intrinsics.rs:7:14: 7:50
+-         _3 = wrapping_add::<i32>(move _4, move _5) -> bb1; // scope 0 at $DIR/lower_intrinsics.rs:7:14: 7:50
 -                                          // mir::Constant
 -                                          // + span: $DIR/lower_intrinsics.rs:7:14: 7:44
--                                          // + literal: Const { ty: extern "rust-intrinsic" fn(T, T) -> T {wrapping_add::<T>}, val: Value(Scalar(<ZST>)) }
+-                                          // + literal: Const { ty: extern "rust-intrinsic" fn(i32, i32) -> i32 {wrapping_add::<i32>}, val: Value(Scalar(<ZST>)) }
 +         _3 = Add(move _4, move _5);      // scope 0 at $DIR/lower_intrinsics.rs:7:14: 7:50
 +         goto -> bb1;                     // scope 0 at $DIR/lower_intrinsics.rs:7:14: 7:50
       }
@@ -46,10 +46,10 @@
           _7 = _1;                         // scope 1 at $DIR/lower_intrinsics.rs:8:45: 8:46
           StorageLive(_8);                 // scope 1 at $DIR/lower_intrinsics.rs:8:48: 8:49
           _8 = _2;                         // scope 1 at $DIR/lower_intrinsics.rs:8:48: 8:49
--         _6 = wrapping_sub::<T>(move _7, move _8) -> bb2; // scope 1 at $DIR/lower_intrinsics.rs:8:14: 8:50
+-         _6 = wrapping_sub::<i32>(move _7, move _8) -> bb2; // scope 1 at $DIR/lower_intrinsics.rs:8:14: 8:50
 -                                          // mir::Constant
 -                                          // + span: $DIR/lower_intrinsics.rs:8:14: 8:44
--                                          // + literal: Const { ty: extern "rust-intrinsic" fn(T, T) -> T {wrapping_sub::<T>}, val: Value(Scalar(<ZST>)) }
+-                                          // + literal: Const { ty: extern "rust-intrinsic" fn(i32, i32) -> i32 {wrapping_sub::<i32>}, val: Value(Scalar(<ZST>)) }
 +         _6 = Sub(move _7, move _8);      // scope 1 at $DIR/lower_intrinsics.rs:8:14: 8:50
 +         goto -> bb2;                     // scope 1 at $DIR/lower_intrinsics.rs:8:14: 8:50
       }
@@ -62,10 +62,10 @@
           _10 = _1;                        // scope 2 at $DIR/lower_intrinsics.rs:9:45: 9:46
           StorageLive(_11);                // scope 2 at $DIR/lower_intrinsics.rs:9:48: 9:49
           _11 = _2;                        // scope 2 at $DIR/lower_intrinsics.rs:9:48: 9:49
--         _9 = wrapping_mul::<T>(move _10, move _11) -> bb3; // scope 2 at $DIR/lower_intrinsics.rs:9:14: 9:50
+-         _9 = wrapping_mul::<i32>(move _10, move _11) -> bb3; // scope 2 at $DIR/lower_intrinsics.rs:9:14: 9:50
 -                                          // mir::Constant
 -                                          // + span: $DIR/lower_intrinsics.rs:9:14: 9:44
--                                          // + literal: Const { ty: extern "rust-intrinsic" fn(T, T) -> T {wrapping_mul::<T>}, val: Value(Scalar(<ZST>)) }
+-                                          // + literal: Const { ty: extern "rust-intrinsic" fn(i32, i32) -> i32 {wrapping_mul::<i32>}, val: Value(Scalar(<ZST>)) }
 +         _9 = Mul(move _10, move _11);    // scope 2 at $DIR/lower_intrinsics.rs:9:14: 9:50
 +         goto -> bb3;                     // scope 2 at $DIR/lower_intrinsics.rs:9:14: 9:50
       }
@@ -73,7 +73,7 @@
       bb3: {
           StorageDead(_11);                // scope 2 at $DIR/lower_intrinsics.rs:9:49: 9:50
           StorageDead(_10);                // scope 2 at $DIR/lower_intrinsics.rs:9:49: 9:50
-          _0 = const ();                   // scope 0 at $DIR/lower_intrinsics.rs:6:38: 10:2
+          _0 = const ();                   // scope 0 at $DIR/lower_intrinsics.rs:6:33: 10:2
           StorageDead(_9);                 // scope 2 at $DIR/lower_intrinsics.rs:10:1: 10:2
           StorageDead(_6);                 // scope 1 at $DIR/lower_intrinsics.rs:10:1: 10:2
           StorageDead(_3);                 // scope 0 at $DIR/lower_intrinsics.rs:10:1: 10:2