Bug 44618 - [4.5 regression] wrong code with -frename-registers
Summary: [4.5 regression] wrong code with -frename-registers
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.6.0
: P2 normal
Target Milestone: 4.6.1
Assignee: Not yet assigned to anyone
URL:
Keywords: patch, wrong-code
: 48604 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-06-21 20:13 UTC by Edmar Wienskoski
Modified: 2012-07-02 09:39 UTC (History)
9 users (show)

See Also:
Host:
Target: powerpc-linux-gnu
Build:
Known to work: 4.6.1, 4.7.0
Known to fail:
Last reconfirmed: 2010-06-21 20:34:41


Attachments
patch for 4.5 and 4.6 (806 bytes, patch)
2010-06-21 20:14 UTC, Edmar Wienskoski
Details | Diff
patch for 4.4 (814 bytes, patch)
2010-06-21 20:15 UTC, Edmar Wienskoski
Details | Diff
ChangeLog for propsed patch (129 bytes, text/plain)
2010-06-21 20:17 UTC, Edmar Wienskoski
Details
ChangeLog for proposed test case (106 bytes, text/plain)
2010-06-21 20:17 UTC, Edmar Wienskoski
Details
Alternative patch that affects powerpc only (1.29 KB, patch)
2010-06-28 15:17 UTC, Edmar Wienskoski
Details | Diff
Alternative patch for 4.5 and trunk (1.30 KB, patch)
2010-06-28 15:18 UTC, Edmar Wienskoski
Details | Diff
Changelog for alternative patches (241 bytes, text/plain)
2010-06-28 15:19 UTC, Edmar Wienskoski
Details
This patch was tested against 4.5/4.6/4.7 (1.84 KB, patch)
2011-05-23 21:58 UTC, Edmar Wienskoski
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Edmar Wienskoski 2010-06-21 20:13:45 UTC
On 32 bit powerpc targets, the family of out-of-line restore functions
(_restgpr_<xx>_x), expects r11 (or something else, depending on the
ABI used) to have the new value of the stack pointer.

Gcc emits rtl that sets r11, and a jump_insn that flags the use of r11.

When compiling the test case with "-Os -frename-registers", the register
rename pass (regrename.c) may rename this r11 def-use chain.

test case:
int
calc (int j)
{
  if (j==0) return 0;
  return calc(j-1)*(j+1);
}

compiled with:
-Os -frename-registers -S

results in assembler like this:
.
.
.
.L3:
        addi 4,1,16
        b _restgpr_31_x

This problem exists in the 4.4 branch, the 4.5 branch, and in the main
trunk as well.

In general, the optimization avoids renaming registers involved in
argument passing, but only when a call_insn is involved. The attached
patches extends this behavior to jump_insn as well.

Is my approach to fix the problem adequate ?

In case it is, attached are a patch that can be applied to gcc-4.5 and
the trunk, and a specific one for the 4.4 branch. I have bootstraped
and regression tested each of the following:
(All of them completed with no regressions)
Note: I have no WAA privilege.


4.4 branch rev 160848 on target powerpc-unkown-linux-gnu (--with-cpu=603e, G5 hardware)
4.4 branch rev 160838 on target powerpc64-unkown-linux-gnu (--with-cpu=970, G5 hardware)
4.4 branch rev 160837 on target i686-pc-linux (--with-cpu=generic, --with-arch=i686, Xeon hardware)
4.4 branch rev 160837 on target x86_64-pc-linux (--with-cpu=generic, Xeon hardware)

4.5 branch rev 160890 on target powerpc-unkown-linux-gnu (--with-cpu=603e, G5 hardware)
4.5 branch rev 160869 on target powerpc64-unkown-linux-gnu (--with-cpu=970, G5 hardware)
4.5 branch rev 160869 on target i686-pc-linux (--with-cpu=generic, --with-arch=i686, Xeon hardware)
4.5 branch rev 160857 on target x86_64-pc-linux (--with-cpu=generic, Xeon hardware)

