Re: [Patch, ARM] PR71061, length pop* pattern in epilogue correctly

[Jiong Wang <jiong dot wang at foss dot arm dot com>]

On 13/05/16 14:54, Jiong Wang wrote:
> For thumb mode, this is causing wrong size calculation and may affect
> some rtl pass, for example bb-order where copy_bb_p needs accurate insn
> length info.
>
> This have eventually part of the reason for
> https://gcc.gnu.org/ml/gcc-patches/2016-05/msg00639.html  where bb-order
> failed to do the bb copy.
>
> For the fix, I think we should extend arm_attr_length_push_multi to pop*
> pattern.
>
> OK for trunk?
>
> 2016-05-13  Jiong. Wang <jiong.wang@arm.com>
>
> gcc/
>   PR target/71061
>   * config/arm/arm-protos.h (arm_attr_length_push_multi): Rename to
>   "arm_attr_length_pp_multi".  Add one parameter "first_index".
>   * config/arm/arm.md (*push_multi): Use new function.
>   (*load_multiple_with_writeback): Set "length" attribute.
>   (*pop_multiple_with_writeback_and_return): Likewise.
>   (*pop_multiple_with_return): Likewise.

Wilco pointed out offline the new length function is wrong as for pop we
should check PC_REG instead of LR_REG, meanwhile these pop patterns may
allow ldm thus base register needs to be checked also.

I updated this patch to address these issues.

OK for trunk ?

gcc/

2016-05-19  Jiong Wang  <jiong.wang@arm.com>

    PR target/71061
    * config/arm/arm-protos.h (arm_attr_length_pop_multi): New declaration.
    * config/arm/arm.c (arm_attr_length_pop_multi): New function to return
    length for pop patterns.
    * config/arm/arm.md (*load_multiple_with_writeback): Set "length" attribute.
    (*pop_multiple_with_writeback_and_return): Likewise.
    (*pop_multiple_with_return): Likewise.


diff --git a/gcc/config/arm/arm-protos.h b/gcc/config/arm/arm-protos.h
index d8179c4..5dd3e0d 100644
--- a/gcc/config/arm/arm-protos.h
+++ b/gcc/config/arm/arm-protos.h
@@ -163,6 +163,7 @@ extern const char *arm_output_iwmmxt_shift_immediate (const char *, rtx *, bool)
 extern const char *arm_output_iwmmxt_tinsr (rtx *);
 extern unsigned int arm_sync_loop_insns (rtx , rtx *);
 extern int arm_attr_length_push_multi(rtx, rtx);
+extern int arm_attr_length_pop_multi(rtx *, bool, bool);
 extern void arm_expand_compare_and_swap (rtx op[]);
 extern void arm_split_compare_and_swap (rtx op[]);
 extern void arm_split_atomic_op (enum rtx_code, rtx, rtx, rtx, rtx, rtx, rtx);
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index c3f74dc..4d19d3a 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -27812,6 +27812,65 @@ arm_attr_length_push_multi(rtx parallel_op, rtx first_op)
   return 4;
 }
 
+/* Compute the atrribute "length" of insn.  Currently, this function is used
+   for "*load_multiple_with_writeback", "*pop_multiple_with_return" and
+   "*pop_multiple_with_writeback_and_return".  */
+
+int
+arm_attr_length_pop_multi(rtx *operands, bool return_pc, bool write_back_p)
+{
+  /* ARM mode.  */
+  if (TARGET_ARM)
+    return 4;
+  /* Thumb1 mode.  */
+  if (TARGET_THUMB1)
+    return 2;
+
+  /* For POP/PUSH/LDM/STM under Thumb2 mode, we can use 16-bit Thumb encodings
+     if the register list is 8-bit.  Normally this means all registers in the
+     list must be LO_REGS, that is (R0 -R7).  If any HI_REGS used, then we must
+     use 32-bit encodings.  But there are two exceptions to this rule where
+     special HI_REGS used in PUSH/POP.
+
+     1. 16-bit Thumb encoding POP can use an 8-bit register list, and an
+        additional bit, P, that corresponds to the PC.  This means it can access
+	any of {PC, R7-R0}.
+     2. 16-bit Thumb encoding PUSH can use an 8-bit register list, and an
+	additional bit, M , that corresponds to the LR.  This means it can
+	access any of {LR, R7-R0}.  */
+
+  rtx parallel_op = operands[0];
+  /* Initialize to elements number of PARALLEL.  */
+  unsigned indx = XVECLEN (parallel_op, 0) - 1;
+  /* Initialize the value to base register.  */
+  unsigned regno = REGNO (operands[1]);
+  /* Skip return and write back pattern.
+     We only need register pop pattern for later analysis.  */
+  unsigned first_indx = 0;
+  first_indx += return_pc ? 1 : 0;
+  first_indx += write_back_p ? 1 : 0;
+
+  /* A pop operation can be done through LDM or POP.  If the base register is SP
+     and if it's with write back, then a LDM will be alias of POP.  */
+  bool pop_p = (regno == SP_REGNUM && write_back_p);
+  bool ldm_p = !pop_p;
+
+  /* Check base register for LDM.  */
+  if (ldm_p && REGNO_REG_CLASS (regno) == HI_REGS)
+    return 4;
+
+  /* Check each register in the list.  */
+  for (; indx >= first_indx; indx--)
+    {
+      regno = REGNO (XEXP (XVECEXP (parallel_op, 0, indx), 0));
+      if (REGNO_REG_CLASS (regno) == HI_REGS
+	  && (regno != PC_REGNUM || ldm_p))
+	return 4;
+    }
+
+  return 2;
+}
+
 /* Compute the number of instructions emitted by output_move_double.  */
 int
 arm_count_output_move_double_insns (rtx *operands)
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 4049f10..d746690 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -10565,7 +10565,11 @@
   }
   "
   [(set_attr "type" "load4")
-   (set_attr "predicable" "yes")]
+   (set_attr "predicable" "yes")
+   (set (attr "length")
+	(symbol_ref "arm_attr_length_pop_multi (operands,
+						/*return_pc=*/false,
+						/*write_back_p=*/true)"))]
 )
 
 ;; Pop with return (as used in epilogue RTL)
@@ -10594,7 +10598,10 @@
   }
   "
   [(set_attr "type" "load4")
-   (set_attr "predicable" "yes")]
+   (set_attr "predicable" "yes")
+   (set (attr "length")
+	(symbol_ref "arm_attr_length_pop_multi (operands, /*return_pc=*/true,
+						/*write_back_p=*/true)"))]
 )
 
 (define_insn "*pop_multiple_with_return"
@@ -10614,7 +10621,10 @@
   }
   "
   [(set_attr "type" "load4")
-   (set_attr "predicable" "yes")]
+   (set_attr "predicable" "yes")
+   (set (attr "length")
+	(symbol_ref "arm_attr_length_pop_multi (operands, /*return_pc=*/true,
+						/*write_back_p=*/false)"))]
 )
 
 ;; Load into PC and return