Re: RFA: Fix dse / postreload not to bypass add expanders

[Eric Botcazou <ebotcazou at adacore dot com>]

> This patch makes emit_inc_dec_insn_before use add3_insn / gen_move_insn
> so that the appropriate expanders are used to create the new instructions,
> and for dse it use the available register liveness information to check
> that no live fixed hard register, like a flags register, is clobbered in
> the process.  For postreload, there is no such information available, so we
> give up when we see a clobber / set that might be problematic.

2011-10-31  Joern Rennecke <joern.rennecke@embecosm.com>

	* regset.h (fixed_regset): Declare.
	* dse.c: Include regset.h .
	(struct insn_info): Add member fixed_regs_live.
	(note_add_store_info): New typedef.
	(note_add_store): New function.
	(emit_inc_dec_insn_before): Expect arg to be of type insn_info_t .
	Use gen_add3_insn / gen_move_insn.
	Check new insn for unwanted clobbers before emitting it.
	(check_for_inc_dec): Rename to...
	(check_for_inc_dec_1:) ... this.  Return bool.  Take insn_info
	parameter.  Changed all callers in file.
	(check_for_inc_dec, copy_fixed_regs): New functions.
	(scan_insn): Set fixed_regs_live field of insn_info.
	* rtl.h (check_for_inc_dec): Update prototype.
	* postreload.c (reload_cse_simplify): Take new signature of
	check_ind_dec into account.
	* reginfo.c (fixed_regset): New variable.
	(init_reg_sets_1): Initialize it.

OK modulo the following:

+typedef struct
+{
+  rtx insert_before;

This field is never read.


+  rtx first, current;
+  regset fixed_regs_live;
+  bool failure;
+} note_add_store_info;
+
+/* Callback for emit_inc_dec_insn_before via note_stores.
+   Check if a register is clobbered which is life afterwards.  */

"live"


+static void
+note_add_store (rtx loc, const_rtx expr ATTRIBUTE_UNUSED, void *data)

Missing blank line.  The functions in dse.c have a blank line between head 
comment and body.


+{
+  rtx insn, *nextp;
+  note_add_store_info *info = (note_add_store_info *) data;
+  int r, n;
+
+  if (!REG_P (loc))
+    return;
+  /* If this register is referenced by the current or an earlier insn,
+     that's OK.  E.g. this applies to the register that is being incremented
+     with this addition.  */

Blank line before the comment.


+  nextp = &info->first;
+  do
+    {
+      insn = *nextp;
+      nextp = &NEXT_INSN (insn);
+      if (reg_referenced_p (loc, PATTERN (insn)))
+	return;
+    }
+  while (insn != info->current);

Isn't that a convoluted way of writing this?

  for (insn = info->first;
       insn != NEXT_INSN (info->current);
       insn = NEXT_INSN (insn))
    if (reg_referenced_p (loc, PATTERN (insn)))
      return;


+  if (!info->fixed_regs_live)
+    {
+      info->failure =  true;
+      return;
+    }

Missing comment explaining why we do that.



+  /* Now check if this is a live fixed register.  */
+  r = REGNO (loc);
+  n = HARD_REGNO_NREGS (r, GET_MODE (loc));
+  while (--n >=  0)
+    if (REGNO_REG_SET_P (info->fixed_regs_live, r+n))
+      info->failure =  true;

Blank line before the comment.  What's the point in the reverse iteration?

  for (i = 0; i < hard_regno_nregs[regno][GET_MODE (loc)]; i++)
    if (REGNO_REG_SET_P (info->fixed_regs_live, regno + i))
       {
         info->failure =  true;
         return;
       }

hard_regno_nregs in small letters.


+  info.insert_before = insn;
+  info.first = new_insn;
+  info.fixed_regs_live = insn_info->fixed_regs_live;
+  info.failure = false;
+  for (cur = new_insn; cur; cur = NEXT_INSN (cur))
+    {
+      info.current = cur;
+      note_stores (PATTERN (cur), note_add_store, &info);
+    }
+  if (info.failure)
+    return 1;

Missing comment explaining what we're doing.



 /* Before we delete INSN, make sure that the auto inc/dec, if it is
-   there, is split into a separate insn.  */
+   there, is split into a separate insn.
+   Return true on success (or if there was nothing to do), false on failure.  
*/
 
-void
-check_for_inc_dec (rtx insn)
+static bool
+check_for_inc_dec_1 (insn_info_t insn_info)

Missing adjustment in the comment: "Before we delete the insn described by
INSN_INFO, make sure..."


+/* Entry point for postreload.  */
+bool
+check_for_inc_dec (rtx insn, regset fixed_regs_live)

No point in adding an argument if it is always null.  Missing blank line and 
head comment: "Same as above, but take a naked INSN instead.  This is used by 
passes like that don't compute precise liveness information."


+/* Return a bitmap of the fixed registers contained in IN.  */
+static bitmap
+copy_fixed_regs (const_bitmap in)
+{
+  bitmap ret;
+
+  ret = ALLOC_REG_SET (NULL);
+  bitmap_and (ret, in, fixed_regset);
+  return ret;
+}

Missing blank line.


+/* Same information as fixed_reg_set but in regset form.  */
+regset fixed_regset;

Hum, you'd better have a good trick to remember which is which.  This isn't 
pretty, but let's mimic what is just above:

/* Same information as FIXED_REG_SET but in regset form.  */
regset fixed_reg_set_regset;


-- 
Eric Botcazou