[ARM] Fix Thumb-1 ldm (PR89190)

This patch fixes an ICE in the Thumb-1 LDM peepholer.  Thumb-1 LDMs
always update the base register except if the base is loaded.
The current implementation rejects LDMs where the base is not dead,
however this doesn't exclude the case where the base is loaded as
well as dead.  Fix this by explicitly checking whether the base is
loaded.  Also enable LDMs which load the first register.

Bootstrap OK on armhf, testsuite passes. OK for commit?

ChangeLog:
2019-02-04  Wilco Dijkstra  <wdijkstr@arm.com>

	PR target/89190
	* config/arm/arm.c (ldm_stm_operation_p) Set
	addr_reg_in_reglist correctly for first register.
	(load_multiple_sequence): Remove dead base check.
	(gen_ldm_seq): Correctly set write_back for Thumb-1.

	PR target/89190
	* gcc.target/arm/pr89190.c: New test.
--

ping


From: Wilco Dijkstra
Sent: 04 February 2019 14:59
To: GCC Patches
Cc: nd; Kyrylo Tkachov
Subject: [PATCH][ARM] Fix Thumb-1 ldm (PR89190)
  

This patch fixes an ICE in the Thumb-1 LDM peepholer.  Thumb-1 LDMs
always update the base register except if the base is loaded.
The current implementation rejects LDMs where the base is not dead,
however this doesn't exclude the case where the base is loaded as
well as dead.  Fix this by explicitly checking whether the base is
loaded.  Also enable LDMs which load the first register.

Bootstrap OK on armhf, testsuite passes. OK for commit?

ChangeLog:
2019-02-04  Wilco Dijkstra  <wdijkstr@arm.com>

        PR target/89190
        * config/arm/arm.c (ldm_stm_operation_p) Set
        addr_reg_in_reglist correctly for first register.
        (load_multiple_sequence): Remove dead base check.
        (gen_ldm_seq): Correctly set write_back for Thumb-1.

        PR target/89190
        * gcc.target/arm/pr89190.c: New test.
