* [PATCH, ARM] Fix PR44557, Thumb-1 ICE
@ 2010-12-09 10:23 Chung-Lin Tang
2010-12-20 19:26 ` Richard Earnshaw
[not found] ` <1292868188.24737.28.camel@e102346-lin.cambridge.arm.com>
0 siblings, 2 replies; 6+ messages in thread
From: Chung-Lin Tang @ 2010-12-09 10:23 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 608 bytes --]
Hi,
this patch fixes the ICE in PR44557. It now occurs on trunk only under
quite specific option conditions, but debugging this PR leaded to quite
obvious Thumb-1 fixes.
Also added a simplified testcase, derived from the one on bugzilla.
Tested without regressions, okay to commit to trunk?
Thanks,
Chung-Lin
2010-12-09 Chung-Lin Tang <cltang@codesourcery.com>
PR target/44557
* config/arm/arm.h (PREFERRED_RELOAD_CLASS): Add CORE_REGS to
Thumb-1 return LO_REGS case.
* config/arm/arm.md (reload_inhi): Change scratch constraint
from 'r' to 'l'.
testsuite/
* gcc.target/arm/pr44557.c: New.
[-- Attachment #2: pr44557.patch --]
[-- Type: text/plain, Size: 1654 bytes --]
Index: gcc/config/arm/arm.h
===================================================================
--- gcc/config/arm/arm.h (revision 167626)
+++ gcc/config/arm/arm.h (working copy)
@@ -1207,6 +1207,7 @@
(TARGET_32BIT ? (CLASS) : \
((CLASS) == GENERAL_REGS || (CLASS) == HI_REGS \
|| (CLASS) == NO_REGS || (CLASS) == STACK_REG \
+ || (CLASS) == CORE_REGS \
? LO_REGS : (CLASS)))
/* Must leave BASE_REGS reloads alone */
Index: gcc/config/arm/arm.md
===================================================================
--- gcc/config/arm/arm.md (revision 167626)
+++ gcc/config/arm/arm.md (working copy)
@@ -5850,7 +5850,7 @@
(define_expand "reload_inhi"
[(parallel [(match_operand:HI 0 "s_register_operand" "=r")
(match_operand:HI 1 "arm_reload_memory_operand" "o")
- (match_operand:DI 2 "s_register_operand" "=&r")])]
+ (match_operand:DI 2 "s_register_operand" "=&l")])]
"TARGET_EITHER"
"
if (TARGET_ARM)
Index: gcc/testsuite/gcc.target/arm/pr44557.c
===================================================================
--- gcc/testsuite/gcc.target/arm/pr44557.c (revision 0)
+++ gcc/testsuite/gcc.target/arm/pr44557.c (revision 0)
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-options "-mthumb -O1 -march=armv5te -fno-omit-frame-pointer -fno-forward-propagate" } */
+/* { dg-require-effective-target arm_thumb1_ok } */
+
+struct S
+{
+ short x, y;
+};
+
+void foo (struct S *p, struct S *q, char *t, int n)
+{
+ struct S *c, d;
+ int x = 1;
+
+ while (n--)
+ {
+ if (*t && p)
+ c = p;
+ q->x = d.x + c->x + c->y;
+ if (x)
+ {
+ x = 0;
+ d.x += c->x;
+ }
+ }
+}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH, ARM] Fix PR44557, Thumb-1 ICE
2010-12-09 10:23 [PATCH, ARM] Fix PR44557, Thumb-1 ICE Chung-Lin Tang
@ 2010-12-20 19:26 ` Richard Earnshaw
[not found] ` <1292868188.24737.28.camel@e102346-lin.cambridge.arm.com>
1 sibling, 0 replies; 6+ messages in thread
From: Richard Earnshaw @ 2010-12-20 19:26 UTC (permalink / raw)
To: Chung-Lin Tang; +Cc: gcc-patches
On Thu, 2010-12-09 at 14:08 +0800, Chung-Lin Tang wrote:
> Hi,
> this patch fixes the ICE in PR44557. It now occurs on trunk only under
> quite specific option conditions, but debugging this PR leaded to quite
> obvious Thumb-1 fixes.
>
> Also added a simplified testcase, derived from the one on bugzilla.
> Tested without regressions, okay to commit to trunk?
>
> Thanks,
> Chung-Lin
>
> 2010-12-09 Chung-Lin Tang <cltang@codesourcery.com>
>
> PR target/44557
> * config/arm/arm.h (PREFERRED_RELOAD_CLASS): Add CORE_REGS to
> Thumb-1 return LO_REGS case.
> * config/arm/arm.md (reload_inhi): Change scratch constraint
> from 'r' to 'l'.
>
> testsuite/
> * gcc.target/arm/pr44557.c: New.
>
Changing the scratch constraint to 'l' is going to pessimize thumb2 code
reload generation which can quite reasonably take an 'r' class here.
I'm not sure there's an easy way to fix this without going the whole hog
and getting rid of reload_inhi entirely (which would be a good thing,
TM) and replacing it with the new reload infrastructure that Joern wrote
a few years back.
Before approving this I want to be sure that it doesn't cause major
problems on Thumb-2. What testing have you done for that configuration?
R.
PS: Maybe another way to fix this would be to introduce a new register
class that expands to GENERAL_REGS on 32-bit cores and LO_REGS on
Thumb-1 only.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH, ARM] Fix PR44557, Thumb-1 ICE
[not found] ` <1292868188.24737.28.camel@e102346-lin.cambridge.arm.com>
@ 2011-01-01 11:21 ` Chung-Lin Tang
2011-01-26 3:46 ` Ping " Chung-Lin Tang
0 siblings, 1 reply; 6+ messages in thread
From: Chung-Lin Tang @ 2011-01-01 11:21 UTC (permalink / raw)
To: Richard Earnshaw; +Cc: gcc-patches
On 2010/12/21 02:03, Richard Earnshaw wrote:
>
> On Thu, 2010-12-09 at 14:08 +0800, Chung-Lin Tang wrote:
>> Hi,
>> this patch fixes the ICE in PR44557. It now occurs on trunk only under
>> quite specific option conditions, but debugging this PR leaded to quite
>> obvious Thumb-1 fixes.
>>
>> Also added a simplified testcase, derived from the one on bugzilla.
>> Tested without regressions, okay to commit to trunk?
>>
>> Thanks,
>> Chung-Lin
>>
>> 2010-12-09 Chung-Lin Tang <cltang@codesourcery.com>
>>
>> PR target/44557
>> * config/arm/arm.h (PREFERRED_RELOAD_CLASS): Add CORE_REGS to
>> Thumb-1 return LO_REGS case.
>> * config/arm/arm.md (reload_inhi): Change scratch constraint
>> from 'r' to 'l'.
>>
>> testsuite/
>> * gcc.target/arm/pr44557.c: New.
>>
>
> Changing the scratch constraint to 'l' is going to pessimize thumb2 code
> reload generation which can quite reasonably take an 'r' class here.
>
> I'm not sure there's an easy way to fix this without going the whole hog
> and getting rid of reload_inhi entirely (which would be a good thing,
> TM) and replacing it with the new reload infrastructure that Joern wrote
> a few years back.
>
> Before approving this I want to be sure that it doesn't cause major
> problems on Thumb-2. What testing have you done for that configuration?
>
> R.
>
> PS: Maybe another way to fix this would be to introduce a new register
> class that expands to GENERAL_REGS on 32-bit cores and LO_REGS on
> Thumb-1 only.
>
Hi Richard,
My testing was for Thumb-1 (-march=armv5te -mthumb), as that was what
the ICE was about. I later tried another test run with Thumb-2 as you
suggested, to be sure, and saw no regressions too.
I've then been looking into the secondary reload logic. The ARM
backend's SECONDARY_INPUT/OUTPUT_RELOAD_CLASS macros already returns
NO_REGS under TARGET_32BIT for integer modes (no secondary reloads
needed). Looking at the default_secondary_reload() function, this means
that the reload_in/outhi patterns are never used for Thumb-2.
So this patch should only affect Thumb-1, and the Thumb-2 considerations
should be unneeded.
As for the issue of replacing all of this with the more modern
TARGET_SECONDARY_RELOAD machinery, that does seem attractive. Do you
think such a patch can get into 4.6?
Thanks,
Chung-Lin
ps. side question, the arm_reload_in/out_hi() functions does quite some
tricks to do halfword-load/storing. This should be for pre-ARMv4 only
right? (before the ldrh/strh instructions became available)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Ping Re: [PATCH, ARM] Fix PR44557, Thumb-1 ICE
2011-01-01 11:21 ` Chung-Lin Tang
@ 2011-01-26 3:46 ` Chung-Lin Tang
2011-03-07 14:11 ` Chung-Lin Tang
0 siblings, 1 reply; 6+ messages in thread
From: Chung-Lin Tang @ 2011-01-26 3:46 UTC (permalink / raw)
To: Richard Earnshaw; +Cc: gcc-patches
On 2011/1/1 19:21, Chung-Lin Tang wrote:
> On 2010/12/21 02:03, Richard Earnshaw wrote:
>>
>> On Thu, 2010-12-09 at 14:08 +0800, Chung-Lin Tang wrote:
>>> Hi,
>>> this patch fixes the ICE in PR44557. It now occurs on trunk only under
>>> quite specific option conditions, but debugging this PR leaded to quite
>>> obvious Thumb-1 fixes.
>>>
>>> Also added a simplified testcase, derived from the one on bugzilla.
>>> Tested without regressions, okay to commit to trunk?
>>>
>>> Thanks,
>>> Chung-Lin
>>>
>>> 2010-12-09 Chung-Lin Tang <cltang@codesourcery.com>
>>>
>>> PR target/44557
>>> * config/arm/arm.h (PREFERRED_RELOAD_CLASS): Add CORE_REGS to
>>> Thumb-1 return LO_REGS case.
>>> * config/arm/arm.md (reload_inhi): Change scratch constraint
>>> from 'r' to 'l'.
>>>
>>> testsuite/
>>> * gcc.target/arm/pr44557.c: New.
>>>
>>
>> Changing the scratch constraint to 'l' is going to pessimize thumb2 code
>> reload generation which can quite reasonably take an 'r' class here.
>>
>> I'm not sure there's an easy way to fix this without going the whole hog
>> and getting rid of reload_inhi entirely (which would be a good thing,
>> TM) and replacing it with the new reload infrastructure that Joern wrote
>> a few years back.
>>
>> Before approving this I want to be sure that it doesn't cause major
>> problems on Thumb-2. What testing have you done for that configuration?
>>
>> R.
>>
>> PS: Maybe another way to fix this would be to introduce a new register
>> class that expands to GENERAL_REGS on 32-bit cores and LO_REGS on
>> Thumb-1 only.
>>
>
> Hi Richard,
>
> My testing was for Thumb-1 (-march=armv5te -mthumb), as that was what
> the ICE was about. I later tried another test run with Thumb-2 as you
> suggested, to be sure, and saw no regressions too.
>
> I've then been looking into the secondary reload logic. The ARM
> backend's SECONDARY_INPUT/OUTPUT_RELOAD_CLASS macros already returns
> NO_REGS under TARGET_32BIT for integer modes (no secondary reloads
> needed). Looking at the default_secondary_reload() function, this means
> that the reload_in/outhi patterns are never used for Thumb-2.
>
> So this patch should only affect Thumb-1, and the Thumb-2 considerations
> should be unneeded.
Hi Richard, pinging this patch.
I think I've explained the effects of this fix, and it should be Thumb-1
only. Is this patch okay?
Thanks,
Chung-Lin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Ping Re: [PATCH, ARM] Fix PR44557, Thumb-1 ICE
2011-01-26 3:46 ` Ping " Chung-Lin Tang
@ 2011-03-07 14:11 ` Chung-Lin Tang
2011-03-15 1:40 ` Ping^3 " Chung-Lin Tang
0 siblings, 1 reply; 6+ messages in thread
From: Chung-Lin Tang @ 2011-03-07 14:11 UTC (permalink / raw)
To: Richard Earnshaw; +Cc: gcc-patches
Ping.
On 2011/1/26 11:07 AM, Chung-Lin Tang wrote:
> On 2011/1/1 19:21, Chung-Lin Tang wrote:
>> On 2010/12/21 02:03, Richard Earnshaw wrote:
>>>
>>> On Thu, 2010-12-09 at 14:08 +0800, Chung-Lin Tang wrote:
>>>> Hi,
>>>> this patch fixes the ICE in PR44557. It now occurs on trunk only under
>>>> quite specific option conditions, but debugging this PR leaded to quite
>>>> obvious Thumb-1 fixes.
>>>>
>>>> Also added a simplified testcase, derived from the one on bugzilla.
>>>> Tested without regressions, okay to commit to trunk?
>>>>
>>>> Thanks,
>>>> Chung-Lin
>>>>
>>>> 2010-12-09 Chung-Lin Tang <cltang@codesourcery.com>
>>>>
>>>> PR target/44557
>>>> * config/arm/arm.h (PREFERRED_RELOAD_CLASS): Add CORE_REGS to
>>>> Thumb-1 return LO_REGS case.
>>>> * config/arm/arm.md (reload_inhi): Change scratch constraint
>>>> from 'r' to 'l'.
>>>>
>>>> testsuite/
>>>> * gcc.target/arm/pr44557.c: New.
>>>>
>>>
>>> Changing the scratch constraint to 'l' is going to pessimize thumb2 code
>>> reload generation which can quite reasonably take an 'r' class here.
>>>
>>> I'm not sure there's an easy way to fix this without going the whole hog
>>> and getting rid of reload_inhi entirely (which would be a good thing,
>>> TM) and replacing it with the new reload infrastructure that Joern wrote
>>> a few years back.
>>>
>>> Before approving this I want to be sure that it doesn't cause major
>>> problems on Thumb-2. What testing have you done for that configuration?
>>>
>>> R.
>>>
>>> PS: Maybe another way to fix this would be to introduce a new register
>>> class that expands to GENERAL_REGS on 32-bit cores and LO_REGS on
>>> Thumb-1 only.
>>>
>>
>> Hi Richard,
>>
>> My testing was for Thumb-1 (-march=armv5te -mthumb), as that was what
>> the ICE was about. I later tried another test run with Thumb-2 as you
>> suggested, to be sure, and saw no regressions too.
>>
>> I've then been looking into the secondary reload logic. The ARM
>> backend's SECONDARY_INPUT/OUTPUT_RELOAD_CLASS macros already returns
>> NO_REGS under TARGET_32BIT for integer modes (no secondary reloads
>> needed). Looking at the default_secondary_reload() function, this means
>> that the reload_in/outhi patterns are never used for Thumb-2.
>>
>> So this patch should only affect Thumb-1, and the Thumb-2 considerations
>> should be unneeded.
>
> Hi Richard, pinging this patch.
> I think I've explained the effects of this fix, and it should be Thumb-1
> only. Is this patch okay?
>
> Thanks,
> Chung-Lin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Ping^3 Re: [PATCH, ARM] Fix PR44557, Thumb-1 ICE
2011-03-07 14:11 ` Chung-Lin Tang
@ 2011-03-15 1:40 ` Chung-Lin Tang
0 siblings, 0 replies; 6+ messages in thread
From: Chung-Lin Tang @ 2011-03-15 1:40 UTC (permalink / raw)
To: Richard Earnshaw; +Cc: gcc-patches
Ping.
On 2011/3/7 10:11 PM, Chung-Lin Tang wrote:
> Ping.
>
> On 2011/1/26 11:07 AM, Chung-Lin Tang wrote:
>> On 2011/1/1 19:21, Chung-Lin Tang wrote:
>>> On 2010/12/21 02:03, Richard Earnshaw wrote:
>>>>
>>>> On Thu, 2010-12-09 at 14:08 +0800, Chung-Lin Tang wrote:
>>>>> Hi,
>>>>> this patch fixes the ICE in PR44557. It now occurs on trunk only under
>>>>> quite specific option conditions, but debugging this PR leaded to quite
>>>>> obvious Thumb-1 fixes.
>>>>>
>>>>> Also added a simplified testcase, derived from the one on bugzilla.
>>>>> Tested without regressions, okay to commit to trunk?
>>>>>
>>>>> Thanks,
>>>>> Chung-Lin
>>>>>
>>>>> 2010-12-09 Chung-Lin Tang <cltang@codesourcery.com>
>>>>>
>>>>> PR target/44557
>>>>> * config/arm/arm.h (PREFERRED_RELOAD_CLASS): Add CORE_REGS to
>>>>> Thumb-1 return LO_REGS case.
>>>>> * config/arm/arm.md (reload_inhi): Change scratch constraint
>>>>> from 'r' to 'l'.
>>>>>
>>>>> testsuite/
>>>>> * gcc.target/arm/pr44557.c: New.
>>>>>
>>>>
>>>> Changing the scratch constraint to 'l' is going to pessimize thumb2 code
>>>> reload generation which can quite reasonably take an 'r' class here.
>>>>
>>>> I'm not sure there's an easy way to fix this without going the whole hog
>>>> and getting rid of reload_inhi entirely (which would be a good thing,
>>>> TM) and replacing it with the new reload infrastructure that Joern wrote
>>>> a few years back.
>>>>
>>>> Before approving this I want to be sure that it doesn't cause major
>>>> problems on Thumb-2. What testing have you done for that configuration?
>>>>
>>>> R.
>>>>
>>>> PS: Maybe another way to fix this would be to introduce a new register
>>>> class that expands to GENERAL_REGS on 32-bit cores and LO_REGS on
>>>> Thumb-1 only.
>>>>
>>>
>>> Hi Richard,
>>>
>>> My testing was for Thumb-1 (-march=armv5te -mthumb), as that was what
>>> the ICE was about. I later tried another test run with Thumb-2 as you
>>> suggested, to be sure, and saw no regressions too.
>>>
>>> I've then been looking into the secondary reload logic. The ARM
>>> backend's SECONDARY_INPUT/OUTPUT_RELOAD_CLASS macros already returns
>>> NO_REGS under TARGET_32BIT for integer modes (no secondary reloads
>>> needed). Looking at the default_secondary_reload() function, this means
>>> that the reload_in/outhi patterns are never used for Thumb-2.
>>>
>>> So this patch should only affect Thumb-1, and the Thumb-2 considerations
>>> should be unneeded.
>>
>> Hi Richard, pinging this patch.
>> I think I've explained the effects of this fix, and it should be Thumb-1
>> only. Is this patch okay?
>>
>> Thanks,
>> Chung-Lin
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-03-15 1:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-12-09 10:23 [PATCH, ARM] Fix PR44557, Thumb-1 ICE Chung-Lin Tang
2010-12-20 19:26 ` Richard Earnshaw
[not found] ` <1292868188.24737.28.camel@e102346-lin.cambridge.arm.com>
2011-01-01 11:21 ` Chung-Lin Tang
2011-01-26 3:46 ` Ping " Chung-Lin Tang
2011-03-07 14:11 ` Chung-Lin Tang
2011-03-15 1:40 ` Ping^3 " Chung-Lin Tang
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).