4.6 (trunk) rev 160955 on target powerpc-unkown-linux-gnu (--with-cpu=603e, G5 hardware)
4.6 (trunk) rev 161009 on target powerpc64-unkown-linux-gnu (--with-cpu=970, G5 hardware)
4.6 (trunk) rev 160906 on target i686-pc-linux (--with-cpu=generic, --with-arch=i686, Xeon hardware)
4.6 (trunk) rev 160919 on target x86_64-pc-linux (--with-cpu=generic, Xeon hardware)
Comment 1 Edmar Wienskoski 2010-06-21 20:14:45 UTC
Created attachment 20967 [details]
patch for 4.5 and 4.6
Comment 2 Edmar Wienskoski 2010-06-21 20:15:20 UTC
Created attachment 20968 [details]
patch for 4.4
Comment 3 Edmar Wienskoski 2010-06-21 20:17:10 UTC
Created attachment 20969 [details]
ChangeLog for propsed patch
Comment 4 Edmar Wienskoski 2010-06-21 20:17:46 UTC
Created attachment 20970 [details]
ChangeLog for proposed test case
Comment 5 Edmar Wienskoski 2010-06-21 20:24:37 UTC
(In reply to comment #0)

Sorry for the spelling, please read "unknown" through out the report.

Thanks
Edmar


Comment 6 Andrew Pinski 2010-06-21 20:34:41 UTC
I think this is the wrong fix ....  I think the problem is in the patterns not using a hard register or a constraint that says only those registers can be used.

Confirmed.
Comment 7 Edmar Wienskoski 2010-06-21 21:18:03 UTC
(In reply to comment #6)
> I think this is the wrong fix ....  I think the problem is in the patterns not
> using a hard register or a constraint that says only those registers can be
> used.
> 
> Confirmed.
> 

I mostly agree with you. But in this case, it is already a hard register (rtl generated in epilogue).

If the goal is to fix the bug changing only the powerpc end. I must use some constraint already coded in regrename.c. But, IMHO, I did not see anything appropriated. So I resorted to the solution I posted ...
Comment 8 Andrew Pinski 2010-06-21 21:29:49 UTC
(In reply to comment #7) 
> I mostly agree with you. But in this case, it is already a hard register (rtl
> generated in epilogue).

No the pattern accepts any registers which means register rename can rename the register to what ever registers it feels like.  What register rename constraints has is to do with stuff that are implicit (like hard registers for inline-asm and function call arguments).  This case we have an explicit operand which means we need to mark the constraint to be correct.

For an example:
(define_insn "*restore_gpregs_<mode>"
 [(match_parallel 0 "any_parallel_operand"
                  [(clobber (match_operand:P 1 "register_operand" "=l"))
                   (use (match_operand:P 2 "symbol_ref_operand" "s"))
                   (use (match_operand:P 3 "gpc_reg_operand" "r"))
                   (set (match_operand:P 4 "gpc_reg_operand" "=r")
                        (match_operand:P 5 "memory_operand" "m"))])]
 ""
 "bl %2"
 [(set_attr "type" "branch")
  (set_attr "length" "4")])

Should be changed to:
(define_insn "*restore_gpregs_<mode>"
 [(match_parallel 0 "any_parallel_operand"
                  [(clobber (match_operand:P 1 "register_operand" "=l"))
                   (use (match_operand:P 2 "symbol_ref_operand" "s"))
                   (use (match_operand:P 3 "gpc_reg_operand" "d"))
                   (set (match_operand:P 4 "gpc_reg_operand" "=r")
                        (match_operand:P 5 "memory_operand" "m"))])]
 ""
 "bl %2"
 [(set_attr "type" "branch")
  (set_attr "length" "4")])

(etc.)
Where d is a new constraint.  Though you might need more than one constraint and patterns because of the following code:
  RTVEC_ELT (p, offset++)
    = gen_rtx_USE (VOIDmode,
                   gen_rtx_REG (Pmode, DEFAULT_ABI != ABI_AIX ? 11
                                       : gpr && !lr ? 12
                                       : 1));

--- CUT ---
I think you might cause other targets to slow down because of the use of parallels inside jump instructions (mostly indirect calls).


Thanks,
Andrew Pinski
Comment 9 Edmar Wienskoski 2010-06-21 23:36:16 UTC
Hummm, I will work on your input, But now I have more questions:

1) Why do you call this case as explicit, and function call arguments implicit ? The way I see it, this is a special function call (implemented with a jump_insn to save space). So, why r11 is not a function call argument ?

2) On other targets, and indirect calls, gcc generates a parallel but still uses a call_insn to represent it. Which causes build_def_use() to avoid register renaming of these arguments.
So other targets would not be slowed down, because those cases have to be avoided.

3) On the other hand, can you give me an example of a jump_insn, with a parallel, and a symbol reference, where a register rename would be valid ? Wouldn't all those registers be considered function call arguments ?
(Perhaps I should add a test for the existence of a symbol reference in my patch. If the symbol reference is external or global, registers can never be renamed !)

