Dataflow branch review #1

[Ian Lance Taylor]

I've started looking at the overall dataflow patch.  Here is my first
set of comments, covering auto-inc-dec.c.

Ian


In auto-inc-dec.c:

Update copyright date.


+enum form
+  {
+/* This pass was originally removed from flow.c. However there is
+   almost nothing that remains of that code.
+
+   There are (4) basic forms that are matched:
+
+*/
+    FORM_PRE_ADD,
+/*
+           a <- b + c
+           ...
+           *a
+
+        becomes
+
+           a <- b
+           ...
+           *(a += c) pre
+*/

Please move the comments for the enum types before the enum
definition.  gcc practice is to put comments before the object being
described.  So the comment above should come before FORM_PRE_ADD.

Also, indent as

enum form
{
  FORM_PRE_ADD
};


+   The arrays are not cleared when we move from block to block so
+   whenever an insn is retrieved from these arrays, it's block number
+   must be comared with the current block.
+*/

Typo: "comared" => "compared"

+static rtx * reg_next_use = NULL;
+static rtx * reg_next_inc_use = NULL;
+static rtx * reg_next_def = NULL;

Just write "rtx*", not "rtx *".


+      if ((REG_NOTE_KIND(note) == REG_DEAD)
+	  && pattern == XEXP (note, 0))

Space after REG_NOTE_KIND.

+      else prev_note = note;

Put "prev_note = note;" on the next line.


+/* Change mem_insn.mem_loc so that uses NEW_ADDR that increments
+   INC_REG.  To have reached this point, the change is a legitimate
+   one from a dataflow point of view.  The only questions are is this
+   a valid change to the instruction and is this a profitable change
+   to the instruction.  */
+
+static bool
+attempt_change (rtx new_addr, rtx inc_reg)

I don't understand the first sentence in the comment.

+      reg_next_def [regno] = mov_insn;
+      reg_next_use [regno] = NULL;

No space before '[' here and elsewhere.

+  switch (inc_insn.form)
...
+    case FORM_last:
+      gcc_unreachable ();

Add a default: case which calls gcc_unreachable.

+  /* Recompute the df info for the insns that have changed. */
+  delete_insn (inc_insn.insn);

This comment looks wrong.


In try_merge:

+  /* The width of the mem being access.  */

Typo: "access" => "accessed".

+  switch (inc_insn.form)
...
+    case FORM_last:
+      gcc_unreachable ();

Add a default case here also.


+/* Return the next insn that uses (if reg_next_use is passed in
+   NEXT_ARRAY) or defines if reg_next_def is passed in NEXT_ARRAY)
+   REGNO in BB.  */

Missing left parenthesis before "if reg_next_def", I think.


+   This function is called in two contexts, if BEFORE_MEM is true,
+   this is called for each insn in the basic block.  If BEFORE_MEM is
+   false, it is called for the instruction in the block that uses the
+   index register for som memory reference that is currently being
+   processed.  */

Typo: "som" => "some".


+   In the case where the MEM_)INSN has two registers in the reference,
+   this function may be called recursively.  The first time looking
+   for an add of the first register, and if that fails, looking for an
+   add of the second register.  The FIRST_TRY parameter is used to
+   only allow the parameters to be reversed once.  */

Extraneous ')' in first line.