* [patch, ARM] Fix minipool ICE
@ 2011-02-07 6:09 Chung-Lin Tang
2011-02-17 10:01 ` Chung-Lin Tang
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Chung-Lin Tang @ 2011-02-07 6:09 UTC (permalink / raw)
To: gcc-patches; +Cc: Richard Earnshaw, Revital1 Eres
[-- 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"
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch, ARM] Fix minipool ICE
2011-02-07 6:09 [patch, ARM] Fix minipool ICE Chung-Lin Tang
@ 2011-02-17 10:01 ` Chung-Lin Tang
2011-02-25 11:02 ` Ramana Radhakrishnan
2011-02-28 15:40 ` Richard Earnshaw
2 siblings, 0 replies; 7+ messages in thread
From: Chung-Lin Tang @ 2011-02-17 10:01 UTC (permalink / raw)
To: gcc-patches
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch, ARM] Fix minipool ICE
2011-02-07 6:09 [patch, ARM] Fix minipool ICE Chung-Lin Tang
2011-02-17 10:01 ` Chung-Lin Tang
@ 2011-02-25 11:02 ` Ramana Radhakrishnan
2011-02-28 15:40 ` Richard Earnshaw
2 siblings, 0 replies; 7+ messages in thread
From: Ramana Radhakrishnan @ 2011-02-25 11:02 UTC (permalink / raw)
To: Chung-Lin Tang; +Cc: gcc-patches, Richard Earnshaw, Revital1 Eres
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch, ARM] Fix minipool ICE
2011-02-07 6:09 [patch, ARM] Fix minipool ICE Chung-Lin Tang
2011-02-17 10:01 ` Chung-Lin Tang
2011-02-25 11:02 ` Ramana Radhakrishnan
@ 2011-02-28 15:40 ` Richard Earnshaw
2011-02-28 16:43 ` Chung-Lin Tang
2 siblings, 1 reply; 7+ messages in thread
From: Richard Earnshaw @ 2011-02-28 15:40 UTC (permalink / raw)
To: Chung-Lin Tang; +Cc: gcc-patches, Revital1 Eres
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch, ARM] Fix minipool ICE
2011-02-28 15:40 ` Richard Earnshaw
@ 2011-02-28 16:43 ` Chung-Lin Tang
2011-03-02 11:58 ` Jakub Jelinek
2011-03-05 14:09 ` Richard Earnshaw
0 siblings, 2 replies; 7+ messages in thread
From: Chung-Lin Tang @ 2011-02-28 16:43 UTC (permalink / raw)
To: gcc-patches
[-- 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];
+ }
+ }
+}
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch, ARM] Fix minipool ICE
2011-02-28 16:43 ` Chung-Lin Tang
@ 2011-03-02 11:58 ` Jakub Jelinek
2011-03-05 14:09 ` Richard Earnshaw
1 sibling, 0 replies; 7+ messages in thread
From: Jakub Jelinek @ 2011-03-02 11:58 UTC (permalink / raw)
To: Chung-Lin Tang; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch, ARM] Fix minipool ICE
2011-02-28 16:43 ` Chung-Lin Tang
2011-03-02 11:58 ` Jakub Jelinek
@ 2011-03-05 14:09 ` Richard Earnshaw
1 sibling, 0 replies; 7+ messages in thread
From: Richard Earnshaw @ 2011-03-05 14:09 UTC (permalink / raw)
To: Chung-Lin Tang; +Cc: gcc-patches
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-03-05 14:09 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-07 6:09 [patch, ARM] Fix minipool ICE Chung-Lin Tang
2011-02-17 10:01 ` Chung-Lin Tang
2011-02-25 11:02 ` Ramana Radhakrishnan
2011-02-28 15:40 ` Richard Earnshaw
2011-02-28 16:43 ` Chung-Lin Tang
2011-03-02 11:58 ` Jakub Jelinek
2011-03-05 14:09 ` Richard Earnshaw
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).