Re: [PATCH] Fix VTA sp (and fp) based MEM handling (PR debug/43051, PR debug/43092) and also PR debug/43494

[Eric Botcazou <ebotcazou@adacore.com>]

[Sorry for the delay]

> Ping?  Adjusted to apply on current trunk, re-tested.

This is PR rtl-optimization/43494, not PR debug/43494.


@@ -1916,6 +1916,17 @@ extern int computed_jump_p (const_rtx);
 typedef int (*rtx_function) (rtx *, void *);
 extern int for_each_rtx (rtx *, rtx_function, void *);
 
+/* Callback for for_each_inc_dec, to process the autoinc operation OP
+   within MEM.  It sets DEST to SRC + SRCOFF, or SRC if SRCOFF is
+   NULL.

The "It" is ambiguous.

"Callback for for_each_inc_dec, to process the autoinc operation OP within
 MEM that sets DEST to SRC + SRCOFF, or SRC if SRCOFF is NULL."


+/* Add an insn to do the add inside a x if it is a
+   PRE/POST-INC/DEC/MODIFY.  D is an structure containing the insn and
+   the size of the mode of the MEM that this is inside of.  */
+
+static int
+for_each_inc_dec_find_inc_dec (rtx *r, void *d)

The comment is obsolete.  Something like:

"Find PRE/POST-INC/DEC/MODIFY operations within *R, extract the operands of
 the equivalent add insn and pass the result to the operator specified by *D".


+  int size = GET_MODE_SIZE (GET_MODE (data->mem));

SIZE doesn't need to be computed in all cases.


+/* If X is a MEM, check the address to see if it is PRE/POST-INC/DEC/MODIFY
+   and generate an add to replace that.  */
+static int
+for_each_inc_dec_find_mem (rtx *r, void *d)

"If *R is a MEM, find PRE/POST-INC/DEC/MODIFY operations within its address,
 extract the operands of the equivalent add insn and pass the result to the
 operator specified by *D"


-/* Add an insn to do the add inside a x if it is a
-   PRE/POST-INC/DEC/MODIFY.  D is an structure containing the insn and
-   the size of the mode of the MEM that this is inside of.  */
-
 static int
-replace_inc_dec (rtx *r, void *d)
+emit_inc_dec_insn_before (rtx mem ATTRIBUTE_UNUSED,
+			  rtx op ATTRIBUTE_UNUSED,
+			  rtx dest, rtx src, rtx srcoff, void *arg)

Missing comment for the function.


@@ -476,7 +476,7 @@ reload_cse_simplify_operands (rtx insn, 
 	    continue;
 	}
 #endif /* LOAD_EXTEND_OP */
-      v = cselib_lookup (op, recog_data.operand_mode[i], 0);
+      v =  cselib_lookup (op, recog_data.operand_mode[i], 0, VOIDmode);
       if (! v)
 	continue;

Superfluous space.


+static int
+rtx_equal_for_cselib_1 (rtx x, rtx y, enum machine_mode memmode)

Missing comment for the function.