--

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 16e22eed871ca34d56c83c334688e5a95970638e..c4c9b4a667100d81d918196713e40b01ee232ee2 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -13191,6 +13191,9 @@ ldm_stm_operation_p (rtx op, bool load, machine_mode mode,
   if (load && (REGNO (reg) == SP_REGNUM) && (REGNO (addr) != SP_REGNUM))
     return false;
 
+  if (regno == REGNO (addr))
+    addr_reg_in_reglist = true;
+
   for (; i < count; i++)
     {
       elt = XVECEXP (op, 0, i);
@@ -13385,7 +13388,6 @@ load_multiple_sequence (rtx *operands, int nops, int *regs, int *saved_order,
   int unsorted_regs[MAX_LDM_STM_OPS];
   HOST_WIDE_INT unsorted_offsets[MAX_LDM_STM_OPS];
   int order[MAX_LDM_STM_OPS];
-  rtx base_reg_rtx = NULL;
   int base_reg = -1;
   int i, ldm_case;
 
@@ -13430,7 +13432,6 @@ load_multiple_sequence (rtx *operands, int nops, int *regs, int *saved_order,
           if (i == 0)
             {
               base_reg = REGNO (reg);
-             base_reg_rtx = reg;
               if (TARGET_THUMB1 && base_reg > LAST_LO_REGNUM)
                 return 0;
             }
@@ -13489,10 +13490,6 @@ load_multiple_sequence (rtx *operands, int nops, int *regs, int *saved_order,
       *load_offset = unsorted_offsets[order[0]];
     }
 
-  if (TARGET_THUMB1
-      && !peep2_reg_dead_p (nops, base_reg_rtx))
-    return 0;
-
   if (unsorted_offsets[order[0]] == 0)
     ldm_case = 1; /* ldmia */
   else if (TARGET_ARM && unsorted_offsets[order[0]] == 4)
@@ -13868,9 +13865,17 @@ gen_ldm_seq (rtx *operands, int nops, bool sort_regs)
 
   if (TARGET_THUMB1)
     {
-      gcc_assert (peep2_reg_dead_p (nops, base_reg_rtx));
       gcc_assert (ldm_case == 1 || ldm_case == 5);
-      write_back = TRUE;
+
+      /* Thumb-1 ldm uses writeback except if the base is loaded.  */
+      write_back = true;
+      for (i = 0; i < nops; i++)
+       if (base_reg == regs[i])
+         write_back = false;
+
+      /* Ensure the base is dead if it is updated.  */
+      if (write_back && !peep2_reg_dead_p (nops, base_reg_rtx))
+       return false;
     }
 
   if (ldm_case == 5)
@@ -13878,8 +13883,7 @@ gen_ldm_seq (rtx *operands, int nops, bool sort_regs)
       rtx newbase = TARGET_THUMB1 ? base_reg_rtx : gen_rtx_REG (SImode, regs[0]);
       emit_insn (gen_addsi3 (newbase, base_reg_rtx, GEN_INT (offset)));
       offset = 0;
-      if (!TARGET_THUMB1)
-       base_reg_rtx = newbase;
+      base_reg_rtx = newbase;
     }
 
   for (i = 0; i < nops; i++)
diff --git a/gcc/testsuite/gcc.target/arm/pr89190.c b/gcc/testsuite/gcc.target/arm/pr89190.c
new file mode 100644
index 0000000000000000000000000000000000000000..e622d7081ed01a6318e2c4b3e07598d495bd7e0f
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr89190.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target arm_arch_v8m_base_ok } */
+/* { dg-options "-O2" } */
+/* { dg-add-options arm_arch_v8m_base } */
+
+long long a;
+int b, c;
+int d(int e, int f) { return e << f; }
+void g() {
+  long long h;
+  char i = d(b >= 7, 2);
+  c = i == 0 ?: 1 / i;
+  h = c && a ?: c + a;
+  b = h;
+}

Hi Wilco,

On 2/13/19 12:42 PM, Wilco Dijkstra wrote:
>
> ping
>
>
> From: Wilco Dijkstra
> Sent: 04 February 2019 14:59
> To: GCC Patches
> Cc: nd; Kyrylo Tkachov
> Subject: [PATCH][ARM] Fix Thumb-1 ldm (PR89190)
>
>
> This patch fixes an ICE in the Thumb-1 LDM peepholer. Thumb-1 LDMs
> always update the base register except if the base is loaded.
> The current implementation rejects LDMs where the base is not dead,
> however this doesn't exclude the case where the base is loaded as
> well as dead.  Fix this by explicitly checking whether the base is
> loaded.  Also enable LDMs which load the first register.
>
> Bootstrap OK on armhf, testsuite passes. OK for commit?
>
Ok.

Thanks,

Kyrill


> ChangeLog:
> 2019-02-04  Wilco Dijkstra  <wdijkstr@arm.com>
>
>         PR target/89190
>         * config/arm/arm.c (ldm_stm_operation_p) Set
>         addr_reg_in_reglist correctly for first register.
>         (load_multiple_sequence): Remove dead base check.
>         (gen_ldm_seq): Correctly set write_back for Thumb-1.
>
>         PR target/89190
>         * gcc.target/arm/pr89190.c: New test.
> --
>
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index 
> 16e22eed871ca34d56c83c334688e5a95970638e..c4c9b4a667100d81d918196713e40b01ee232ee2 
> 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -13191,6 +13191,9 @@ ldm_stm_operation_p (rtx op, bool load, 
> machine_mode mode,
>    if (load && (REGNO (reg) == SP_REGNUM) && (REGNO (addr) != SP_REGNUM))
>      return false;
>
> +  if (regno == REGNO (addr))
> +    addr_reg_in_reglist = true;
> +
>    for (; i < count; i++)
>      {
>        elt = XVECEXP (op, 0, i);
> @@ -13385,7 +13388,6 @@ load_multiple_sequence (rtx *operands, int 
> nops, int *regs, int *saved_order,
>    int unsorted_regs[MAX_LDM_STM_OPS];
>    HOST_WIDE_INT unsorted_offsets[MAX_LDM_STM_OPS];
>    int order[MAX_LDM_STM_OPS];
> -  rtx base_reg_rtx = NULL;
>    int base_reg = -1;
>    int i, ldm_case;
>
> @@ -13430,7 +13432,6 @@ load_multiple_sequence (rtx *operands, int 
> nops, int *regs, int *saved_order,
>            if (i == 0)
>              {
>                base_reg = REGNO (reg);
> -             base_reg_rtx = reg;
>                if (TARGET_THUMB1 && base_reg > LAST_LO_REGNUM)
>                  return 0;
>              }
> @@ -13489,10 +13490,6 @@ load_multiple_sequence (rtx *operands, int 
> nops, int *regs, int *saved_order,
>        *load_offset = unsorted_offsets[order[0]];
>      }
>
> -  if (TARGET_THUMB1
> -      && !peep2_reg_dead_p (nops, base_reg_rtx))
> -    return 0;
> -
>    if (unsorted_offsets[order[0]] == 0)
>      ldm_case = 1; /* ldmia */
>    else if (TARGET_ARM && unsorted_offsets[order[0]] == 4)
> @@ -13868,9 +13865,17 @@ gen_ldm_seq (rtx *operands, int nops, bool 
> sort_regs)
>
>    if (TARGET_THUMB1)
>      {
> -      gcc_assert (peep2_reg_dead_p (nops, base_reg_rtx));
>        gcc_assert (ldm_case == 1 || ldm_case == 5);
> -      write_back = TRUE;
> +
> +      /* Thumb-1 ldm uses writeback except if the base is loaded.  */
> +      write_back = true;
> +      for (i = 0; i < nops; i++)
> +       if (base_reg == regs[i])
> +         write_back = false;
> +
> +      /* Ensure the base is dead if it is updated.  */
> +      if (write_back && !peep2_reg_dead_p (nops, base_reg_rtx))
> +       return false;
>      }
>
>    if (ldm_case == 5)
> @@ -13878,8 +13883,7 @@ gen_ldm_seq (rtx *operands, int nops, bool 
> sort_regs)
>        rtx newbase = TARGET_THUMB1 ? base_reg_rtx : gen_rtx_REG 
> (SImode, regs[0]);
>        emit_insn (gen_addsi3 (newbase, base_reg_rtx, GEN_INT (offset)));
>        offset = 0;
> -      if (!TARGET_THUMB1)
> -       base_reg_rtx = newbase;
> +      base_reg_rtx = newbase;
>      }
>
>    for (i = 0; i < nops; i++)
> diff --git a/gcc/testsuite/gcc.target/arm/pr89190.c 
> b/gcc/testsuite/gcc.target/arm/pr89190.c
> new file mode 100644
> index 
> 0000000000000000000000000000000000000000..e622d7081ed01a6318e2c4b3e07598d495bd7e0f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/pr89190.c
> @@ -0,0 +1,15 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target arm_arch_v8m_base_ok } */
> +/* { dg-options "-O2" } */
> +/* { dg-add-options arm_arch_v8m_base } */
> +
> +long long a;
> +int b, c;
> +int d(int e, int f) { return e << f; }
> +void g() {
> +  long long h;
> +  char i = d(b >= 7, 2);
> +  c = i == 0 ?: 1 / i;
> +  h = c && a ?: c + a;
> +  b = h;
> +}