Code review (in progress)

backend/cfg/cfg_dominators.mli

@@ -1,6 +1,6 @@
 [@@@ocaml.warning "+a-4-30-40-41-42"]
 
-(* Dominator-related utility functions. *)
+(** Dominator-related utility functions. *)
 
 type dominator_tree = private
   { label : Label.t;
@@ -9,31 +9,35 @@ type dominator_tree = private
 
 type t
 
-val build : Cfg.t -> t
-(* Computes all dominator-related information, in particular immediate
+(** Computes all dominator-related information, in particular immediate
    dominators, dominance frontiers, and dominator forest for the passed CFG. *)
+val build : Cfg.t -> t
 
-val is_dominating : t -> Label.t -> Label.t -> bool
-(* [is_dominating doms x y] is [true] iff [x] is dominating [y] according to
+(** [is_dominating doms x y] is [true] iff [x] is dominating [y] according to
    [doms]. That is, all paths from the entry node to [y] go through [x]. All
    edges, regular and exceptional are treated the same way. *)
+val is_dominating : t -> Label.t -> Label.t -> bool
 
-val is_strictly_dominating : t -> Label.t -> Label.t -> bool
-(* [is_strictly_dominating doms x y] is [true] iff [x] is strictly dominating
+(** [is_strictly_dominating doms x y] is [true] iff [x] is strictly dominating
    [y] according to [doms]. That is, [is_dominating doms x y = true] and [x] is
    not equal [y]. All edges, regular and exceptional are treated the same way.*)
+val is_strictly_dominating : t -> Label.t -> Label.t -> bool
 
-val find_dominance_frontier : t -> Label.t -> Label.Set.t
-(* [find_dominance_frontier doms label] returns the dominance frontier for
+(** [find_dominance_frontier doms label] returns the dominance frontier for
    [label] according to [doms]. The definition we use is the following: "the
    dominance frontier of a node n is the set of all nodes m such that n
    dominates a predecessor of m, but does not strictly dominate m itself". *)
+val find_dominance_frontier : t -> Label.t -> Label.Set.t
 
+(** Returns all dominator trees. Typically one of these will correspond to the
+   entry point of the program and the remainder to dead code. *)
 val dominator_forest : t -> dominator_tree list
 
+(** Returns the unique dominator tree rooted at the entry point of the program
+   (thus ignoring any isolated trees that correspond to dead code). *)
 val dominator_tree_for_entry_point : t -> dominator_tree
 
-val iter_breadth_dominator_forest : t -> f:(Label.t -> unit) -> unit
-(* [iter_breadth_dominator_forest doms ~f] iterates over the dominator forest
+(** [iter_breadth_dominator_forest doms ~f] iterates over the dominator forest
    from [doms] in a breadth-first manner (iterating over a whole tree of the
    forest before moving to the next tree), applying [f] to visited nodes. *)
+val iter_breadth_dominator_forest : t -> f:(Label.t -> unit) -> unit

backend/cfg/cfg_stack_checks.ml

@@ -173,8 +173,6 @@ let insert_stack_checks (cfg : Cfg.t) ~max_frame_size
   let loop_infos = lazy (Cfg_loop_infos.build cfg doms) in
   (* note: the other entries in the forest are dead code *)
   let tree = Cfg_dominators.dominator_tree_for_entry_point doms in
-  (* XCR mshinwell: maybe there should be a method on Cfg to just retrieve the
-     correct dominator tree and ignore the dead code ones? *)
   let num_checks = Label.Tbl.create (Label.Tbl.length cfg.blocks) in
   let num_checks_root =
     num_checks_tree tree ~blocks_needing_stack_checks ~num_checks
@@ -201,10 +199,6 @@ let insert_stack_checks (cfg : Cfg.t) ~max_frame_size
         fdo = Fdo_info.none;
         live = Reg.Set.empty;
         stack_offset;
-        (* XCR mshinwell: maybe there should be an explicit check that this code
-           hasn't been executed twice, since we don't update [max_instr_id] (or
-           expect checks to be duplicated at present)? xclerc: with the utility
-           function to get the only interesting tree, we no longer iterate. *)
         id = succ max_instr_id;
         irc_work_list = Unknown_list;
         ls_order = 0;