Patchwork Fix PR54814

login
register
mail settings
Submitter Georg-Johann Lay
Date Jan. 25, 2013, 6:41 p.m.
Message ID <5102D1CD.4020509@gjlay.de>
Download mbox | patch
Permalink /patch/215826/
State New
Headers show

Comments

Georg-Johann Lay - Jan. 25, 2013, 6:41 p.m.
PR54814 causes spill fails because reload.c:find_valid_class_1 tests only one
hard register instead of all hard registers of regno:mode in rclass:

http://gcc.gnu.org/PR54814


The patch was originally worked out by Bernd Schmidt and fixed a problem
introduced in

http://gcc.gnu.org/r190252

The patch is bootstrapped and tested on x86_64-linux and also fixes the spill
fails that originally occured on avr-unknown-one.

Ok to apply?


	PR other/54814
	* reload.c (find_valid_class_1): Use in_hard_reg_set_p instead of
	TEST_HARD_REG_BIT.
Jeff Law - Jan. 25, 2013, 7:50 p.m.
On 01/25/2013 11:41 AM, Georg-Johann Lay wrote:
> PR54814 causes spill fails because reload.c:find_valid_class_1 tests only one
> hard register instead of all hard registers of regno:mode in rclass:
>
> http://gcc.gnu.org/PR54814
>
>
> The patch was originally worked out by Bernd Schmidt and fixed a problem
> introduced in
>
> http://gcc.gnu.org/r190252
>
> The patch is bootstrapped and tested on x86_64-linux and also fixes the spill
> fails that originally occured on avr-unknown-one.
>
> Ok to apply?
>
>
> 	PR other/54814
> 	* reload.c (find_valid_class_1): Use in_hard_reg_set_p instead of
> 	TEST_HARD_REG_BIT.
Is this a regression relative to a prior version of GCC?  If not, it'll 
probably need release manager approval before it can go in.

Please attach your patch to PR54814 and attach PR 54814 to the 4.9 
pending patches meta bug.

jeff
Georg-Johann Lay - Jan. 25, 2013, 8:49 p.m.
Jeff Law schrieb:
> On 01/25/2013 11:41 AM, Georg-Johann Lay wrote:
>> PR54814 causes spill fails because reload.c:find_valid_class_1 tests 
>> only one
>> hard register instead of all hard registers of regno:mode in rclass:
>>
>> http://gcc.gnu.org/PR54814
>>
>>
>> The patch was originally worked out by Bernd Schmidt and fixed a problem
>> introduced in
>>
>> http://gcc.gnu.org/r190252
>>
>> The patch is bootstrapped and tested on x86_64-linux and also fixes 
>> the spill
>> fails that originally occurred on avr-unknown-one.
>>
>> Ok to apply?
>>
>>
>>     PR other/54814
>>     * reload.c (find_valid_class_1): Use in_hard_reg_set_p instead of
>>     TEST_HARD_REG_BIT.
> Is this a regression relative to a prior version of GCC?

Yes, it's 4.8 regression.  4.7 works and r190252 was only applied to 4.8 
trunk.

> If not, it'll probably need release manager approval before it can go in.
> 
> Please attach your patch to PR54814 and attach PR 54814 to the 4.9 
> pending patches meta bug.

The patch *is* already attached, I can attach it again if that helps.

Johann
Georg-Johann Lay - Jan. 27, 2013, 10:09 p.m.
Georg-Johann Lay schrieb:
> Jeff Law schrieb:
>> On 01/25/2013 11:41 AM, Georg-Johann Lay wrote:
>>> PR54814 causes spill fails because reload.c:find_valid_class_1 tests 
>>> only one
>>> hard register instead of all hard registers of regno:mode in rclass:
>>>
>>> http://gcc.gnu.org/PR54814
>>>
>>>
>>> The patch was originally worked out by Bernd Schmidt and fixed a problem
>>> introduced in
>>>
>>> http://gcc.gnu.org/r190252
>>>
>>> The patch is bootstrapped and tested on x86_64-linux and also fixes 
>>> the spill
>>> fails that originally occurred on avr-unknown-one.
>>>
>>> Ok to apply?
>>>
>>>
>>>     PR other/54814
>>>     * reload.c (find_valid_class_1): Use in_hard_reg_set_p instead of
>>>     TEST_HARD_REG_BIT.
>> Is this a regression relative to a prior version of GCC?
> 
> Yes, it's 4.8 regression.  4.7 works and r190252 was only applied to 4.8 
> trunk.
> 
>> If not, it'll probably need release manager approval before it can go in.
>>
>> Please attach your patch to PR54814 and attach PR 54814 to the 4.9 
>> pending patches meta bug.

Does this mean the fix is rejected for 4.8?

I found no "4.9 meta-bug" in the 47 meta-bugs. You have th PR?

