diff mbox

[ARM] Fix minipool ICE

Message ID 4D6BC1E5.50401@codesourcery.com
State New
Headers show

Commit Message

Chung-Lin Tang Feb. 28, 2011, 3:40 p.m. UTC 
On 2011/2/28 11:07 PM, Richard Earnshaw wrote:
> 
> On Mon, 2011-02-07 at 14:09 +0800, Chung-Lin Tang wrote:
>> Hi,
>> the *arm_zero_extendhisi2[_v6] patterns currently do not have the
>> constant pool range attributes specified, causing a minipool ICE case.
>> This patch adds the needed pool_range/neg_pool_range settings.
>>
>> Reported originally from https://bugs.launchpad.net/bugs/711819 , also
>> happens to occur when building ffmpeg svn trunk.
>>
>> Ok for trunk?
>>
>> Thanks,
>> Chung-Lin
>>
>> 2011-02-07  Chung-Lin Tang  <cltang@codesourcery.com>
>>
>>         * config/arm/arm.md (*arm_zero_extendhisi2): Set pool_range,
>>         neg_pool_range attributes for ldrh alternative.
>>         (*arm_zero_extendhisi2_v6): Same.
> 
> This is wrong.  Neither the predicate nor the constraint accept an
> immediate, so these should never need to generate mini-pool entries.  If
> reload is letting these through, then that's a bug in reload IMO.  If
> it's not reload, then that needs investigating further too.

Hi Richard,

To re-cap some of the discussion Bernd, Ramana, and I had off-list: we
are seeing in some cases, IRA/reload ending up with insns with constant
pool references (not generated from ARM reorg):
(insn 262 92 94 3 (set (reg:HI 4 r4 [orig:328 iftmp.6 ] [328])
        (mem/u/c/i:HI (symbol_ref/u:SI ("*.LC0") [flags 0x2]) [2 S2
A16])) k.c:11 177 {*movhi_insn_arch4}
     (expr_list:REG_EQUAL (const_int 2047 [0x7ff])
        (nil)))

It looks like IRA spilled a register which is a constant. Probably
because 2047 is not a valid ARM movable constant, it gets turned into a
constant pool reference.


After postreload, the above insn gets turned into:
(insn 262 92 94 3 (set (reg:SI 4 r4)
        (zero_extend:SI (mem/u/c/i:HI (symbol_ref/u:SI ("*.LC0") [flags
0x2]) [2 S2 A16]))) k.c:11 148 {*arm_zero_extendhisi2_v6}
     (expr_list:REG_EQUAL (const_int 2047 [0x7ff])
        (nil)))

Thus arm.c:note_invalid_constants() kicks in, then ICEing later

I haven't yet investigated where exactly postreload produces that
zero_extend (maybe someone here can be quick to point out where), but if
it's correct behavior, then I guess the pool range attributes have to be
there...

Ramana also mentioned about using MOVW for doing HImode constant moves,
which currently do not seem to be supported under ARM mode (only
Thumb-2). This could be added, but still the pre-ARMv6T2 case needs solving.

Here's the patch with a testcase added.

Thanks,
Chung-Lin

Comments

Jakub Jelinek March 2, 2011, 11:58 a.m. UTC | #1 
On Mon, Feb 28, 2011 at 11:40:21PM +0800, Chung-Lin Tang wrote:
> > This is wrong.  Neither the predicate nor the constraint accept an
> > immediate, so these should never need to generate mini-pool entries.  If
> > reload is letting these through, then that's a bug in reload IMO.  If

No.  arm backend asked for it by defining LOAD_EXTEND_OP.

> > it's not reload, then that needs investigating further too.
> 
> To re-cap some of the discussion Bernd, Ramana, and I had off-list: we
> are seeing in some cases, IRA/reload ending up with insns with constant
> pool references (not generated from ARM reorg):
> (insn 262 92 94 3 (set (reg:HI 4 r4 [orig:328 iftmp.6 ] [328])
>         (mem/u/c/i:HI (symbol_ref/u:SI ("*.LC0") [flags 0x2]) [2 S2
> A16])) k.c:11 177 {*movhi_insn_arch4}
>      (expr_list:REG_EQUAL (const_int 2047 [0x7ff])
>         (nil)))
> 
> It looks like IRA spilled a register which is a constant. Probably
> because 2047 is not a valid ARM movable constant, it gets turned into a
> constant pool reference.
> 
> 
> After postreload, the above insn gets turned into:
> (insn 262 92 94 3 (set (reg:SI 4 r4)
>         (zero_extend:SI (mem/u/c/i:HI (symbol_ref/u:SI ("*.LC0") [flags
> 0x2]) [2 S2 A16]))) k.c:11 148 {*arm_zero_extendhisi2_v6}
>      (expr_list:REG_EQUAL (const_int 2047 [0x7ff])
>         (nil)))
> 
> Thus arm.c:note_invalid_constants() kicks in, then ICEing later
> 
> I haven't yet investigated where exactly postreload produces that
> zero_extend (maybe someone here can be quick to point out where), but if
> it's correct behavior, then I guess the pool range attributes have to be
> there...