Thanks,
Edmar

Comment 10 Andrew Pinski 2010-06-21 23:54:06 UTC
(In reply to comment #9)
> Hummm, I will work on your input, But now I have more questions:
> 
> 1) Why do you call this case as explicit, and function call arguments implicit
> ? The way I see it, this is a special function call (implemented with a
> jump_insn to save space). So, why r11 is not a function call argument ?

Because the insn has a register reference to r11/r1/r12 :) that is the (use (match_operand: )) part of the rtx.  This is unlike call instructions which don't have match_operands for function arguments.  That is what I mean explicit vs implicit.  


> 
> 2) On other targets, and indirect calls, gcc generates a parallel but still
> uses a call_insn to represent it. Which causes build_def_use() to avoid
> register renaming of these arguments.
> So other targets would not be slowed down, because those cases have to be
> avoided.

I mixed up insn rtl codes, woops.  I thought calls was always done using jump_insn.  Anyways I am saying you are hard coding a target specific information inside a target generic part of the code.  This is why I think it is the wrong approach.

> 
> 3) On the other hand, can you give me an example of a jump_insn, with a
> parallel, and a symbol reference, where a register rename would be valid ?
> Wouldn't all those registers be considered function call arguments ?
> (Perhaps I should add a test for the existence of a symbol reference in my
> patch. If the symbol reference is external or global, registers can never be
> renamed !)

From i386/i386.md:

(define_insn "return_indirect_internal"
  [(return)
   (use (match_operand:SI 0 "register_operand" "r"))]
  "reload_completed"
  "jmp\t%A0"
  [(set_attr "type" "ibr")
   (set_attr "length_immediate" "0")])

Though it is harder to invoke that but still it can happen if I read the code correctly (the pop needs to be greater than 64k).
Comment 11 Edmar Wienskoski 2010-06-22 16:53:32 UTC
(In reply to comment #10)
> Because the insn has a register reference to r11/r1/r12 :) that is the (use
> (match_operand: )) part of the rtx.  This is unlike call instructions which
> don't have match_operands for function arguments.  That is what I mean explicit
> vs implicit.  

Ok.

> I mixed up insn rtl codes, woops.  I thought calls was always done using
> jump_insn.  Anyways I am saying you are hard coding a target specific
> information inside a target generic part of the code.  This is why I think it
> is the wrong approach.

I understand your point of view. But I am still convinced that a jump_insn with a parallel and symbol reference (external or global) cannot have the explicit registers renamed, regardless of the target architecture.

Now, if powerpc is the only target that can generate such a rtl (jump_insn, with external symbol reference), then I have to agree with you...

> (define_insn "return_indirect_internal"
>   [(return)
>    (use (match_operand:SI 0 "register_operand" "r"))]
>   "reload_completed"
>   "jmp\t%A0"
>   [(set_attr "type" "ibr")
>    (set_attr "length_immediate" "0")])
> 
> Though it is harder to invoke that but still it can happen if I read the code
> correctly (the pop needs to be greater than 64k).

