Re: New register allocator

[Geoff Keating <geoffk at geoffk dot org>]

I finally got around to looking at this, sorry for the delay.

It wasn't clear from all the e-mail messages precisely what patch you
wanted reviewed and would commit.  I looked at:

<http://user.cs.tu-berlin.de/~matzmich/ra/ra_all.diff.gz>

with a datestamp of 06/26/2002 09:07:01 AM

and have the following comments:

- Please be sure to list all the authors in the ChangeLog entry.
- In Makefile.in, OBJS is currently in approximately alphabetical
  order, please make it really in alphabetical order and put the new
  files in the right places.

- Can you add comments describing what these global variables do,
  copied from ra.h, or at least a comment that says that these are all
  described in ra.h?  You might also want to double-check that none of
  them can be 'static'.

+/* Somewhen we want to get rid of one of those sbitmaps.
+   (for now I need the sup_igraph to note if there is any conflict
between
+   parts of webs at all.  I can't use igraph for this, as there only
the real
+   conflicts are noted.)  This is only used to prevent coalescing two
+   conflicting webs, were only parts of them are in conflict.  */
+sbitmap igraph;
+sbitmap sup_igraph;
+
+/* Note the insns not inserted by the allocator, where we detected
any
+   deaths of pseudos.  It is used to detect closeness of defs and
uses.
+   In the first pass this is empty (we could initialize it from
REG_DEAD
+   notes), in the other passes it is left from the pass before.  */
+sbitmap insns_with_deaths;
+int death_insns_max_uid;
+
+struct web_part *web_parts;
+
+unsigned int num_webs;
+unsigned int num_subwebs;
+unsigned int num_allwebs;
+struct web **id2web;
+struct web *hardreg2web[FIRST_PSEUDO_REGISTER];
+struct web **def2web;
+struct web **use2web;
+struct move_list *wl_moves;
+int ra_max_regno;
+short *ra_reg_renumber;
+struct df *df;
+bitmap *live_at_end;
+int ra_pass;
+unsigned int max_normal_pseudo;
+int an_unusable_color;
+
+/* The different lists on which a web can be (based on the type).  */
+struct dlist *web_lists[(int) LAST_NODE_TYPE];
+
+unsigned int last_def_id;
+unsigned int last_use_id;
+unsigned int last_num_webs;
+int last_max_uid;
+sbitmap last_check_uses;
+unsigned int remember_conflicts;
+
+int orig_max_uid;
+
+HARD_REG_SET never_use_colors;
+HARD_REG_SET usable_regs[N_REG_CLASSES];
+unsigned int num_free_regs[N_REG_CLASSES];
+HARD_REG_SET hardregs_for_mode[NUM_MACHINE_MODES];
+unsigned char byte2bitcount[256];
+
+unsigned int debug_new_regalloc = -1;
+int flag_ra_biased = 0;
+int flag_ra_ir_spilling = 0;
+int flag_ra_optimistic_coalescing = 0;
+int flag_ra_break_aliases = 0;
+int flag_ra_merge_spill_costs = 0;
+int flag_ra_spill_every_use = 0;
+int flag_ra_dump_notes = 0;

- In most other places in the compiler, there's a blank line between
  the comment above a function and the start of the function, here:

+/* Like ra_alloc(), but clear the returned memory.  */
>>> here
+void *
+ra_calloc (size)
+     size_t size;

  Could you add blank lines here?

- I think this piece of code could really use a comment explaining
  why it does what it does:

+#ifdef ELIMINABLE_REGS
+  for (j = 0; j < ARRAY_SIZE (eliminables); j++)
+    {
+      if (! CAN_ELIMINATE (eliminables[j].from, eliminables[j].to)
+        || (eliminables[j].to == STACK_POINTER_REGNUM && need_fp))
+	 for (i = HARD_REGNO_NREGS (eliminables[j].from, Pmode); i--;)
+	   SET_HARD_REG_BIT (never_use_colors, eliminables[j].from +
i);
+    }
+#if FRAME_POINTER_REGNUM != HARD_FRAME_POINTER_REGNUM
+  if (need_fp)
+    for (i = HARD_REGNO_NREGS (HARD_FRAME_POINTER_REGNUM, Pmode);
i--;)
+      SET_HARD_REG_BIT (never_use_colors, HARD_FRAME_POINTER_REGNUM +
i);
+#endif
+
+#else
+  if (need_fp)
+    for (i = HARD_REGNO_NREGS (FRAME_POINTER_REGNUM, Pmode); i--;)
+      SET_HARD_REG_BIT (never_use_colors, FRAME_POINTER_REGNUM + i);
+#endif

- Please don't add any new invocations of ggc_add_root or
  ggc_mark_rtx.  Both these routines are obsolete and will be deleted
  Real Soon Now.  Use GTY markers instead.

- The following line is commented out:

+  /*rewrite_program (new_deaths);*/

  Why?  If it's really not supposed to ever be used, shouldn't the
  whole rewrite_program procedure, and this line, just be deleted?
  It's confusing to have code sitting around that is never used, and I
  think that GCC will produce a warning that 'rewrite_program' is not
  used.

I think these changes are sufficiently minor that when you've
addressed them, you can just commit the resulting patch without
needing further review.

-- 
- Geoffrey Keating <geoffk@geoffk.org> <geoffk@redhat.com>