Comments, todos and small improvements

Author: nagisa

src/librustc_mir/transform/dataflow/combinators.rs

@@ -1,3 +1,12 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
 use rustc::mir::repr as mir;
 
 use std::marker::PhantomData;

src/librustc_mir/transform/dataflow/lattice.rs

@@ -1,3 +1,13 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
 use std::fmt::{Debug, Formatter};
 use std::collections::hash_map::Entry;
 use std::collections::HashMap;
@@ -140,7 +150,6 @@ impl<T: Debug> Debug for WBottom<T> {
 type WTopBottom<T> = WTop<WBottom<T>>;
 
 
-// TODO: should have wrapper, really, letting to pick between union or intersection..
 /// A hashmap lattice with union join operation.
 impl<K, T, H> Lattice for HashMap<K, T, H>
 where K: Clone + Eq + ::std::hash::Hash,

src/librustc_mir/transform/dataflow/mod.rs

@@ -1,3 +1,19 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+//! The interleaved, speculative MIR dataflow framework. This framework is heavily inspired by
+//! Hoopl[1][2], and while it has diverged from its inspiration quite a bit in implementation
+//! aproach, the general idea stays the same.
+//!
+//! [1]: https://github.com/haskell/hoopl
+//! [2]: http://research.microsoft.com/en-us/um/people/simonpj/papers/c--/hoopl-haskell10.pdf
+
 use rustc::mir::repr as mir;
 use rustc::mir::repr::{BasicBlock, BasicBlockData, Mir, START_BLOCK};
 use rustc_data_structures::bitvec::BitVector;
@@ -9,19 +25,31 @@ mod lattice;
 mod combinators;
 
 pub enum StatementChange<'tcx> {
+    /// Remove the statement being inspected.
     Remove,
+    /// Replace the statement with another (or the same) one.
     Statement(mir::Statement<'tcx>)
+    // FIXME: Should grow a way to do replacements with arbitrary graphs.
+    // Fixing this needs figuring out how to represent such an arbitrary graph in a way that could
+    // be both analysed (by and_then combinator, for example) and applied to the MIR/constructed
+    // out from MIR. Such representation needs to allow at least:
+    // * Adding new blocks, edges between blocks in the replacement as well as edges to the other
+    //   blocks in the MIR;
+    // * Adding new locals and perhaps changing existing ones (e.g. the type)
 }
 pub enum TerminatorChange<'tcx> {
+    /// Replace the terminator with another (or the same) one.
     Terminator(mir::Terminator<'tcx>)
+    // FIXME: Should grow a way to do replacements with arbitrary graphs.
 }
 
 pub trait Transfer<'tcx> {
+    /// The lattice used with this transfer function.
     type Lattice: Lattice;
 
     /// Return type of terminator transfer function.
     ///
-    /// `Vec<Lattice>` for forward analysis, `Lattice` for backward analysis.
+    /// `Vec<Self::Lattice>` for forward analysis, `Self::Lattice` for backward analysis.
     type TerminatorReturn;
 
     /// The transfer function which given a statement and a fact produces another fact which is
@@ -43,12 +71,19 @@ pub trait Rewrite<'tcx, T: Transfer<'tcx>> {
     -> TerminatorChange<'tcx>;
 
     /// Combine two rewrites using RewriteAndThen combinator.
+    ///
+    /// See documentation for the combinator for explanation of its behaviour.
     fn and_then<R2>(self, other: R2)
     -> RewriteAndThen<'tcx, T, Self, R2>
     where Self: Sized, R2: Rewrite<'tcx, T>
     {
         RewriteAndThen::new(self, other)
     }
+    // FIXME: should gain at least these combinators:
+    // * Fueled – only runs rewriter a set amount of times (needs saving the state of rewriter at
+    // certain points);
+    // * Deep – rewrite graph produced by rewriter with the same rewriter again;
+    // * maybe a wrapper to hold a tcx?
 }
 
 #[derive(Clone)]
@@ -61,7 +96,9 @@ pub struct Dataflow<'a, 'tcx: 'a, T, R>
 where T: Transfer<'tcx>, R: Rewrite<'tcx, T>
 {
     mir: &'a Mir<'tcx>,
-    // FIXME: bitvector may not be the best choice
+    // FIXME: bitvector may not be the best choice, I feel like using a FIFO queue should yield
+    // better results at some cost of space use. This queue needs to be a set (no duplicate
+    // entries), though, so plain linked-list based queue is not suitable.
     queue: BitVector,
     knowledge: IndexVec<BasicBlock, Option<Knowledge<'tcx, T::Lattice>>>,
     rewrite: R,
@@ -73,6 +110,7 @@ where L: Lattice,
       T: Transfer<'tcx, Lattice=L, TerminatorReturn=Vec<L>>,
       R: Rewrite<'tcx, T>
 {
+    /// Execute dataflow in forward direction
     pub fn forward(mir: &'a Mir<'tcx>, transfer: T, rewrite: R) -> Mir<'tcx> {
         let block_count = mir.basic_blocks().len();
         let mut queue = BitVector::new(block_count);
@@ -90,7 +128,6 @@ where L: Lattice,
     }
 
     fn forward_block(&mut self, bb: BasicBlock, mut fact: T::Lattice) {
-        // debug!("forward dataflow analysing {:?}", block);
         let bb_data = &self.mir[bb];
         let mut new_stmts = Vec::with_capacity(bb_data.statements.len());
         for stmt in &bb_data.statements {
@@ -108,6 +145,11 @@ where L: Lattice,
         };
         let successors = new_term.successors().into_owned();
         assert!(successors.len() == new_facts.len(), "new_facts.len() must match successor count");
+        // Replace block first and update facts after. This order is important, because updating
+        // facts for a block invalidates the block replacement. If you consider a case like a block
+        // having a backedge into itself, then this particular ordering will correctly invalidate
+        // the replacement block and put this block back into the queue for repeated analysis,
+        // whereas the swapped ordering would not invalidate the replacement at all.
         self.replace_block(bb, BasicBlockData {
             statements: new_stmts,
             terminator: Some(new_term),
@@ -126,6 +168,7 @@ where L: Lattice,
       T: Transfer<'tcx, Lattice=L, TerminatorReturn=L>,
       R: Rewrite<'tcx, T>
 {
+    /// Execute dataflow in backward direction.
     pub fn backward(mir: &'a Mir<'tcx>, transfer: T, rewrite: R) -> Mir<'tcx> {
         let block_count = mir.basic_blocks().len();
         let mut queue = BitVector::new(block_count);
@@ -184,9 +227,9 @@ where T: Transfer<'tcx>,
         // problematic, consider a constant propagation pass for a loop.
         //
         // → --- { idx = 1 } ---
-        // |    idx = idx + 1  # replaced with `idx = 2`
-        // |   if(...) break;  # consider else branch taken
-        // ↑ --- { idx = 2 } ---
+        // |    idx = idx + 1    # replaced with `idx = 2`
+        // |   if(...) break;    # consider else branch taken
+        // ↑ --- { idx = 2 } --- # backedge to the top
         //
         // Here once we analyse the backedge the fact {idx = 1} is joined with fact {idx = 2}
         // producing a Top ({idx = ⊤}) and rendering our replacement of `idx = idx + 1` with `idx =
@@ -208,15 +251,15 @@ where T: Transfer<'tcx>,
     fn update_fact(&mut self, bb: BasicBlock, new_fact: T::Lattice) -> bool {
         match self.knowledge[bb] {
             ref mut val@None => {
-                // In case of no prior knowledge about this block, we essentially introduce new
-                // data and thus return true.
+                // In case of no prior knowledge about this block, it means we are introducing new
+                // knowledge, and therefore, must return true.
                 *val = Some(Knowledge { fact: new_fact, new_block: None });
                 true
             },
             Some(Knowledge { ref mut fact, ref mut new_block }) => {
                 let join = T::Lattice::join(fact, new_fact);
-                // In case of some prior knowledge and provided our knowledge changed, we must
-                // invalidate any new_block that could exist.
+                // In case of some prior knowledge and provided our knowledge changed, we should
+                // invalidate any replacement block that could already exist.
                 if join {
                     *new_block = None;
                 }
@@ -235,6 +278,7 @@ where T: Transfer<'tcx>,
         }
     }
 
+    /// Build the new MIR by combining the replacement blocks and original MIR into a clone.
     fn construct_mir(mut self) -> Mir<'tcx> {
         let mut new_blocks = IndexVec::with_capacity(self.mir.basic_blocks().len());
         for (block, old_data) in self.mir.basic_blocks().iter_enumerated() {
@@ -261,8 +305,8 @@ where T: Transfer<'tcx>,
 }
 
 fn mir_exits<'tcx>(mir: &Mir<'tcx>, exits: &mut BitVector) {
-    // Do this smartly. First of all, find all the nodes without successors (these are guaranteed
-    // exit nodes).
+    // Do this smartly (cough… using bruteforce… cough). First of all, find all the nodes without
+    // successors. These are guaranteed exit nodes.
     let mir_len = mir.basic_blocks().len();
     let mut lead_to_exit = BitVector::new(mir_len);
     for (block, block_data) in mir.basic_blocks().iter_enumerated() {
@@ -284,8 +328,8 @@ fn mir_exits<'tcx>(mir: &Mir<'tcx>, exits: &mut BitVector) {
     // ```
     //
     // make it considerably more complex. In the end, it doesn’t matter very much which node we
-    // pick here, as picking any node does not make analysis incorrect, and a bad choice will only
-    // result in dataflow doing an extra pass over some of the blocks.
+    // pick here. Picking any node inside the loop will make dataflow visit all the nodes, only
+    // potentially doing an extra pass or two on a few blocks.
     lead_to_exit.invert();
     while let Some(exit) = lead_to_exit.pop() {
         if exit >= mir_len { continue }

src/librustc_mir/transform/liveness.rs

@@ -1,3 +1,12 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
 use rustc_data_structures::bitvec::BitVector;
 use rustc_data_structures::indexed_vec::Idx;
 use rustc::mir::repr::*;