Patchwork [ARM] Fix PR44557, Thumb-1 ICE

login
register
mail settings
Submitter Chung-Lin Tang
Date Dec. 9, 2010, 6:08 a.m.
Message ID <4D007252.7060102@codesourcery.com>
Download mbox | patch
Permalink /patch/74852/
State New
Headers show

Comments

Chung-Lin Tang - Dec. 9, 2010, 6:08 a.m.
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.
Richard Earnshaw - Dec. 20, 2010, 6:03 p.m.
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.
Chung-Lin Tang - Jan. 1, 2011, 11:21 a.m.
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)
Chung-Lin Tang - Jan. 26, 2011, 3:07 a.m.
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
Chung-Lin Tang - March 7, 2011, 2:11 p.m.
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
Chung-Lin Tang - March 15, 2011, 1:40 a.m.
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
>

Patch

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