Comment changes in forkify and phi_elim

hercules_ir/Cargo.toml

 [package]
 name = "hercules_ir"
 version = "0.1.0"
-authors = ["Russel Arbore <rarbore2@illinois.edu>"]
+authors = ["Russel Arbore <rarbore2@illinois.edu>, Aaron Councilman <aaronjc4@illinois.edu>"]
 
 [dependencies]
 rand = "*"

hercules_opt/Cargo.toml

 [package]
 name = "hercules_opt"
 version = "0.1.0"
-authors = ["Russel Arbore <rarbore2@illinois.edu>"]
+authors = ["Russel Arbore <rarbore2@illinois.edu>, Aaron Councilman <aaronjc4@illinois.edu>"]
 
 [dependencies]
 ordered-float = "*"

hercules_opt/src/forkify.rs

@@ -17,7 +17,9 @@ pub fn forkify(
     def_use: &ImmutableDefUseMap,
     loops: &LoopTree,
 ) {
-    // Ignore loops that are already fork-joins.
+    // Ignore loops that are already fork-joins. TODO: re-calculate def_use per
+    // loop, since it's technically invalidated after each individual loop
+    // modification.
     let natural_loops = loops
         .bottom_up_loops()
         .into_iter()

hercules_opt/src/phi_elim.rs

-/* A Hercules IR transformation that 
+extern crate hercules_ir;
+
+use std::collections::HashMap;
+
+use self::hercules_ir::get_uses_mut;
+use self::hercules_ir::ir::*;
+
+/*
+ * This is a Hercules IR transformation that:
  * - Eliminates phi nodes where all inputs are the same (here this means the
- *   same node in IR, we are not performing GVM, SCCP, or any similar
- *   optimization)
- * - Eliminate regions with only a single predecessor
+ *   same node in IR, GVN and related are separate).
+ * - Eliminate regions with only a single predecessor.
  *
  * The first of these optimizations is inspired by the description of the SSA
  * construction algorithm in Braun et al., CC 2013 used by the front-end. This
  * optimization performs the phi removal suggested by that paper, as in our
- * construction algorithm performing it during code generation is difficult.
+ * construction algorithm performing it during IR code generation is difficult.
  */
-extern crate hercules_ir;
-
-use std::collections::HashMap;
-
-use self::hercules_ir::ir::*;
-use self::hercules_ir::get_uses_mut;
 
 /*
- * Top level function to run phi elimination, as described above.
- * Deletes nodes by setting nodes to gravestones. Works with a function already
- * containing gravestones.
+ * Top level function to run phi elimination, as described above. Deletes nodes
+ * by setting nodes to gravestones. Works with a function already containing
+ * gravestones.
  */
-pub fn phi_elim(function : &mut Function) {
+pub fn phi_elim(function: &mut Function) {
     // Keep a map of nodes that we need to replace, and what we need to replace
-    // them with
-    let mut replace_nodes : HashMap<usize, NodeID> = HashMap::new();
+    // them with.
+    let mut replace_nodes: HashMap<usize, NodeID> = HashMap::new();
 
-    // Iterate over the nodes of the function until convergence.
-    // In this loop, we look for phis and regions that can be eliminated, mark
-    // them as gravestones, and add them to the replacement map. For all other
-    // nodes, we see if any of their arguments are in the replacement map and
-    // if so eliminate them
+    // Iterate over the nodes of the function until convergence. In this loop,
+    // we look for phis and regions that can be eliminated, mark them as
+    // gravestones, and add them to the replacement map. For all other nodes, we
+    // see if any of their arguments are in the replacement map - if so,
+    // eliminate them.
     let mut changed = true;
     while changed {
         changed = false;
 
         for (idx, node) in function.nodes.iter_mut().enumerate() {
             // Replace any nodes that this node uses that are in the replacement
-            // map
+            // map.
             for u in get_uses_mut(node).as_mut() {
                 let old_id = u.idx();
                 if let Some(replacement) = replace_nodes.get(&old_id) {
@@ -46,31 +47,34 @@ pub fn phi_elim(function : &mut Function) {
                 }
             }
 
-            // Then, check if this node can be removed
-            if let Node::Phi { control : _, data } = node {
-                // For a phi, we can remove it if all of its data inputs are
-                // the same node or self-cycles
+            // Then, check if this node can be removed.
+            if let Node::Phi { control: _, data } = node {
+                // For a phi, we can remove it if all of its data inputs are the
+                // same node or self-cycles.
                 let mut unique = Some(data[0]);
                 for i in 1..data.len() {
-                    // Ignore self-loops
-                    if data[i].idx() != idx && Some(data[i]) != unique{
+                    // Ignore self-loops.
+                    if data[i].idx() != idx && Some(data[i]) != unique {
                         if unique.unwrap().idx() == idx {
                             unique = Some(data[i]);
                         } else {
-                            unique = None; break;
+                            unique = None;
+                            break;
                         }
                     }
                 }
                 if let Some(value) = unique {
                     changed = true;
                     replace_nodes.insert(idx, value);
-                    *node = Node::Start; // mark node as removed
+                    // Delete this node.
+                    *node = Node::Start;
                 }
             } else if let Node::Region { preds } = node {
                 if preds.len() == 1 {
                     changed = true;
                     replace_nodes.insert(idx, preds[0]);
-                    *node = Node::Start; // mark as dead
+                    // Delete this node.
+                    *node = Node::Start;
                 }
             }
         }