http://gcc.gnu.org/bugzilla/buglist.cgi?keywords=meta-bug%2C%20&keywords_type=allwords&list_id=51998&cf_known_to_fail_type=allwords&cf_known_to_work_type=allwords&query_format=advanced&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=SUSPENDED&bug_status=WAITING&bug_status=REOPENED&product=gcc

FYI, this bug breaks the avr port almost completely.  Many real-world 
programs ICE.
Marc Glisse - Jan. 27, 2013, 10:15 p.m.
On Sun, 27 Jan 2013, Georg-Johann Lay wrote:

> I found no "4.9 meta-bug" in the 47 meta-bugs. You have th PR?

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55996
Steven Bosscher - Jan. 27, 2013, 10:26 p.m.
On Sun, Jan 27, 2013 at 11:09 PM, Georg-Johann Lay wrote:
>>>> The patch was originally worked out by Bernd Schmidt and fixed a problem
>>>> introduced in
>>>>
>>>> http://gcc.gnu.org/r190252

Ironically, this revision fixes a reload problem on x86/x86_64 --
which doesn't use reload anymore now...


> Does this mean the fix is rejected for 4.8?

No, just that it probably helps to add a RM to the CC list.

FWIW, it seems to me that this patch should go into 4.8, because the
bug is probably not limited to AVR.

Ciao!
Steven
Richard Guenther - Jan. 28, 2013, 10:25 a.m.
On Sun, Jan 27, 2013 at 11:26 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> On Sun, Jan 27, 2013 at 11:09 PM, Georg-Johann Lay wrote:
>>>>> The patch was originally worked out by Bernd Schmidt and fixed a problem
>>>>> introduced in
>>>>>
>>>>> http://gcc.gnu.org/r190252
>
> Ironically, this revision fixes a reload problem on x86/x86_64 --
> which doesn't use reload anymore now...
>
>
>> Does this mean the fix is rejected for 4.8?
>
> No, just that it probably helps to add a RM to the CC list.
>
> FWIW, it seems to me that this patch should go into 4.8, because the
> bug is probably not limited to AVR.

Indeed, the fix also looks quite obvious though I know nothing about the
code at all.

Thus, ok from a RM perspective if a reload-affine person approves it.

Thanks,
Richard.

> Ciao!
> Steven
Jeff Law - Jan. 28, 2013, 5:35 p.m.
On 01/27/2013 03:26 PM, Steven Bosscher wrote:
> On Sun, Jan 27, 2013 at 11:09 PM, Georg-Johann Lay wrote:
>>>>> The patch was originally worked out by Bernd Schmidt and fixed a problem
>>>>> introduced in
>>>>>
>>>>> http://gcc.gnu.org/r190252
>
> Ironically, this revision fixes a reload problem on x86/x86_64 --
> which doesn't use reload anymore now...
>
>
>> Does this mean the fix is rejected for 4.8?
>
> No, just that it probably helps to add a RM to the CC list.
>
> FWIW, it seems to me that this patch should go into 4.8, because the
> bug is probably not limited to AVR.
At this stage, I tend to be more conservative.  However, it looks like 
Ulrich & Richi have taken a looksie and think the patch is fine.  I'm 
certainly not going to object.

jeff
Jeff Law - Jan. 28, 2013, 5:38 p.m.
On 01/27/2013 03:09 PM, Georg-Johann Lay wrote:
>>
>>> If not, it'll probably need release manager approval before it can go
>>> in.
>>>
>>> Please attach your patch to PR54814 and attach PR 54814 to the 4.9
>>> pending patches meta bug.
>
> Does this mean the fix is rejected for 4.8?
Not necessarily.  We're in a regression bugfix only stage; so 
regressions can obviously be fixed.  If a change does not fix a 
regression, then it really needs the release manager's approval to go 
forward at this stage.

Jeff

Patch

Index: gcc/reload.c
===================================================================
--- gcc/reload.c	(revision 194553)
+++ gcc/reload.c	(working copy)
@@ -709,7 +709,7 @@  find_valid_class (enum machine_mode oute
 }
 
 /* We are trying to reload a subreg of something that is not a register.
-   Find the largest class which has at least one register valid in
+   Find the largest class which contains only registers valid in
    mode MODE.  OUTER is the mode of the subreg, DEST_CLASS the class in
    which we would eventually like to obtain the object.  */
 
@@ -729,10 +729,12 @@  find_valid_class_1 (enum machine_mode ou
     {
       int bad = 0;
       for (regno = 0; regno < FIRST_PSEUDO_REGISTER && !bad; regno++)
-	if (TEST_HARD_REG_BIT (reg_class_contents[rclass], regno)
-	    && !HARD_REGNO_MODE_OK (regno, mode))
-	  bad = 1;
-
+	{
+	  if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno)
+	      && !HARD_REGNO_MODE_OK (regno, mode))
+	    bad = 1;
+	}
+      
       if (bad)
 	continue;