The zero_extend is created because arm defines LOAD_EXTEND_OP. 
reload_cse_simplify_operands has:

#ifdef LOAD_EXTEND_OP
      if (MEM_P (op)
          && GET_MODE_BITSIZE (GET_MODE (op)) < BITS_PER_WORD 
          && LOAD_EXTEND_OP (GET_MODE (op)) != UNKNOWN)
        {
...
          /* If this is a straight load, make the extension explicit.  */
          else if (REG_P (SET_DEST (set))
                   && recog_data.n_operands == 2
                   && SET_SRC (set) == op
                   && SET_DEST (set) == recog_data.operand[1-i])
            {
              validate_change (insn, recog_data.operand_loc[i],
                               gen_rtx_fmt_e (LOAD_EXTEND_OP (GET_MODE (op)),
                                              word_mode, op),
                               1);
              validate_change (insn, recog_data.operand_loc[1-i],
                               gen_rtx_REG (word_mode, REGNO (SET_DEST (set))),
                               1);
              if (! apply_change_group ())
                return 0;

This isn't particularly new code, it was added for PR11864 more than 7 years
ago.

	Jakub
diff mbox

Patch

Index: gcc/config/arm/arm.md
===================================================================
--- gcc/config/arm/arm.md	(revision 170534)
+++ gcc/config/arm/arm.md	(working copy)
@@ -4202,7 +4202,9 @@ 
    #
    ldr%(h%)\\t%0, %1"
   [(set_attr "type" "alu_shift,load_byte")
-   (set_attr "predicable" "yes")]
+   (set_attr "predicable" "yes")
+   (set_attr "pool_range" "*,256")
+   (set_attr "neg_pool_range" "*,244")]
 )
 
 (define_insn "*arm_zero_extendhisi2_v6"
@@ -4213,7 +4215,9 @@ 
    uxth%?\\t%0, %1
    ldr%(h%)\\t%0, %1"
   [(set_attr "type" "alu_shift,load_byte")
-   (set_attr "predicable" "yes")]
+   (set_attr "predicable" "yes")
+   (set_attr "pool_range" "*,256")
+   (set_attr "neg_pool_range" "*,244")]
 )
 
 (define_insn "*arm_zero_extendhisi2addsi"
Index: gcc/testsuite/gcc.target/arm/pr47719.c
===================================================================
--- gcc/testsuite/gcc.target/arm/pr47719.c	(revision 0)
+++ gcc/testsuite/gcc.target/arm/pr47719.c	(revision 0)
@@ -0,0 +1,45 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O3 -march=armv5te -marm" } */
+
+static inline short baz (int a)
+{
+  int x;
+  __asm__ ("mov %0, %1" : "=r"(x) : "r"(a));
+  return x;
+}
+
+static void bar (short *out, unsigned char *in)
+{
+  int sc = ((unsigned short)*in << 8);
+  int i, s0, s1, d;
+  in += 2;
+  s1 = *in;
+  for(i=0;i<16;i++)
+    {
+      d = in[i];
+      d = ((signed char)d >> 4);
+      s0 = (0x4000*d*sc + 0x7298*s1 - 0x3350*s1)>>14;
+      s1 = baz (s0);
+      *out++=s1;
+      d = in[i];
+      d = ((signed char)(d<<4) >> 4);
+      s0 = (0x4000*d*sc + 0x7298*s1 - 0x3350*s1)>>14;
+      s1 = baz (s0);
+      *out++=s1;
+    }
+}
+
+void foo (int c, void *data, int size)
+{
+  int i;
+  short *samples = data, tmp[64];
+  while(size-- >= 32)
+    {
+      bar (tmp, data);
+      for (i = 0; i < 32; i++)
+	{
+	  samples[i*2] = tmp[i];
+	  samples[i*2+1] = tmp[i+32];
+	}
+    }
+}