[patch, ARM] Fix minipool ICE

[patch, ARM] Fix minipool ICE

[Chung-Lin Tang]

[-- Attachment #1: Type: text/plain, Size: 582 bytes --]

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.

[-- Attachment #2: pool_range.patch --]
[-- Type: text/plain, Size: 760 bytes --]

Index: config/arm/arm.md
===================================================================
--- config/arm/arm.md	(revision 169875)
+++ 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"

Re: [patch, ARM] Fix minipool ICE

[Chung-Lin Tang]

On 2011/2/7 02:09 PM, 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.

PR 47719 should be the same ICE.
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47719

Re: [patch, ARM] Fix minipool ICE

[Ramana Radhakrishnan]

Hi Chung-Lin,

On 07/02/2011 06:09, 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?

Can you please add a regression test in gcc.target/arm ? This is a 
regression in trunk compared to GCC 4.5 . Can you mark PR47719 accordingly ?

These attributes were removed because we thought it wasn't possible for 
the patterns to refer to a constant pool.  If this needs to be applied 
to the z-e-hisi2 patterns, I would like to see some analysis to see 
where these get generated and to make sure that this isn't possible for 
the other cases.

See the fix for PR43137 here

http://patchwork.ozlabs.org/patch/58731/

Cheers,
Ramana

>
> 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.
	

Re: [patch, ARM] Fix minipool ICE

[Richard Earnshaw]

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.

R.

Re: [patch, ARM] Fix minipool ICE

[Chung-Lin Tang]

[-- Attachment #1: Type: text/plain, Size: 2477 bytes --]

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


[-- Attachment #2: pr47719.diff --]
[-- Type: text/plain, Size: 1929 bytes --]

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];
+	}
+    }
+}

Re: [patch, ARM] Fix minipool ICE

[Jakub Jelinek]

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

Re: [patch, ARM] Fix minipool ICE

[Richard Earnshaw]

[-- Attachment #1: Type: text/plain, Size: 2463 bytes --]


On Mon, 2011-02-28 at 23:40 +0800, Chung-Lin Tang wrote:
> 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)))
> 

Thanks for the testcase.  It looks like the problems all started when
the insn above was created.  Reload shouldn't have done that, but was
forced to because the constant being spilt wasn't valid in the
arm_movhi_arch4 pattern.  Once that is done we start down the slippery
slope that leads to the ICE.

It's easily fixed by making that pattern accept any constant and then
processing that directly into the minipools rather than letting reload
push it in to the per-function constant pool and then trying to
eliminate that.

We should do more to optimize handling of constants in HImode,
particularly when we have MOVW available.  But for now this patch will
solve the ICE.

2011-03-05  Richard Earnshaw  <rearnsha@arm.com>

	PR target/47719
	* arm.md (movhi_insn_arch4):  Accept any immediate constant.

[-- Attachment #2: himode.patch --]
[-- Type: text/x-patch, Size: 1084 bytes --]

*** config/arm/arm.md	(revision 170711)
--- config/arm/arm.md	(local)
*************** (define_expand "movhi_bigend"
*** 5790,5801 ****
  ;; Pattern to recognize insn generated default case above
  (define_insn "*movhi_insn_arch4"
    [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r")
! 	(match_operand:HI 1 "general_operand"      "rI,K,r,m"))]
    "TARGET_ARM
     && arm_arch4
!    && (GET_CODE (operands[1]) != CONST_INT
!        || const_ok_for_arm (INTVAL (operands[1]))
!        || const_ok_for_arm (~INTVAL (operands[1])))"
    "@
     mov%?\\t%0, %1\\t%@ movhi
     mvn%?\\t%0, #%B1\\t%@ movhi
--- 5790,5800 ----
  ;; Pattern to recognize insn generated default case above
  (define_insn "*movhi_insn_arch4"
    [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r")
! 	(match_operand:HI 1 "general_operand"      "rI,K,r,mi"))]
    "TARGET_ARM
     && arm_arch4
!    && (register_operand (operands[0], HImode)
!        || register_operand (operands[1], HImode))"
    "@
     mov%?\\t%0, %1\\t%@ movhi
     mvn%?\\t%0, #%B1\\t%@ movhi