This was close, but it has no symbol reference. Therefore it is ok to rename the register, because the use of the register is in the "jmp" instruction and not
inside the code that the "jmp" points to. (i.e., this insn is not passing an argument to a function).

Ok, I have to change my patch, but before I do that, I want to make sure we agree on the details. Is it safe to assume that powerpc is the only target that can generate such a rtl ?

If not, I will modify the patch to add a test that checks if there is a symbol reference.

If it is, I will scratch all this, and will make the powerpc target generate a call_insn instead of a jump_insn (At this point we agree that reg rename cannot swallow a jump_insn, right ?)

Thanks,
Edmar


Comment 12 Edmar Wienskoski 2010-06-22 20:51:44 UTC
Ok. Following your lines from comment 8, (You suggested to create 3 new constraint, "d", which would accepts only one register each (11, 12, 1), right ?).

The following is more explicit, and would avoid to allocate 3 more letters for constraints.

Thanks,
Edmar



If a register is not referenced through a match_operand, build_def_use will not consider that register for renaming.

Using this fact, I can replace:

(define_insn "*return_and_restore_gpregs_<mode>"
 [(match_parallel 0 "any_parallel_operand"
                  [(return)
                   (clobber (match_operand:P 1 "register_operand" "=l"))
                   (use (match_operand:P 2 "symbol_ref_operand" "s"))
                   (use (match_operand:P 3 "gpc_reg_operand" "r"))
                   (set (match_operand:P 4 "gpc_reg_operand" "=r")
                        (match_operand:P 5 "memory_operand" "m"))])]
 ""
 "b %2"
 [(set_attr "type" "branch")
  (set_attr "length" "4")])

-----------------------------------------
with this:

(define_insn "*return_and_restore_gpregs1_<mode>"
 [(match_parallel 0 "any_parallel_operand"
                  [(return)
                   (clobber (match_operand:P 1 "register_operand" "=l"))
                   (use (match_operand:P 2 "symbol_ref_operand" "s"))
                   (use (reg:P 11))
                   (set (match_operand:P 3 "gpc_reg_operand" "=r")
                        (match_operand:P 4 "memory_operand" "m"))])]
 "DEFAULT_ABI != ABI_AIX"
 "b %2"
 [(set_attr "type" "branch")
  (set_attr "length" "4")])

(define_insn "*return_and_restore_gpregs2_<mode>"
 [(match_parallel 0 "any_parallel_operand"
                  [(return)
                   (clobber (match_operand:P 1 "register_operand" "=l"))
                   (use (match_operand:P 2 "symbol_ref_operand" "s"))
                   (use (reg:P 12))
                   (set (match_operand:P 3 "gpc_reg_operand" "=r")
                        (match_operand:P 4 "memory_operand" "m"))])]
 "DEFAULT_ABI == ABI_AIX"
 "b %2"
 [(set_attr "type" "branch")
  (set_attr "length" "4")])

(define_insn "*return_and_restore_gpregs3_<mode>"
 [(match_parallel 0 "any_parallel_operand"
                  [(return)
                   (clobber (match_operand:P 1 "register_operand" "=l"))
                   (use (match_operand:P 2 "symbol_ref_operand" "s"))
                   (use (reg:P 1))
                   (set (match_operand:P 3 "gpc_reg_operand" "=r")
                        (match_operand:P 4 "memory_operand" "m"))])]
 "DEFAULT_ABI == ABI_AIX"
 "b %2"
 [(set_attr "type" "branch")
  (set_attr "length" "4")])
Comment 13 Jakub Jelinek 2010-06-22 20:56:08 UTC
It looks much better than adding new single register constraints to me.
Comment 14 Edmar Wienskoski 2010-06-28 15:15:50 UTC
I am attaching new patches. One for gcc-4.4 and the other for gcc-4.5 and gcc-4.6.

All three branches were bootstrapped and regression tested for both 32 bits powerpc (603e) and 64 bit powerpc (970) with no regressions.

