Re: [RFA/ARM][Patch 01/02]: Thumb2 epilogue in RTL

[Ramana Radhakrishnan <ramana.radhakrishnan@linaro.org>]

Hi Sameera,

The comment about REG_FRAME_RELATED_EXPR vs REG_CFA_RESTORE from one
of your later patches
applies here as well.

>diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>index 3162b30..f86a3e6 100644
>--- a/gcc/config/arm/arm.c
>+++ b/gcc/config/arm/arm.c
>@@ -8754,6 +8754,140 @@ neon_valid_immediate (rtx op, enum machine_mode mode, int inverse,
> #undef CHECK
> }
>
>+/* Return true if OP is a valid load multiple operation for MODE mode.
>+   CONSECUTIVE is true if the registers in the operation must form
>+   a consecutive sequence in the register bank.  STACK_ONLY is true
>+   if the base register must be the stack pointer.  RETURN_PC is true
>+   if value is to be loaded in PC.  */
>+bool
>+load_multiple_operation_p (rtx op, bool consecutive, enum machine_mode mode,
>+                           bool stack_only, bool return_pc)
>+{

<...> snip

>+
>+  /* If DFMode, we must be asking for consecutive,
>+     since FLDMDD can only do consecutive regs.  */

s/DFMode/DFmode
s/FLDMDD/fldmdd (vldm.f64)

Why are you differentiating on stack_only ? Does it really matter ?


>+  gcc_assert ((mode != DFmode) || consecutive);
>+
>+  /* Set up the increments and the regs per val based on the mode.  */
>+  reg_increment = mode == DFmode ? 8 : 4;

Can't you just get the reg_increment based on GET_MODE_SIZE (mode) ?

>+  regs_per_val = mode == DFmode ? 2 : 1;
>+  offset_adj = return_pc ? 1 : 0;
>+
>+  if (count <= 1
>+      || GET_CODE (XVECEXP (op, 0, offset_adj)) != SET
>+      || !REG_P (SET_DEST (XVECEXP (op, 0, offset_adj))))
>+    return false;
>+
>+  /* Check to see if this might be a write-back.  */
>+  if (GET_CODE (SET_SRC (elt = XVECEXP (op, 0, offset_adj))) == PLUS)
>+    {
>+      i++;
>+      base = 1;
>+      update = true;
>+
>+      /* Now check it more carefully.  */
>+      if (!REG_P (SET_DEST (elt))
>+          || !REG_P (XEXP (SET_SRC (elt), 0))
>+          || !CONST_INT_P (XEXP (SET_SRC (elt), 1))
>+          || INTVAL (XEXP (SET_SRC (elt), 1)) !=
>+              ((count - 1 - offset_adj) * reg_increment))
>+        return false;

A comment here explaining that you are checking for the
increment amount being sane would be good.


>+
>+      /* Check the nature of the base_register being written to.  */
>+      if (stack_only && (REGNO (SET_DEST (elt)) != SP_REGNUM))
>+        return false;
>+    }
>+
>+  i = i + offset_adj;
>+  base = base + offset_adj;
>+  /* Perform a quick check so we don't blow up below.  */
>+  if (GET_CODE (XVECEXP (op, 0, i - 1)) != SET
>+      || !REG_P (SET_DEST (XVECEXP (op, 0, i - 1)))
>+      || !MEM_P (SET_SRC (XVECEXP (op, 0, i - 1))))
>+    return false;
>+
>+  /* If only one reg being loaded, success depends on the type:
>+     FLDMDD can do just one reg, LDM must do at least two.  */

Hmmm isn't this true of only LDM's in Thumb state ? Though it could be argued
that this patch is only T2 epilogues.

>+  if (count <= i)
>+    return mode == DFmode ? true : false;

Again a comment here would be useful.

>+
>+  first_dest_regno = REGNO (SET_DEST (XVECEXP (op, 0, i - 1)));
>+  dest_regno = first_dest_regno;
>+
>+  src_addr = XEXP (SET_SRC (XVECEXP (op, 0, i - 1)), 0);
>+
>+  if (GET_CODE (src_addr) == PLUS)
>+    {
>+      if (!CONST_INT_P (XEXP (src_addr, 1)))
>+	return false;

Watch out for the indentation of the return.

<...snip>
>+)
>+
>+(define_insn "*floating_point_pop_multiple_with_stack_update"

s/floating_point/vfp

>+  [(match_parallel 0 "load_multiple_operation_stack_fp"
>+    [(set (match_operand:SI 1 "s_register_operand" "=k")
>+          (plus:SI (match_operand:SI 2 "s_register_operand" "1")
>+                   (match_operand:SI 3 "const_int_operand" "I")))
>+     (set (match_operand:DF 4 "arm_hard_register_operand" "")
>+          (mem:DF (match_dup 2)))])]
>+  "TARGET_THUMB2"

&& TARGET_HARD_FLOAT && TARGET_VFP

>+  "*
>+  {
>+    int num_regs = XVECLEN (operands[0], 0);
>+    static const struct { const char *const name; } table[]
>+                  = { {\"d0\"}, {\"d1\"}, {\"d2\"}, {\"d3\"},
>+                      {\"d4\"}, {\"d5\"}, {\"d6\"}, {\"d7\"},
>+                      {\"d8\"}, {\"d9\"}, {\"d10\"}, {\"d11\"},
>+                      {\"d12\"}, {\"d13\"}, {\"d14\"}, {\"d15\"},
>+                      {\"d16\"}, {\"d17\"}, {\"d18\"}, {\"d19\"},
>+                      {\"d20\"}, {\"d21\"}, {\"d22\"}, {\"d23\"},
>+                      {\"d24\"}, {\"d25\"}, {\"d26\"}, {\"d27\"},
>+                      {\"d28\"}, {\"d29\"}, {\"d30\"}, {\"d31\"} };


>+    int i;
>+    char pattern[100];
>+    strcpy (pattern, \"fldmfdd\\t\");
>+    strcat (pattern,
>+                    reg_names[REGNO (SET_DEST (XVECEXP (operands[0], 0, 0)))]);
>+    strcat (pattern, \"!, {\");
>+    strcat (pattern, table[(REGNO (XEXP (XVECEXP (operands[0], 0, 1), 0))
>+                           - FIRST_VFP_REGNUM) / 2].name);

Can't you reuse names from arm.h and avoid the table here ?


>+    for (i = 2; i < num_regs; i++)
>+      {
>+        strcat (pattern, \", %|\");
>+        strcat (pattern, table[(REGNO (XEXP (XVECEXP (operands[0], 0, i), 0))
>+                               - FIRST_VFP_REGNUM) / 2].name);
>+      }

Can't you use fldmfdd {reg_lo-reg_hi} instead of enumerating all the
registers here.

>+    strcat (pattern, \"}\");
>+    output_asm_insn (pattern, operands);
>+    return \"\";
>+  }
>+  "
>+  [(set_attr "type" "load4")]




Ramana