@@ -807,6 +900,8 @@ wrap_constant (enum machine_mode mode, r
    that take commutativity into account.
    If we wanted to also support associative rules, we'd have to use a 
different
    strategy to avoid returning spurious 0, e.g. return ~(~0U >> 1) .
+   MEMMODE indicates the mode of an enclosing MEM, and it's only
+   used to compute auto-inc values.
    We used to have a MODE argument for hashing for CONST_INTs, but that
    didn't make sense, since it caused spurious hash differences between
     (set (reg:SI 1) (const_int))

In every other place you write "autoinc".


@@ -911,10 +1006,26 @@ cselib_hash_rtx (rtx x, int create)
 
     case PRE_DEC:
     case PRE_INC:
+      /* We can't compute these without knowing the MEM mode.  */
+      gcc_assert (memmode != VOIDmode);
+      i = GET_MODE_SIZE (memmode);
+      if (code == PRE_DEC)
+	i = -i;
+      hash += (unsigned) PLUS - (unsigned)code
+	+ cselib_hash_rtx (XEXP (x, 0), create, memmode)
+	+ cselib_hash_rtx (GEN_INT (i), create, memmode);
+      return hash ? hash : 1 + (unsigned) PLUS;

Maybe a small comment to say that we're directly hashing like a PLUS.


@@ -1064,7 +1175,7 @@ add_mem_for_addr (cselib_val *addr_elt, 
 static cselib_val *
 cselib_lookup_mem (rtx x, int create)
 {
-  enum machine_mode mode = GET_MODE (x);
+  enum machine_mode mode = GET_MODE (x), pmode = Pmode;
   void **slot;
   cselib_val *addr;
   cselib_val *mem_elt;

Use addr_mode instead of pmode and remove the initial value.


@@ -1075,8 +1186,11 @@ cselib_lookup_mem (rtx x, int create)
       || (FLOAT_MODE_P (mode) && flag_float_store))
     return 0;
 
+  if (GET_MODE (XEXP (x, 0)) != VOIDmode)
+    pmode = GET_MODE (XEXP (x, 0));

  addr_mode = GET_MODE (XEXP (x, 0));
  if (addr_mode == VOIDmode)
    addr_mode = Pmode;


@@ -1566,14 +1681,24 @@ cselib_subst_to_values (rtx x)
     case CONST_FIXED:
       return x;
 
-    case POST_INC:
+    case PRE_DEC:
     case PRE_INC:
+      gcc_assert (memmode != VOIDmode);
+      i = GET_MODE_SIZE (memmode);
+      if (code == PRE_DEC)
+	i = -i;
+      return cselib_subst_to_values (plus_constant (XEXP (x, 0), i),
+				     memmode);
+
+    case PRE_MODIFY:
+      gcc_assert (memmode != VOIDmode);
+      return cselib_subst_to_values (XEXP (x, 1), memmode);
+
     case POST_DEC:
-    case PRE_DEC:
+    case POST_INC:
     case POST_MODIFY:
-    case PRE_MODIFY:
-      e = new_cselib_val (next_uid, GET_MODE (x), x);
-      return e->val_rtx;
+      gcc_assert (memmode != VOIDmode);
+      return cselib_subst_to_values (XEXP (x, 0), memmode);
 
     default:
       break;

Can you adjust the comment for the MEM case in the same function?


+/* Callback for for_each_inc_dec.  Records in ARG the SETs implied by
+   autoinc RTXs: SRCBASE plus SRCOFF if non-NULL is stored in
+   DEST.  */

"Record in ARG".  No need to break the line before "DEST".


+static int
+cselib_record_autoinc_cb (rtx mem ATTRIBUTE_UNUSED, rtx op ATTRIBUTE_UNUSED,
+			  rtx dest, rtx srcbase, rtx srcoff, void *arg)

The prototype in rtl.h has SRC instead of SRCBASE.


+  if (srcoff)
+    data->sets[data->n_sets].src = gen_rtx_PLUS (GET_MODE (srcbase),
+						 srcbase, srcoff);

  if (srcoff)
    data->sets[data->n_sets].src
       = gen_rtx_PLUS (GET_MODE (srcbase), srcbase, srcoff);


@@ -1963,13 +2093,6 @@ cselib_invalidate_rtx (rtx dest)
     cselib_invalidate_regno (REGNO (dest), GET_MODE (dest));
   else if (MEM_P (dest))
     cselib_invalidate_mem (dest);
-
-  /* Some machines don't define AUTO_INC_DEC, but they still use push
-     instructions.  We need to catch that case here in order to
-     invalidate the stack pointer correctly.  Note that invalidating
-     the stack pointer is different from invalidating DEST.  */
-  if (push_operand (dest, GET_MODE (dest)))
-    cselib_invalidate_rtx (stack_pointer_rtx);
 }

This hunk isn't mentioned in the ChangeLog.  You also need to mention there 
that cselib_record_sets now handles the autoinc stuff, in particular the 
invalidation.  In light of this, isn't

#ifdef AUTO_INC_DEC
  /* Clobber any registers which appear in REG_INC notes.  We
     could keep track of the changes to their values, but it is
     unlikely to help.  */
  for (x = REG_NOTES (insn); x; x = XEXP (x, 1))
    if (REG_NOTE_KIND (x) == REG_INC)
      cselib_invalidate_rtx (XEXP (x, 0));
#endif

in cselib_process_insn superfluous now?


Otherwise looks OK to me.  Any idea of the impact on compilation performances 
for non-AUTO_INC_DEC targets?

-- 
Eric Botcazou