Comment 15 Edmar Wienskoski 2010-06-28 15:17:49 UTC
Created attachment 21026 [details]
Alternative patch that affects powerpc only
Comment 16 Edmar Wienskoski 2010-06-28 15:18:24 UTC
Created attachment 21027 [details]
Alternative patch for 4.5 and trunk
Comment 17 Edmar Wienskoski 2010-06-28 15:19:13 UTC
Created attachment 21028 [details]
Changelog for alternative patches
Comment 18 Eric Botcazou 2011-04-27 09:18:29 UTC
*** Bug 48604 has been marked as a duplicate of this bug. ***
Comment 19 Eric Botcazou 2011-04-27 09:35:44 UTC
Yes, the problem is in the PowerPC back-end.  And I also think that the use trick is the way to go.  The mildly annoying thing is the need to duplicate patterns just because of the different register number, but I'm not sure this can be avoided.  In any case, this is up to the PowerPC maintainer at this point.
Comment 20 Jakub Jelinek 2011-05-19 12:42:02 UTC
Edmar, have you posted your alternative patch to gcc-patches?
That's where patch review is done, not in bugzilla.
Comment 21 Edmar Wienskoski 2011-05-19 15:52:02 UTC
(In reply to comment #20)
> Edmar, have you posted your alternative patch to gcc-patches?
> That's where patch review is done, not in bugzilla.

I don't remember, I am posting it today just in case I missed.

Thanks
Edmar
Comment 22 Edmar Wienskoski 2011-05-23 21:57:08 UTC
I completed re-testing everything.
It turns out I cannot reproduce the original error on gcc-4.4 (rev 173968)
So, I am submitting only the patch that I tested for gcc-4.5/4.6/4.7

Regression tested for e500mc target on:
4.5: Revision: 173928
4.6: Revision: 173936
trunk: Revision: 173966

The patch gcc.fix_rnreg4 applies directly to 4.6, 4.7 (1 line offset),
and 4.5 (-632 lines offset)

Thanks,
Edmar
Comment 23 Edmar Wienskoski 2011-05-23 21:58:28 UTC
Created attachment 24337 [details]
This patch was tested against 4.5/4.6/4.7
Comment 24 Edmar Wienskoski 2011-06-02 15:33:16 UTC
The patch was approved by David, but I don't have WAA.
Can I get anyone in this list to volunteer to do the commit ?

Thanks,
Edmar


2011-05-23  Edmar Wienskoski  edmar@freescale.com

        * gcc.target/powerpc/outofline_rnreg.c: New testcase.


2011-05-23  Edmar Wienskoski  edmar@freescale.com

        * rs6000.md (save_gpregs_<mode>): Replaced pattern with a set
        of similar patterns, where the MATCH_OPERAND for the function
        argument is replaced with individual references to hardware
        registers.
        * rs6000.md (save_fpregs_<mode>): Ditto
        * rs6000.md (restore_gpregs_<mode>): Ditto
        * rs6000.md (return_and_restore_gpregs_<mode>): Ditto
        * rs6000.md (return_and_restore_fpregs_<mode>): Ditto
        * rs6000.md (return_and_restore_fpregs_aix_<mode>): Ditto
Comment 25 Richard Biener 2011-06-12 12:45:40 UTC
Please ask somebody to commit it as followup to the approval mail.
Comment 26 Jakub Jelinek 2011-06-15 16:44:12 UTC
Committed to the trunk so far apparently:
http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=174997
Comment 27 Jakub Jelinek 2011-06-16 07:50:00 UTC
Author: jakub
Date: Thu Jun 16 07:49:58 2011
New Revision: 175093

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=175093
Log:
2011-06-13  Edmar Wienskoski  <edmar@freescale.com>

	PR target/44618
	* config/rs6000/rs6000.md (save_gpregs_<mode>): Replaced pattern
	with a set of similar patterns, where the MATCH_OPERAND for the
	function argument is replaced with individual references to hardware
	registers.
	(save_fpregs_<mode>): Ditto
	(restore_gpregs_<mode>): Ditto
	(return_and_restore_gpregs_<mode>): Ditto
	(return_and_restore_fpregs_<mode>): Ditto
	(return_and_restore_fpregs_aix_<mode>): Ditto

	* gcc.target/powerpc/outofline_rnreg.c: New testcase.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
Comment 28 Jakub Jelinek 2011-06-16 07:52:47 UTC
Author: jakub
Date: Thu Jun 16 07:52:44 2011
New Revision: 175094

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=175094
Log:
	Backported from mainline
	2011-06-13  Edmar Wienskoski  <edmar@freescale.com>

	PR target/44618
	* config/rs6000/rs6000.md (save_gpregs_<mode>): Replaced pattern
	with a set of similar patterns, where the MATCH_OPERAND for the
	function argument is replaced with individual references to hardware
	registers.
	(save_fpregs_<mode>): Ditto
	(restore_gpregs_<mode>): Ditto
	(return_and_restore_gpregs_<mode>): Ditto
	(return_and_restore_fpregs_<mode>): Ditto
	(return_and_restore_fpregs_aix_<mode>): Ditto

	* gcc.target/powerpc/outofline_rnreg.c: New testcase.

Added:
    branches/gcc-4_6-branch/gcc/testsuite/gcc.target/powerpc/outofline_rnreg.c
Modified:
    branches/gcc-4_6-branch/gcc/ChangeLog
    branches/gcc-4_6-branch/gcc/config/rs6000/rs6000.md
    branches/gcc-4_6-branch/gcc/testsuite/ChangeLog
Comment 29 Kumar Gala 2011-08-24 04:57:31 UTC
(In reply to comment #27)
> Author: jakub
> Date: Thu Jun 16 07:49:58 2011
> New Revision: 175093
> 
> URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=175093
> Log:
> 2011-06-13  Edmar Wienskoski  <edmar@freescale.com>
> 
>     PR target/44618
>     * config/rs6000/rs6000.md (save_gpregs_<mode>): Replaced pattern
>     with a set of similar patterns, where the MATCH_OPERAND for the
>     function argument is replaced with individual references to hardware
>     registers.
>     (save_fpregs_<mode>): Ditto
>     (restore_gpregs_<mode>): Ditto
>     (return_and_restore_gpregs_<mode>): Ditto
>     (return_and_restore_fpregs_<mode>): Ditto
>     (return_and_restore_fpregs_aix_<mode>): Ditto
> 
>     * gcc.target/powerpc/outofline_rnreg.c: New testcase.
> 
> Modified:
>     trunk/gcc/ChangeLog
>     trunk/gcc/testsuite/ChangeLog

Any reason this wasn't applied to 4.5 branch?
Comment 30 Edmar Wienskoski 2011-08-24 15:06:48 UTC
Not really.

A year ago when I opened the bug, it was affecting all branches.
When it was finally approved (David E.), I re-factored the patch.

At this point I found 4.4 did not need the patch any more. I submitted
patch for all others (4.5, 4.6, 4.7). As I sad before, it was already 
approved,
but I depend on David to do the commit as I don't have WAA.

He said he was busy, it took almost 2 weeks for him to do the commit on 4.7,
few days latter it came the commit on 4.6. And the 4.5 never came...

Edmar



On 08/23/2011 11:57 PM, galak at kernel dot crashing.org wrote:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44618
>
> Kumar Gala<galak at kernel dot crashing.org>  changed:
>
>             What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                   CC|                            |galak at kernel dot
>                     |                            |crashing.org
>
> --- Comment #29 from Kumar Gala<galak at kernel dot crashing.org>  2011-08-24 04:57:31 UTC ---
> (In reply to comment #27)
>> Author: jakub
>> Date: Thu Jun 16 07:49:58 2011
>> New Revision: 175093
>>
>> URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=175093
>> Log:
>> 2011-06-13  Edmar Wienskoski<edmar@freescale.com>
>>
>>      PR target/44618
>>      * config/rs6000/rs6000.md (save_gpregs_<mode>): Replaced pattern
>>      with a set of similar patterns, where the MATCH_OPERAND for the
>>      function argument is replaced with individual references to hardware
>>      registers.
>>      (save_fpregs_<mode>): Ditto
>>      (restore_gpregs_<mode>): Ditto
>>      (return_and_restore_gpregs_<mode>): Ditto
>>      (return_and_restore_fpregs_<mode>): Ditto
>>      (return_and_restore_fpregs_aix_<mode>): Ditto
>>
>>      * gcc.target/powerpc/outofline_rnreg.c: New testcase.
>>
>> Modified:
>>      trunk/gcc/ChangeLog
>>      trunk/gcc/testsuite/ChangeLog
> Any reason this wasn't applied to 4.5 branch?
>
Comment 31 Bernhard Kaindl 2012-02-17 11:03:13 UTC
(In reply to comment #30)
> A year ago when I opened the bug, it was affecting all branches.
> When it was finally approved (David E.), I re-factored the patch.
> 
> At this point I found 4.4 did not need the patch any more.

I cannot confirm this as of 4.4.6. gcc-4.4.6 still produces the bug:

echo 'int calc(int j){if (j==0)return 0;return calc(j-1)*(j+1);}' >rename.c
powerpc-aeos5-linux-gcc rename.c -Os -frename-registers -S -v 2>&1|sed s,/tag/mt/gcc.gnu.org/4.4.6-plf2.1-pre1/rhe5/x86/toolchain/ppc-aeos5,/SYSROOT,g && grep -B1 restgpr rename.s
Using built-in specs.
Target: powerpc-aeos5-linux
Configured with: /home/mt/gcc.gnu.org/4.4.6-plf2.1-pre1/build/rhe5/x86/src/os/oe/trunk/446r01/work/i686-ppce300c3-sdk-aeos5-linux/gcc-cross-sdk-4.4.6-r0.1/gcc-4.4.6/configure --build=i686-linux --host=i686-linux --target=powerpc-aeos5-linux --prefix=/SYSROOT --exec_prefix=/SYSROOT --bindir=/SYSROOT/bin --sbindir=/SYSROOT/bin --libexecdir=/SYSROOT/libexec --datadir=/SYSROOT/share --sysconfdir=/SYSROOT/etc --sharedstatedir=/SYSROOT/share/com --localstatedir=/SYSROOT/var --libdir=/SYSROOT/lib --includedir=/SYSROOT/include --oldincludedir=/SYSROOT/include --infodir=/SYSROOT/share/info --mandir=/SYSROOT/share/man --disable-nls --enable-clocale=generic --with-gnu-ld --without-cloog --enable-shared --enable-languages=c,c++ --enable-threads=posix --disable-multilib --enable-c99 --enable-long-long --enable-symvers=gnu --enable-libstdcxx-pch --program-prefix=powerpc-aeos5-linux- --enable-libssp --disable-bootstrap --disable-libgomp --disable-libmudflap --without-long-double-128 --with-sysroot=/SYSROOT/powerpc-aeos5-linux --with-build-time-tools=/home/mt/gcc.gnu.org/4.4.6-plf2.1-pre1/build/rhe5/x86/src/os/oe/trunk/446r01/cross/ppce300c3/powerpc-aeos5-linux/bin --with-build-sysroot=/home/mt/gcc.gnu.org/4.4.6-plf2.1-pre1/build/rhe5/x86/src/os/oe/trunk/446r01/staging/ppce300c3-aeos5-linux --with-mpfr=/home/mt/gcc.gnu.org/4.4.6-plf2.1-pre1/build/rhe5/x86/src/os/oe/trunk/446r01/staging/i686-linux/usr --disable-libgomp --enable-libmudflap --without-long-double-128 --enable-__cxa_atexit
Thread model: posix
gcc version 4.4.6 (GCC) 
COLLECT_GCC_OPTIONS='-Os' '-frename-registers' '-S' '-v'
 /nfsshares/tas/SYSROOT/bin/../libexec/gcc/powerpc-aeos5-linux/4.4.6/cc1 -quiet -v -iprefix /nfsshares/tas/SYSROOT/bin/../lib/gcc/powerpc-aeos5-linux/4.4.6/ -isysroot /nfsshares/tas/SYSROOT/bin/../powerpc-aeos5-linux -D__unix__ -D__gnu_linux__ -D__linux__ -Dunix -D__unix -Dlinux -D__linux -Asystem=linux -Asystem=unix -Asystem=posix rename.c -quiet -dumpbase rename.c -auxbase rename -Os -version -frename-registers -o rename.s
ignoring nonexistent directory "/nfsshares/tas/SYSROOT/bin/../powerpc-aeos5-linux/usr/local/include"
ignoring duplicate directory "/nfsshares/tas/SYSROOT/bin/../lib/gcc/../../lib/gcc/powerpc-aeos5-linux/4.4.6/include"
ignoring duplicate directory "/nfsshares/tas/SYSROOT/bin/../lib/gcc/../../lib/gcc/powerpc-aeos5-linux/4.4.6/include-fixed"
ignoring duplicate directory "/nfsshares/tas/SYSROOT/bin/../lib/gcc/../../lib/gcc/powerpc-aeos5-linux/4.4.6/../../../../powerpc-aeos5-linux/include"
#include "..." search starts here:
#include <...> search starts here:
 /nfsshares/tas/SYSROOT/bin/../lib/gcc/powerpc-aeos5-linux/4.4.6/include
 /nfsshares/tas/SYSROOT/bin/../lib/gcc/powerpc-aeos5-linux/4.4.6/include-fixed
 /nfsshares/tas/SYSROOT/bin/../lib/gcc/powerpc-aeos5-linux/4.4.6/../../../../powerpc-aeos5-linux/include
 /nfsshares/tas/SYSROOT/bin/../powerpc-aeos5-linux/usr/include
End of search list.
GNU C (GCC) version 4.4.6 (powerpc-aeos5-linux)
        compiled by GNU C version 4.1.2 20080704 (Red Hat 4.1.2-44), GMP version 4.2.4, MPFR version 2.3.2.
GGC heuristics: --param ggc-min-expand=100 --param ggc-min-heapsize=131072
Compiler executable checksum: 08c68272536463fe09fe596df17cf497
COMPILER_PATH=/nfsshares/tas/SYSROOT/bin/../libexec/gcc/powerpc-aeos5-linux/4.4.6/:/nfsshares/tas/SYSROOT/bin/../libexec/gcc/:/nfsshares/tas/SYSROOT/bin/../lib/gcc/powerpc-aeos5-linux/4.4.6/../../../../powerpc-aeos5-linux/bin/
LIBRARY_PATH=/nfsshares/tas/SYSROOT/bin/../lib/gcc/powerpc-aeos5-linux/4.4.6/:/nfsshares/tas/SYSROOT/bin/../lib/gcc/:/nfsshares/tas/SYSROOT/bin/../lib/gcc/powerpc-aeos5-linux/4.4.6/../../../../powerpc-aeos5-linux/lib/:/nfsshares/tas/SYSROOT/bin/../powerpc-aeos5-linux/lib/:/nfsshares/tas/SYSROOT/bin/../powerpc-aeos5-linux/usr/lib/
COLLECT_GCC_OPTIONS='-Os' '-frename-registers' '-S' '-v'
        addi 4,1,16
        b _restgpr_31_x

> I submitted patch for all others (4.5, 4.6, 4.7). As I sad before,
> it was already  approved,
> but I depend on David to do the commit as I don't have WAA.
> 
> He said he was busy, it took almost 2 weeks for him to do the commit on 4.7,
> few days latter it came the commit on 4.6. And the 4.5 never came...

As far as I can tell from the above test, applying the fix to the 4.4 branch with target milestone 4.4.7 is still in order.

(besides committing the fix to 4.5 as well)

David, still busy?
Comment 32 Jakub Jelinek 2012-03-13 12:47:08 UTC
4.4 branch is being closed, moving to 4.5.4 target.
Comment 33 Richard Biener 2012-07-02 09:39:48 UTC
Fixed in 4.6.1.