[Bug rtl-optimization/44618] New: Arguments are not passed correctly to out-of-line restore functions. [4.4][4.5][4.6]

[Bug rtl-optimization/44618] New: Arguments are not passed correctly to out-of-line restore functions. [4.4][4.5][4.6]

[edmar at freescale dot com]

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)


-- 
           Summary: Arguments are not passed correctly to out-of-line
                    restore functions. [4.4][4.5][4.6]
           Product: gcc
           Version: 4.6.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: rtl-optimization
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: edmar at freescale dot com
  GCC host triplet: powerpc-unkonw-linux-gnu
GCC target triplet: powerpc-unkonw-linux-gnu


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44618

[Bug rtl-optimization/44618] Arguments are not passed correctly to out-of-line restore functions. [4.4][4.5][4.6]

[edmar at freescale dot com]

------- Comment #1 from edmar at freescale dot com  2010-06-21 20:14 -------
Created an attachment (id=20967)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=20967&action=view)
patch for 4.5 and 4.6


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44618

[Bug rtl-optimization/44618] Arguments are not passed correctly to out-of-line restore functions. [4.4][4.5][4.6]

[edmar at freescale dot com]

------- Comment #2 from edmar at freescale dot com  2010-06-21 20:15 -------
Created an attachment (id=20968)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=20968&action=view)
patch for 4.4


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44618

[Bug rtl-optimization/44618] Arguments are not passed correctly to out-of-line restore functions. [4.4][4.5][4.6]

[edmar at freescale dot com]

------- Comment #3 from edmar at freescale dot com  2010-06-21 20:17 -------
Created an attachment (id=20969)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=20969&action=view)
ChangeLog for propsed patch


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44618

[Bug rtl-optimization/44618] Arguments are not passed correctly to out-of-line restore functions. [4.4][4.5][4.6]

[edmar at freescale dot com]

------- Comment #4 from edmar at freescale dot com  2010-06-21 20:17 -------
Created an attachment (id=20970)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=20970&action=view)
ChangeLog for proposed test case


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44618

[Bug rtl-optimization/44618] Arguments are not passed correctly to out-of-line restore functions. [4.4][4.5][4.6]

[edmar at freescale dot com]

------- Comment #5 from edmar at freescale dot com  2010-06-21 20:24 -------
(In reply to comment #0)

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

Thanks
Edmar


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44618

[Bug target/44618] Arguments are not passed correctly to out-of-line restore functions.

[pinskia at gcc dot gnu dot org]

------- Comment #6 from pinskia at gcc dot gnu dot org  2010-06-21 20:34 -------
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.


-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
          Component|rtl-optimization            |target
     Ever Confirmed|0                           |1
   GCC host triplet|powerpc-unkonw-linux-gnu    |
 GCC target triplet|powerpc-unkonw-linux-gnu    |powerpc-linux-gnu
           Keywords|                            |wrong-code
   Last reconfirmed|0000-00-00 00:00:00         |2010-06-21 20:34:41
               date|                            |
            Summary|Arguments are not passed    |Arguments are not passed
                   |correctly to out-of-line    |correctly to out-of-line
                   |restore functions.          |restore functions.
                   |[4.4][4.5][4.6]             |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44618

[Bug target/44618] Arguments are not passed correctly to out-of-line restore functions.

[edmar at freescale dot com]

------- Comment #7 from edmar at freescale dot com  2010-06-21 21:18 -------
(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 ...


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44618

[Bug target/44618] Arguments are not passed correctly to out-of-line restore functions.

[pinskia at gcc dot gnu dot org]

------- Comment #8 from pinskia at gcc dot gnu dot org  2010-06-21 21:29 -------
(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


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44618

[Bug target/44618] Arguments are not passed correctly to out-of-line restore functions.

[edmar at freescale dot com]

------- Comment #9 from edmar at freescale dot com  2010-06-21 23:36 -------
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


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44618

[Bug target/44618] Arguments are not passed correctly to out-of-line restore functions.

[pinskia at gcc dot gnu dot org]

------- Comment #10 from pinskia at gcc dot gnu dot org  2010-06-21 23:54 -------
(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).


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44618

[Bug target/44618] Arguments are not passed correctly to out-of-line restore functions.

[edmar at freescale dot com]

------- Comment #11 from edmar at freescale dot com  2010-06-22 16:53 -------
(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


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44618

[Bug target/44618] Arguments are not passed correctly to out-of-line restore functions.

[edmar at freescale dot com]

------- Comment #12 from edmar at freescale dot com  2010-06-22 20:51 -------
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")])


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44618

[Bug target/44618] Arguments are not passed correctly to out-of-line restore functions.

[jakub at gcc dot gnu dot org]

------- Comment #13 from jakub at gcc dot gnu dot org  2010-06-22 20:56 -------
It looks much better than adding new single register constraints to me.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44618

[Bug target/44618] Arguments are not passed correctly to out-of-line restore functions.

[edmar at freescale dot com]

------- Comment #14 from edmar at freescale dot com  2010-06-28 15:15 -------
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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44618

[Bug target/44618] Arguments are not passed correctly to out-of-line restore functions.

[edmar at freescale dot com]

------- Comment #15 from edmar at freescale dot com  2010-06-28 15:17 -------
Created an attachment (id=21026)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=21026&action=view)
Alternative patch that affects powerpc only


-- 

edmar at freescale dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #20968|0                           |1
        is obsolete|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44618

[Bug target/44618] Arguments are not passed correctly to out-of-line restore functions.

[edmar at freescale dot com]

------- Comment #16 from edmar at freescale dot com  2010-06-28 15:18 -------
Created an attachment (id=21027)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=21027&action=view)
Alternative patch for 4.5 and trunk


-- 

edmar at freescale dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #20967|0                           |1
        is obsolete|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44618

[Bug target/44618] Arguments are not passed correctly to out-of-line restore functions.

[edmar at freescale dot com]

------- Comment #17 from edmar at freescale dot com  2010-06-28 15:19 -------
Created an attachment (id=21028)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=21028&action=view)
Changelog for alternative patches


-- 

edmar at freescale dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #20969|0                           |1
        is obsolete|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44618

[Bug target/44618] Arguments are not passed correctly to out-of-line restore functions.

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44618

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |sebastian.huber@embedded-br
                   |                            |ains.de

--- Comment #18 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2011-04-27 09:18:29 UTC ---
*** Bug 48604 has been marked as a duplicate of this bug. ***