fuse-caller-save - hook format

fuse-caller-save - hook format

[Tom de Vries]

Vladimir,

All patches for the fuse-caller-save optimization have been ok-ed. The only part
not approved is the MIPS-specific part.

The objection of Richard S. is not so much the patch itself, but more the idea
of the hook fn_other_hard_reg_usage.

For clarity, I'm restating the current hook definition here:
...
+@deftypefn {Target Hook} bool TARGET_FN_OTHER_HARD_REG_USAGE (struct
hard_reg_set_container *@var{regs})
Add any hard registers to @var{regs} that are set or clobbered by a call to the
function.  This hook only needs to add registers that cannot be found by
examination of the final RTL representation of a function.  This hook returns
true if it managed to determine which registers need to be added.  The default
version of this hook returns false.
...

Richard prefers to, rather than having a hook specifying what registers are
implicitly clobbered, adding those clobbers to CALL_INSN_FUNCTION_USAGE.

I can see these possibilities (and perhaps there are more):

1. We go with Richards proposal: we make each target responsible for adding
these clobbers in CALL_INSN_FUNCTION_USAGE, and use a hook called f.i.
targetm.fuse_caller_save_p or targetm.implicit_call_clobbers_in_fusage_p, to
indicate whether a target has taken care of that, meaning it's safe to do the
fuse-caller-save optimization.

2. A mixed solution: we make each target responsible for specifying which
clobbers need to be added in CALL_INSN_FUNCTION_USAGE, using a hook called f.i.
targetm.call_clobbered_regs, and add generic code to add those clobbers to
CALL_INSN_FUNCTION_USAGE.

3. We stick with the current, approved hook format, and try to convince Richard
to live with it.


Since you are a register allocator maintainer, familiar with the
fuse-caller-save optimization, and have approved the original hook, I would like
to ask you to make a decision on how to proceed from here.

Thanks,
- Tom

Re: fuse-caller-save - hook format

[Richard Sandiford]

Tom de Vries <Tom_deVries@mentor.com> writes:
> Vladimir,
>
> All patches for the fuse-caller-save optimization have been ok-ed. The only part
> not approved is the MIPS-specific part.
>
> The objection of Richard S. is not so much the patch itself, but more the idea
> of the hook fn_other_hard_reg_usage.
>
> For clarity, I'm restating the current hook definition here:
> ...
> +@deftypefn {Target Hook} bool TARGET_FN_OTHER_HARD_REG_USAGE (struct
> hard_reg_set_container *@var{regs})
> Add any hard registers to @var{regs} that are set or clobbered by a call to the
> function.  This hook only needs to add registers that cannot be found by
> examination of the final RTL representation of a function.  This hook returns
> true if it managed to determine which registers need to be added.  The default
> version of this hook returns false.
> ...

Just for the record, I think this hook was defined as applying during final
to a potential callee function, rather than applying to a particular call.
I.e., after calculating which registers a function uses, the code would
add the registers returned by this hook to the set.

My objection to that was that the set of registers clobbered while making
a call depends on the caller.

With that proviso...

> Richard prefers to, rather than having a hook specifying what registers are
> implicitly clobbered, adding those clobbers to CALL_INSN_FUNCTION_USAGE.

...I agree this is a fair summary.

> I can see these possibilities (and perhaps there are more):
>
> 1. We go with Richards proposal: we make each target responsible for adding
> these clobbers in CALL_INSN_FUNCTION_USAGE, and use a hook called f.i.
> targetm.fuse_caller_save_p or targetm.implicit_call_clobbers_in_fusage_p, to
> indicate whether a target has taken care of that, meaning it's safe to do the
> fuse-caller-save optimization.
>
> 2. A mixed solution: we make each target responsible for specifying which
> clobbers need to be added in CALL_INSN_FUNCTION_USAGE, using a hook called f.i.
> targetm.call_clobbered_regs, and add generic code to add those clobbers to
> CALL_INSN_FUNCTION_USAGE.
>
> 3. We stick with the current, approved hook format, and try to convince Richard
> to live with it.

The reason I don't like (2) is that, on targets like MIPS where the
different call cases are quite complicated, the implementation of
the hook would need to follow the same logic as the call expander
to figure out which case applies.  It just seems more elegant to me
to add the clobbers when emitting the call.

IMO CALL_INSN_FUNCTION_USAGE is like a "varargs" part of the call pattern.
In other words it's a way of allowing the set of uses and clobbers to
vary from call to call without having to define lots of different call
define_insns.  If you look at it like that, adding the clobbers when
emitting the insn seems more correct as well.

Thanks,
Richard

Re: fuse-caller-save - hook format

[Jeff Law]

On 04/16/14 13:41, Richard Sandiford wrote:
>
> IMO CALL_INSN_FUNCTION_USAGE is like a "varargs" part of the call pattern.
> In other words it's a way of allowing the set of uses and clobbers to
> vary from call to call without having to define lots of different call
> define_insns.  If you look at it like that, adding the clobbers when
> emitting the insn seems more correct as well.
This seems like a better direction to me as well.  There's just 
something clean and elegant about attaching this stuff to the CALL_INSN.

jeff

Re: fuse-caller-save - hook format

[Vladimir Makarov]

On 2014-04-16, 3:19 PM, Tom de Vries wrote:
> Vladimir,
> 
> All patches for the fuse-caller-save optimization have been ok-ed. The only part
> not approved is the MIPS-specific part.
> 
> The objection of Richard S. is not so much the patch itself, but more the idea
> of the hook fn_other_hard_reg_usage.
> 
> For clarity, I'm restating the current hook definition here:
> ...
> +@deftypefn {Target Hook} bool TARGET_FN_OTHER_HARD_REG_USAGE (struct
> hard_reg_set_container *@var{regs})
> Add any hard registers to @var{regs} that are set or clobbered by a call to the
> function.  This hook only needs to add registers that cannot be found by
> examination of the final RTL representation of a function.  This hook returns
> true if it managed to determine which registers need to be added.  The default
> version of this hook returns false.
> ...
> 
> Richard prefers to, rather than having a hook specifying what registers are
> implicitly clobbered, adding those clobbers to CALL_INSN_FUNCTION_USAGE.
> 
> I can see these possibilities (and perhaps there are more):
> 
> 1. We go with Richards proposal: we make each target responsible for adding
> these clobbers in CALL_INSN_FUNCTION_USAGE, and use a hook called f.i.
> targetm.fuse_caller_save_p or targetm.implicit_call_clobbers_in_fusage_p, to
> indicate whether a target has taken care of that, meaning it's safe to do the
> fuse-caller-save optimization.
> 
> 2. A mixed solution: we make each target responsible for specifying which
> clobbers need to be added in CALL_INSN_FUNCTION_USAGE, using a hook called f.i.
> targetm.call_clobbered_regs, and add generic code to add those clobbers to
> CALL_INSN_FUNCTION_USAGE.
> 
> 3. We stick with the current, approved hook format, and try to convince Richard
> to live with it.
> 
> 
> Since you are a register allocator maintainer, familiar with the
> fuse-caller-save optimization, and have approved the original hook, I would like
> to ask you to make a decision on how to proceed from here.
> 

I have no preferences and it is a matter of taste.  Each solution has
own advantages and disadvantages.  Putting this info into
CALL_INSN_FUNCTION_USAGE helps GCC developing a lot but it has a big
drawback in RTL memory footprint (especially for some targets which have
a lot of regs like AM29k or IA64).  On the order hand analogous approach
is already used in DF-infrastructure (which would be nice to fix it imho).

Still between GCC users and GCC developers, I'd prefer solution (even
the effect on amount of resources used by GCC is quite insignificant)
for users as their number is in a few magnitudes more then the developers.

But I can live with any solution.  So it is up to you.  I am flexible.

Re: fuse-caller-save - hook format

[Richard Sandiford]

Vladimir Makarov <vmakarov@redhat.com> writes:
> On 2014-04-16, 3:19 PM, Tom de Vries wrote:
>> Vladimir,
>> 
>> All patches for the fuse-caller-save optimization have been ok-ed. The
>> only part
>> not approved is the MIPS-specific part.
>> 
>> The objection of Richard S. is not so much the patch itself, but more the idea
>> of the hook fn_other_hard_reg_usage.
>> 
>> For clarity, I'm restating the current hook definition here:
>> ...
>> +@deftypefn {Target Hook} bool TARGET_FN_OTHER_HARD_REG_USAGE (struct
>> hard_reg_set_container *@var{regs})
>> Add any hard registers to @var{regs} that are set or clobbered by a
>> call to the
>> function.  This hook only needs to add registers that cannot be found by
>> examination of the final RTL representation of a function.  This hook returns
>> true if it managed to determine which registers need to be added.  The default
>> version of this hook returns false.
>> ...
>> 
>> Richard prefers to, rather than having a hook specifying what registers are
>> implicitly clobbered, adding those clobbers to CALL_INSN_FUNCTION_USAGE.
>> 
>> I can see these possibilities (and perhaps there are more):
>> 
>> 1. We go with Richards proposal: we make each target responsible for adding
>> these clobbers in CALL_INSN_FUNCTION_USAGE, and use a hook called f.i.
>> targetm.fuse_caller_save_p or targetm.implicit_call_clobbers_in_fusage_p, to
>> indicate whether a target has taken care of that, meaning it's safe to do the
>> fuse-caller-save optimization.
>> 
>> 2. A mixed solution: we make each target responsible for specifying which
>> clobbers need to be added in CALL_INSN_FUNCTION_USAGE, using a hook
>> called f.i.
>> targetm.call_clobbered_regs, and add generic code to add those clobbers to
>> CALL_INSN_FUNCTION_USAGE.
>> 
>> 3. We stick with the current, approved hook format, and try to
>> convince Richard
>> to live with it.
>> 
>> 
>> Since you are a register allocator maintainer, familiar with the
>> fuse-caller-save optimization, and have approved the original hook, I
>> would like
>> to ask you to make a decision on how to proceed from here.
>> 
>
> I have no preferences and it is a matter of taste.  Each solution has
> own advantages and disadvantages.  Putting this info into
> CALL_INSN_FUNCTION_USAGE helps GCC developing a lot but it has a big
> drawback in RTL memory footprint (especially for some targets which have
> a lot of regs like AM29k or IA64).  On the order hand analogous approach
> is already used in DF-infrastructure (which would be nice to fix it imho).
>
> Still between GCC users and GCC developers, I'd prefer solution (even
> the effect on amount of resources used by GCC is quite insignificant)
> for users as their number is in a few magnitudes more then the developers.

Hmm, but you're talking like there are going to be a lot of these registers.
This isn't about which registers are call-clobbered or call-saved according
to the ABI (that's already available in other places).  All we want here
are the set of registers that are clobbered _in the caller_ before reaching
the callee or after the callee has returned.

So although IA-64 has lots of registers, the caller doesn't AFAIK use
lots of registers in the process of making the call.

On all targets we should be talking about one or two registers here.

Thanks,
Richard

Re: fuse-caller-save - hook format

[Vladimir Makarov]

On 2014-04-17, 11:29 AM, Richard Sandiford wrote:
> Vladimir Makarov <vmakarov@redhat.com> writes:
>> On 2014-04-16, 3:19 PM, Tom de Vries wrote:
>>> Vladimir,
>>>
>>> All patches for the fuse-caller-save optimization have been ok-ed. The
>>> only part
>>> not approved is the MIPS-specific part.
>>>
>>> The objection of Richard S. is not so much the patch itself, but more the idea
>>> of the hook fn_other_hard_reg_usage.
>>>
>>> For clarity, I'm restating the current hook definition here:
>>> ...
>>> +@deftypefn {Target Hook} bool TARGET_FN_OTHER_HARD_REG_USAGE (struct
>>> hard_reg_set_container *@var{regs})
>>> Add any hard registers to @var{regs} that are set or clobbered by a
>>> call to the
>>> function.  This hook only needs to add registers that cannot be found by
>>> examination of the final RTL representation of a function.  This hook returns
>>> true if it managed to determine which registers need to be added.  The default
>>> version of this hook returns false.
>>> ...
>>>
>>> Richard prefers to, rather than having a hook specifying what registers are
>>> implicitly clobbered, adding those clobbers to CALL_INSN_FUNCTION_USAGE.
>>>
>>> I can see these possibilities (and perhaps there are more):
>>>
>>> 1. We go with Richards proposal: we make each target responsible for adding
>>> these clobbers in CALL_INSN_FUNCTION_USAGE, and use a hook called f.i.
>>> targetm.fuse_caller_save_p or targetm.implicit_call_clobbers_in_fusage_p, to
>>> indicate whether a target has taken care of that, meaning it's safe to do the
>>> fuse-caller-save optimization.
>>>
>>> 2. A mixed solution: we make each target responsible for specifying which
>>> clobbers need to be added in CALL_INSN_FUNCTION_USAGE, using a hook
>>> called f.i.
>>> targetm.call_clobbered_regs, and add generic code to add those clobbers to
>>> CALL_INSN_FUNCTION_USAGE.
>>>
>>> 3. We stick with the current, approved hook format, and try to
>>> convince Richard
>>> to live with it.
>>>
>>>
>>> Since you are a register allocator maintainer, familiar with the
>>> fuse-caller-save optimization, and have approved the original hook, I
>>> would like
>>> to ask you to make a decision on how to proceed from here.
>>>
>>
>> I have no preferences and it is a matter of taste.  Each solution has
>> own advantages and disadvantages.  Putting this info into
>> CALL_INSN_FUNCTION_USAGE helps GCC developing a lot but it has a big
>> drawback in RTL memory footprint (especially for some targets which have
>> a lot of regs like AM29k or IA64).  On the order hand analogous approach
>> is already used in DF-infrastructure (which would be nice to fix it imho).
>>
>> Still between GCC users and GCC developers, I'd prefer solution (even
>> the effect on amount of resources used by GCC is quite insignificant)
>> for users as their number is in a few magnitudes more then the developers.
>
> Hmm, but you're talking like there are going to be a lot of these registers.

Yes, you are right.  That is what I thought.  I should have read Tom's 
email with more attention.

> This isn't about which registers are call-clobbered or call-saved according
> to the ABI (that's already available in other places).  All we want here
> are the set of registers that are clobbered _in the caller_ before reaching
> the callee or after the callee has returned.
>
> So although IA-64 has lots of registers, the caller doesn't AFAIK use
> lots of registers in the process of making the call.
>
> On all targets we should be talking about one or two registers here.
>

I see.  I guess your proposed solution is ok then.

Re: fuse-caller-save - hook format

[Tom de Vries]

On 17-04-14 18:49, Vladimir Makarov wrote:
> I see.  I guess your proposed solution is ok then.

Vladimir,
Richard,

I've updated the fuse-caller-save patch series to model non-callee call clobbers 
in CALL_INSN_FUNCTION_USAGE.

There are 2 new hooks:

1. call_fusage_contains_non_callee_clobbers.
A hook to indicate whether a target has added the non-callee call clobbers to 
CALL_INSN_FUNCTION_USAGE, meaning it's safe to do the fuse-caller-save optimization.

2. post_expand_call_insn.
A utility hook to facilitate adding the clobbers to CALL_INSN_FUNCTION_USAGE.

I've bootstrapped and reg-tested on x86_64, and I've build and reg-tested on MIPS.

The series now looks like:

      1	 -fuse-caller-save - Add command line option
      2	 -fuse-caller-save - Add new reg-note REG_CALL_DECL
      3	 Add implicit parameter to find_all_hard_reg_sets
      4	 Register CALL_INSN_FUNCTION_USAGE in find_all_hard_reg_sets
      5	 Add call_fusage_contains_non_callee_clobbers hook
      6	 -fuse-caller-save - Collect register usage information
      7	 -fuse-caller-save - Use collected register usage information
      8	 -fuse-caller-save - Enable by default at O2 and higher
      9	 -fuse-caller-save - Add documentation
     10	 -fuse-caller-save - Add test-case
     11	 Add post_expand_call_insn hook
     12	 Add clobber_reg
     13	 -fuse-caller-save - Enable for MIPS
     14	 -fuse-caller-save - Enable for ARM
     15	 -fuse-caller-save - Enable for AArch64
     16	 -fuse-caller-save - Support in lra
     17	 -fuse-caller-save - Enable for i386

The submission/approval status is:
1-3, 7-10, 16: approved
4: submitted, pinged Eric Botcazou 16-04-2014
5, 11: new hook, need to submit
6, 14-15: approved earlier, but need to resubmit due to updated hook
12: new utility patch, need to submit
13: need to resubmit due to updated hook
17: need to submit

I'll post the patches that need (re)submitting.

Thanks,
- Tom

Re: fuse-caller-save - hook format

[Richard Sandiford]

Tom de Vries <Tom_deVries@mentor.com> writes:
> 2. post_expand_call_insn.
> A utility hook to facilitate adding the clobbers to CALL_INSN_FUNCTION_USAGE.

Why is this needed though?  Like I say, I think targets should update
CALL_INSN_FUNCTION_USAGE when emitting calls as part of the call expander.
Splitting the functionality of the call expanders across the define_expand
and a new hook just makes things unnecessarily complicated IMO.

Thanks,
Richard

Re: fuse-caller-save - hook format

[Tom de Vries]

On 22-04-14 17:27, Richard Sandiford wrote:
> Tom de Vries <Tom_deVries@mentor.com> writes:
>> 2. post_expand_call_insn.
>> A utility hook to facilitate adding the clobbers to CALL_INSN_FUNCTION_USAGE.
>
> Why is this needed though?  Like I say, I think targets should update
> CALL_INSN_FUNCTION_USAGE when emitting calls as part of the call expander.
> Splitting the functionality of the call expanders across the define_expand
> and a new hook just makes things unnecessarily complicated IMO.
>

Richard,

It is not needed, but it is convenient.

There are targets where the define_expands for calls use the rtl template. 
Having to add clobbers to the CALL_INSN_FUNCTION_USAGE for such a target means 
you cannot use the rtl template any more and instead need to generate all needed 
RTL insns in C code.

This hook means that you can keep using the rtl template, which is less 
intrusive for those targets.

Thanks,
- Tom

> Thanks,
> Richard
>

Re: fuse-caller-save - hook format

[Richard Sandiford]

Tom de Vries <Tom_deVries@mentor.com> writes:

> On 22-04-14 17:27, Richard Sandiford wrote:
>> Tom de Vries <Tom_deVries@mentor.com> writes:
>>> 2. post_expand_call_insn.
>>> A utility hook to facilitate adding the clobbers to CALL_INSN_FUNCTION_USAGE.
>>
>> Why is this needed though?  Like I say, I think targets should update
>> CALL_INSN_FUNCTION_USAGE when emitting calls as part of the call expander.
>> Splitting the functionality of the call expanders across the define_expand
>> and a new hook just makes things unnecessarily complicated IMO.
>>
>
> Richard,
>
> It is not needed, but it is convenient.
>
> There are targets where the define_expands for calls use the rtl template. 
> Having to add clobbers to the CALL_INSN_FUNCTION_USAGE for such a target means 
> you cannot use the rtl template any more and instead need to generate
> all needed
> RTL insns in C code.
>
> This hook means that you can keep using the rtl template, which is less 
> intrusive for those targets.

But if the target is simple enough to use a single call pattern for call
cases, wouldn't it be possible to add the clobber directly to the call
pattern?  Which target do you have in mind?

Thanks,
Richard

Add call_fusage_contains_non_callee_clobbers hook

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 789 bytes --]

On 22-04-14 17:05, Tom de Vries wrote:
> I've updated the fuse-caller-save patch series to model non-callee call clobbers
> in CALL_INSN_FUNCTION_USAGE.
>

Vladimir,

This patch adds a hook to indicate whether a target has added the non-callee 
call clobbers to CALL_INSN_FUNCTION_USAGE, meaning it's safe to do the 
fuse-caller-save optimization.

OK for trunk?

Thanks,
- Tom

2013-04-29  Radovan Obradovic  <robradovic@mips.com>
             Tom de Vries  <tom@codesourcery.com>

	* target.def (call_fusage_contains_non_callee_clobbers): New DEFHOOK.
	* doc/tm.texi.in (@node Stack and Calling): Add Miscellaneous Register
	Hooks to @menu.
	(@node Miscellaneous Register Hooks): New node.
	(@hook TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS): New hook.
	* doc/tm.texi: Regenerate.


[-- Attachment #2: 0005-Add-call_fusage_contains_non_callee_clobbers-hook.patch --]
[-- Type: text/x-patch, Size: 3714 bytes --]

diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index b8ca17e..8af8efd 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -3091,6 +3091,7 @@ This describes the stack layout and calling conventions.
 * Profiling::
 * Tail Calls::
 * Stack Smashing Protection::
+* Miscellaneous Register Hooks::
 @end menu
 
 @node Frame Layout
@@ -5016,6 +5017,21 @@ normally defined in @file{libgcc2.c}.
 Whether this target supports splitting the stack when the options described in @var{opts} have been passed.  This is called after options have been parsed, so the target may reject splitting the stack in some configurations.  The default version of this hook returns false.  If @var{report} is true, this function may issue a warning or error; if @var{report} is false, it must simply return a value
 @end deftypefn
 
+@node Miscellaneous Register Hooks
+@subsection Miscellaneous register hooks
+@cindex miscellaneous register hooks
+
+@deftypefn {Target Hook} bool TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS (void)
+Return true if all the calls in the current function contain clobbers in
+CALL_INSN_FUNCTION_USAGE for the registers that are clobbered by the call
+rather than by the callee, and are not already set or clobbered in the call
+pattern.  Examples of such registers are registers used in PLTs and stubs,
+and temporary registers used in the call instruction but not present in the
+rtl pattern.  Another way to formulate it is the registers not present in the
+rtl pattern that are clobbered by the call assuming the callee does not
+clobber any register.  The default version of this hook returns false.
+@end deftypefn
+
 @node Varargs
 @section Implementing the Varargs Macros
 @cindex varargs implementation
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index d793d26..8991c3c 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -2720,6 +2720,7 @@ This describes the stack layout and calling conventions.
 * Profiling::
 * Tail Calls::
 * Stack Smashing Protection::
+* Miscellaneous Register Hooks::
 @end menu
 
 @node Frame Layout
@@ -3985,6 +3986,12 @@ the function prologue.  Normally, the profiling code comes after.
 
 @hook TARGET_SUPPORTS_SPLIT_STACK
 
+@node Miscellaneous Register Hooks
+@subsection Miscellaneous register hooks
+@cindex miscellaneous register hooks
+
+@hook TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS
+
 @node Varargs
 @section Implementing the Varargs Macros
 @cindex varargs implementation
diff --git a/gcc/target.def b/gcc/target.def
index 3a64cd1..ae0bc9c 100644
--- a/gcc/target.def
+++ b/gcc/target.def
@@ -5130,6 +5130,22 @@ FRAME_POINTER_REGNUM, ARG_POINTER_REGNUM, and the PIC_OFFSET_TABLE_REGNUM.",
  void, (bitmap regs),
  hook_void_bitmap)
 
+/* Targets should define this target hook to mark that non-callee clobbers are
+   present in CALL_INSN_FUNCTION_USAGE for all the calls in the current
+   function.  */
+DEFHOOK
+(call_fusage_contains_non_callee_clobbers,
+ "Return true if all the calls in the current function contain clobbers in\n\
+CALL_INSN_FUNCTION_USAGE for the registers that are clobbered by the call\n\
+rather than by the callee, and are not already set or clobbered in the call\n\
+pattern.  Examples of such registers are registers used in PLTs and stubs,\n\
+and temporary registers used in the call instruction but not present in the\n\
+rtl pattern.  Another way to formulate it is the registers not present in the\n\
+rtl pattern that are clobbered by the call assuming the callee does not\n\
+clobber any register.  The default version of this hook returns false.",
+ bool, (void),
+ hook_bool_void_false)
+
 /* Fill in additional registers set up by prologue into a regset.  */
 DEFHOOK
 (set_up_by_prologue,

Re: fuse-caller-save - hook format

[Tom de Vries]

On 22-04-14 18:18, Richard Sandiford wrote:
> Tom de Vries <Tom_deVries@mentor.com> writes:
>
>> On 22-04-14 17:27, Richard Sandiford wrote:
>>> Tom de Vries <Tom_deVries@mentor.com> writes:
>>>> 2. post_expand_call_insn.
>>>> A utility hook to facilitate adding the clobbers to CALL_INSN_FUNCTION_USAGE.
>>>
>>> Why is this needed though?  Like I say, I think targets should update
>>> CALL_INSN_FUNCTION_USAGE when emitting calls as part of the call expander.
>>> Splitting the functionality of the call expanders across the define_expand
>>> and a new hook just makes things unnecessarily complicated IMO.
>>>
>>
>> Richard,
>>
>> It is not needed, but it is convenient.
>>
>> There are targets where the define_expands for calls use the rtl template.
>> Having to add clobbers to the CALL_INSN_FUNCTION_USAGE for such a target means
>> you cannot use the rtl template any more and instead need to generate
>> all needed
>> RTL insns in C code.
>>
>> This hook means that you can keep using the rtl template, which is less
>> intrusive for those targets.
>

[ switching order of questions ]
> Which target do you have in mind?

Aarch64.

 > But if the target is simple enough to use a single call pattern for call
 > cases, wouldn't it be possible to add the clobber directly to the call
 > pattern?

I think that can be done, but that feels intrusive as well. I thought the reason 
that we added these clobbers to CALL_INSN_FUNCTION_USAGE was exactly because we 
did not want to add them to the rtl patterns?

But, if the maintainer is fine with that, so am I.

Richard Earnshaw,

are you ok with adding the IP0_REGNUM/IP1_REGNUM clobbers to all the call 
patterns in the Aarch64 target?

The alternatives are:
- rewrite the call expansions not to use the rtl templates, and add the clobbers
   there to CALL_INSN_FUNCTION_USAGE
- get the post_expand_call_insn hook approved and use that to add the clobbers
   to CALL_INSN_FUNCTION_USAGE.

what is your preference?

Thanks,
- Tom

Re: fuse-caller-save - hook format

[Richard Earnshaw]

On 22/04/14 18:13, Tom de Vries wrote:
> On 22-04-14 18:18, Richard Sandiford wrote:
>> Tom de Vries <Tom_deVries@mentor.com> writes:
>>
>>> On 22-04-14 17:27, Richard Sandiford wrote:
>>>> Tom de Vries <Tom_deVries@mentor.com> writes:
>>>>> 2. post_expand_call_insn.
>>>>> A utility hook to facilitate adding the clobbers to CALL_INSN_FUNCTION_USAGE.
>>>>
>>>> Why is this needed though?  Like I say, I think targets should update
>>>> CALL_INSN_FUNCTION_USAGE when emitting calls as part of the call expander.
>>>> Splitting the functionality of the call expanders across the define_expand
>>>> and a new hook just makes things unnecessarily complicated IMO.
>>>>
>>>
>>> Richard,
>>>
>>> It is not needed, but it is convenient.
>>>
>>> There are targets where the define_expands for calls use the rtl template.
>>> Having to add clobbers to the CALL_INSN_FUNCTION_USAGE for such a target means
>>> you cannot use the rtl template any more and instead need to generate
>>> all needed
>>> RTL insns in C code.
>>>
>>> This hook means that you can keep using the rtl template, which is less
>>> intrusive for those targets.
>>
> 
> [ switching order of questions ]
>> Which target do you have in mind?
> 
> Aarch64.
> 
>  > But if the target is simple enough to use a single call pattern for call
>  > cases, wouldn't it be possible to add the clobber directly to the call
>  > pattern?
> 
> I think that can be done, but that feels intrusive as well. I thought the reason 
> that we added these clobbers to CALL_INSN_FUNCTION_USAGE was exactly because we 
> did not want to add them to the rtl patterns?
> 
> But, if the maintainer is fine with that, so am I.
> 
> Richard Earnshaw,
> 
> are you ok with adding the IP0_REGNUM/IP1_REGNUM clobbers to all the call 
> patterns in the Aarch64 target?
> 
> The alternatives are:
> - rewrite the call expansions not to use the rtl templates, and add the clobbers
>    there to CALL_INSN_FUNCTION_USAGE
> - get the post_expand_call_insn hook approved and use that to add the clobbers
>    to CALL_INSN_FUNCTION_USAGE.
> 
> what is your preference?
> 

It seems undesirable to me to be hard-coding ABI constraints directly
into the MD file.  It's not a major problem while there is one ABI
that's common to all targets; but it's quite possible this sort of
detail would change from platform to platform.  That sort of churn is
best kept out of the MD file itself, if at all possible.

R.

> Thanks,
> - Tom
> 
> 
> 

-fuse-caller-save - Collect register usage information

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 991 bytes --]

On 22-04-14 17:05, Tom de Vries wrote:
> I've updated the fuse-caller-save patch series to model non-callee call clobbers
> in CALL_INSN_FUNCTION_USAGE.

Vladimir,

This is the updated version of the previously approved patch 
http://gcc.gnu.org/ml/gcc-patches/2013-03/msg01320.html , updated for the new 
hook call_fusage_contains_non_callee_clobbers.

The only difference is in the functions get_call_reg_set_usage and 
collect_fn_hard_reg_usage which use the hook.

OK for trunk?

Thanks,
- Tom

2013-04-29  Radovan Obradovic  <robradovic@mips.com>
             Tom de Vries  <tom@codesourcery.com>

	* cgraph.h (struct cgraph_node): Add function_used_regs,
	function_used_regs_initialized and function_used_regs_valid fields.
	* final.c: Move include of hard-reg-set.h to before rtl.h to declare
	find_all_hard_reg_sets.
	(collect_fn_hard_reg_usage, get_call_fndecl, get_call_cgraph_node)
	(get_call_reg_set_usage): New function.
	(rest_of_handle_final): Use collect_fn_hard_reg_usage.


[-- Attachment #2: 0006-fuse-caller-save-Collect-register-usage-information.patch --]
[-- Type: text/x-patch, Size: 5309 bytes --]

diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index 15310d8..eb0fe8e 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -408,6 +408,15 @@ public:
   /* Time profiler: first run of function.  */
   int tp_first_run;
 
+  /* Call unsaved hard registers really used by the corresponding
+     function (including ones used by functions called by the
+     function).  */
+  HARD_REG_SET function_used_regs;
+  /* Set if function_used_regs is initialized.  */
+  unsigned function_used_regs_initialized: 1;
+  /* Set if function_used_regs is valid.  */
+  unsigned function_used_regs_valid: 1;
+
   /* Set when decl is an abstract function pointed to by the
      ABSTRACT_DECL_ORIGIN of a reachable function.  */
   unsigned used_as_abstract_origin : 1;
diff --git a/gcc/final.c b/gcc/final.c
index 83abee2..0b1947d 100644
--- a/gcc/final.c
+++ b/gcc/final.c
@@ -49,6 +49,7 @@ along with GCC; see the file COPYING3.  If not see
 
 #include "tree.h"
 #include "varasm.h"
+#include "hard-reg-set.h"
 #include "rtl.h"
 #include "tm_p.h"
 #include "regs.h"
@@ -57,7 +58,6 @@ along with GCC; see the file COPYING3.  If not see
 #include "recog.h"
 #include "conditions.h"
 #include "flags.h"
-#include "hard-reg-set.h"
 #include "output.h"
 #include "except.h"
 #include "function.h"
@@ -223,6 +223,7 @@ static int alter_cond (rtx);
 static int final_addr_vec_align (rtx);
 #endif
 static int align_fuzz (rtx, rtx, int, unsigned);
+static void collect_fn_hard_reg_usage (void);
 \f
 /* Initialize data in final at the beginning of a compilation.  */
 
@@ -4425,6 +4426,7 @@ rest_of_handle_final (void)
   assemble_start_function (current_function_decl, fnname);
   final_start_function (get_insns (), asm_out_file, optimize);
   final (get_insns (), asm_out_file, optimize);
+  collect_fn_hard_reg_usage ();
   final_end_function ();
 
   /* The IA-64 ".handlerdata" directive must be issued before the ".endp"
@@ -4720,3 +4722,119 @@ make_pass_clean_state (gcc::context *ctxt)
 {
   return new pass_clean_state (ctxt);
 }
+
+/* Collect hard register usage for the current function.  */
+
+static void
+collect_fn_hard_reg_usage (void)
+{
+  rtx insn;
+  int i;
+  struct cgraph_node *node;
+
+  if (!flag_use_caller_save)
+    return;
+
+  node = cgraph_get_node (current_function_decl);
+  gcc_assert (node != NULL);
+
+  gcc_assert (!node->function_used_regs_initialized);
+  node->function_used_regs_initialized = 1;
+
+  for (insn = get_insns (); insn != NULL_RTX; insn = next_insn (insn))
+    {
+      HARD_REG_SET insn_used_regs;
+
+      if (!NONDEBUG_INSN_P (insn))
+	continue;
+
+      find_all_hard_reg_sets (insn, &insn_used_regs, false);
+
+      if (CALL_P (insn)
+	  && (!targetm.call_fusage_contains_non_callee_clobbers ()
+	      || !get_call_reg_set_usage (insn, &insn_used_regs, call_used_reg_set)))
+	{
+	  CLEAR_HARD_REG_SET (node->function_used_regs);
+	  return;
+	}
+
+      IOR_HARD_REG_SET (node->function_used_regs, insn_used_regs);
+    }
+
+  /* Be conservative - mark fixed and global registers as used.  */
+  IOR_HARD_REG_SET (node->function_used_regs, fixed_reg_set);
+  for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
+    if (global_regs[i])
+      SET_HARD_REG_BIT (node->function_used_regs, i);
+
+#ifdef STACK_REGS
+  /* Handle STACK_REGS conservatively, since the df-framework does not
+     provide accurate information for them.  */
+
+  for (i = FIRST_STACK_REG; i <= LAST_STACK_REG; i++)
+    SET_HARD_REG_BIT (node->function_used_regs, i);
+#endif
+
+  node->function_used_regs_valid = 1;
+}
+
+/* Get the declaration of the function called by INSN.  */
+
+static tree
+get_call_fndecl (rtx insn)
+{
+  rtx note, datum;
+
+  if (!flag_use_caller_save)
+    return NULL_TREE;
+
+  note = find_reg_note (insn, REG_CALL_DECL, NULL_RTX);
+  if (note == NULL_RTX)
+    return NULL_TREE;
+
+  datum = XEXP (note, 0);
+  if (datum != NULL_RTX)
+    return SYMBOL_REF_DECL (datum);
+
+  return NULL_TREE;
+}
+
+static struct cgraph_node *
+get_call_cgraph_node (rtx insn)
+{
+  tree fndecl;
+
+  if (insn == NULL_RTX)
+    return NULL;
+
+  fndecl = get_call_fndecl (insn);
+  if (fndecl == NULL_TREE
+      || !targetm.binds_local_p (fndecl))
+    return NULL;
+
+  return cgraph_get_node (fndecl);
+}
+
+/* Find hard registers used by function call instruction INSN, and return them
+   in REG_SET.  Return DEFAULT_SET in REG_SET if not found.  */
+
+bool
+get_call_reg_set_usage (rtx insn, HARD_REG_SET *reg_set,
+			HARD_REG_SET default_set)
+{
+  if (flag_use_caller_save
+      && targetm.call_fusage_contains_non_callee_clobbers ())
+    {
+      struct cgraph_node *node = get_call_cgraph_node (insn);
+      if (node != NULL
+	  && node->function_used_regs_valid)
+	{
+	  COPY_HARD_REG_SET (*reg_set, node->function_used_regs);
+	  AND_HARD_REG_SET (*reg_set, default_set);
+	  return true;
+	}
+    }
+
+  COPY_HARD_REG_SET (*reg_set, default_set);
+  return false;
+}
diff --git a/gcc/regs.h b/gcc/regs.h
index 006caca..44cc005 100644
--- a/gcc/regs.h
+++ b/gcc/regs.h
@@ -421,4 +421,8 @@ range_in_hard_reg_set_p (const HARD_REG_SET set, unsigned regno, int nregs)
   return true;
 }
 
+/* Get registers used by given function call instruction.  */
+extern bool get_call_reg_set_usage (rtx insn, HARD_REG_SET *reg_set,
+				    HARD_REG_SET default_set);
+
 #endif /* GCC_REGS_H */

Add clobber_reg

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 812 bytes --]

On 22-04-14 17:05, Tom de Vries wrote:
> I've updated the fuse-caller-save patch series to model non-callee call clobbers
> in CALL_INSN_FUNCTION_USAGE.

Eric,

Richard Sandiford mentioned here ( 
http://gcc.gnu.org/ml/gcc-patches/2014-04/msg00870.html ):
...
Although we really should have a utility function like use_reg, but for
clobbers, so that the above would become:

   clobber_reg (&CALL_INSN_FUNCTION_USAGE (insn), gen_rtx_REG (word_mode, 18));
...


I've implemented a patch that adds clobber_reg and clobber_reg_mode, similar to 
use_reg and use_reg_mode.

Bootstrapped and reg-tested on x86_64 as part of the fuse-caller-save series.

OK for trunk?

Thanks,
- Tom

2014-04-18  Tom de Vries  <tom@codesourcery.com>

	* expr.c (clobber_reg_mode): New function.
	* expr.h (clobber_reg): New function.


[-- Attachment #2: 0012-Add-clobber_reg.patch --]
[-- Type: text/x-patch, Size: 1736 bytes --]

diff --git a/gcc/expr.c b/gcc/expr.c
index 72e4401..fc58eb7f 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -2396,6 +2396,18 @@ use_reg_mode (rtx *call_fusage, rtx reg, enum machine_mode mode)
     = gen_rtx_EXPR_LIST (mode, gen_rtx_USE (VOIDmode, reg), *call_fusage);
 }
 
+/* Add a CLOBBER expression for REG to the (possibly empty) list pointed
+   to by CALL_FUSAGE.  REG must denote a hard register.  */
+
+void
+clobber_reg_mode (rtx *call_fusage, rtx reg, enum machine_mode mode)
+{
+  gcc_assert (REG_P (reg) && REGNO (reg) < FIRST_PSEUDO_REGISTER);
+
+  *call_fusage
+    = gen_rtx_EXPR_LIST (mode, gen_rtx_CLOBBER (VOIDmode, reg), *call_fusage);
+}
+
 /* Add USE expressions to *CALL_FUSAGE for each of NREGS consecutive regs,
    starting at REGNO.  All of these registers must be hard registers.  */
 
diff --git a/gcc/expr.h b/gcc/expr.h
index 524da67..1823feb 100644
--- a/gcc/expr.h
+++ b/gcc/expr.h
@@ -346,6 +346,7 @@ extern void copy_blkmode_from_reg (rtx, rtx, tree);
 /* Mark REG as holding a parameter for the next CALL_INSN.
    Mode is TYPE_MODE of the non-promoted parameter, or VOIDmode.  */
 extern void use_reg_mode (rtx *, rtx, enum machine_mode);
+extern void clobber_reg_mode (rtx *, rtx, enum machine_mode);
 
 extern rtx copy_blkmode_to_reg (enum machine_mode, tree);
 
@@ -356,6 +357,13 @@ use_reg (rtx *fusage, rtx reg)
   use_reg_mode (fusage, reg, VOIDmode);
 }
 
+/* Mark REG as clobbered by the call with FUSAGE as CALL_INSN_FUNCTION_USAGE.  */
+static inline void
+clobber_reg (rtx *fusage, rtx reg)
+{
+  clobber_reg_mode (fusage, reg, VOIDmode);
+}
+
 /* Mark NREGS consecutive regs, starting at REGNO, as holding parameters
    for the next CALL_INSN.  */
 extern void use_regs (rtx *, int, int);

Add post_expand_call_insn hook

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1125 bytes --]

On 22-04-14 17:05, Tom de Vries wrote:
> I've updated the fuse-caller-save patch series to model non-callee call clobbers
> in CALL_INSN_FUNCTION_USAGE.

Eric,

this patch adds a post_expand_call_insn hook.

The hook is called right after expansion of calls, and allows a target to do 
additional processing, such as f.i. adding clobbers to CALL_INSN_FUNCTION_USAGE.

Instead of using the hook, we could add code to the preparation statements 
operand of the different call expands, but that requires those expands not to 
use the rtl template, and generate all the rtl through c code. Which requires a 
rewrite of the call expands in case of Aarch64.

Bootstrapped and reg-tested on x86_64 as part of the fuse-caller-save patch series.

OK for trunk?

Thanks,
- Tom

2014-04-18  Tom de Vries  <tom@codesourcery.com

	* target.def (post_expand_call_insn): New DEFHOOK.
	* calls.c (expand_call, emit_library_call_value_1): Call
	post_expand_call_insn hook.
	* tm.texi.in (@section Storage Layout): Add hook
	TARGET_POST_EXPAND_CALL_INSN.
	* hooks.c (hook_void_rtx): New function.
	* hooks.h (hook_void_rtx): Declare function.

[-- Attachment #2: 0011-Add-post_expand_call_insn-hook.patch --]
[-- Type: text/x-patch, Size: 3015 bytes --]

diff --git a/gcc/calls.c b/gcc/calls.c
index e798c7a..0777a02 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -3507,6 +3507,8 @@ expand_call (tree exp, rtx target, int ignore)
 
   free (stack_usage_map_buf);
 
+  targetm.post_expand_call_insn (last_call_insn ());
+
   return target;
 }
 
@@ -4344,6 +4346,8 @@ emit_library_call_value_1 (int retval, rtx orgfun, rtx value,
 
   free (stack_usage_map_buf);
 
+  targetm.post_expand_call_insn (last_call_insn ());
+
   return value;
 
 }
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index 8af8efd..40b5bb1 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -1408,6 +1408,11 @@ registers whenever the function being expanded has any SDmode
 usage.
 @end deftypefn
 
+@deftypefn {Target Hook} void TARGET_POST_EXPAND_CALL_INSN (rtx)
+This hook is called just after expansion of a call_expr into rtl, allowing
+the target to perform additional processing.
+@end deftypefn
+
 @deftypefn {Target Hook} void TARGET_INSTANTIATE_DECLS (void)
 This hook allows the backend to perform additional instantiations on rtl
 that are not actually in any insns yet, but will be later.
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index 8991c3c..812b0b8 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -1285,6 +1285,8 @@ The default definition of this macro returns false for all sizes.
 
 @hook TARGET_EXPAND_TO_RTL_HOOK
 
+@hook TARGET_POST_EXPAND_CALL_INSN
+
 @hook TARGET_INSTANTIATE_DECLS
 
 @hook TARGET_MANGLE_TYPE
diff --git a/gcc/hooks.c b/gcc/hooks.c
index 1c67bdf..53e8591 100644
--- a/gcc/hooks.c
+++ b/gcc/hooks.c
@@ -461,6 +461,13 @@ hook_void_rtx_int (rtx insn ATTRIBUTE_UNUSED, int mode ATTRIBUTE_UNUSED)
 {
 }
 
+/* Generic hook that takes a rtx and an int and returns void.  */
+
+void
+hook_void_rtx (rtx insn ATTRIBUTE_UNUSED)
+{
+}
+
 /* Generic hook that takes a struct gcc_options * and returns void.  */
 
 void
diff --git a/gcc/hooks.h b/gcc/hooks.h
index 896b41d..4df5ae0 100644
--- a/gcc/hooks.h
+++ b/gcc/hooks.h
@@ -66,6 +66,7 @@ extern bool hook_bool_dint_dint_uint_bool_true (double_int, double_int,
 
 extern void hook_void_void (void);
 extern void hook_void_constcharptr (const char *);
+extern void hook_void_rtx (rtx);
 extern void hook_void_rtx_int (rtx, int);
 extern void hook_void_FILEptr_constcharptr (FILE *, const char *);
 extern bool hook_bool_FILEptr_rtx_false (FILE *, rtx);
diff --git a/gcc/target.def b/gcc/target.def
index ae0bc9c..2f7178c 100644
--- a/gcc/target.def
+++ b/gcc/target.def
@@ -4639,6 +4639,15 @@ usage.",
  hook_void_void)
 
 /* This target hook allows the backend to perform additional
+   processing after expansion of a call insn.  */
+DEFHOOK
+(post_expand_call_insn,
+ "This hook is called just after expansion of a call_expr into rtl, allowing\n\
+the target to perform additional processing.",
+ void, (rtx),
+ hook_void_rtx)
+
+/* This target hook allows the backend to perform additional
    instantiations on rtx that are not actually in insns yet,
    but will be later.  */
 DEFHOOK

Re: -fuse-caller-save - Collect register usage information

[Vladimir Makarov]

On 2014-04-23, 6:41 AM, Tom de Vries wrote:
> On 22-04-14 17:05, Tom de Vries wrote:
>> I've updated the fuse-caller-save patch series to model non-callee
>> call clobbers
>> in CALL_INSN_FUNCTION_USAGE.
>
> Vladimir,
>
> This is the updated version of the previously approved patch
> http://gcc.gnu.org/ml/gcc-patches/2013-03/msg01320.html , updated for
> the new hook call_fusage_contains_non_callee_clobbers.
>
> The only difference is in the functions get_call_reg_set_usage and
> collect_fn_hard_reg_usage which use the hook.
>
> OK for trunk?
>
> 2013-04-29  Radovan Obradovic  <robradovic@mips.com>
>              Tom de Vries  <tom@codesourcery.com>
>
>      * cgraph.h (struct cgraph_node): Add function_used_regs,
>      function_used_regs_initialized and function_used_regs_valid fields.
>      * final.c: Move include of hard-reg-set.h to before rtl.h to declare
>      find_all_hard_reg_sets.
>      (collect_fn_hard_reg_usage, get_call_fndecl, get_call_cgraph_node)
>      (get_call_reg_set_usage): New function.
>      (rest_of_handle_final): Use collect_fn_hard_reg_usage.
>

It looks ok for me, Tom.  But to be straight I am not a maintainer for 
this part of the compiler.  So it is just my recommendation.  I guess to 
get an approval for these changes, you should ask Jan Hubicka (cgraph.h) 
or a global or RTL reviewer for final.c.

Re: Add call_fusage_contains_non_callee_clobbers hook

[Richard Sandiford]

Tom de Vries <Tom_deVries@mentor.com> writes:
> On 22-04-14 17:05, Tom de Vries wrote:
>> I've updated the fuse-caller-save patch series to model non-callee
>> call clobbers
>> in CALL_INSN_FUNCTION_USAGE.
>>
>
> Vladimir,
>
> This patch adds a hook to indicate whether a target has added the non-callee 
> call clobbers to CALL_INSN_FUNCTION_USAGE, meaning it's safe to do the 
> fuse-caller-save optimization.

FWIW I think this should be a plain bool rather than a function,
like delay_sched2 etc.

Thanks,
Richard

Re: -fuse-caller-save - Collect register usage information

[Richard Sandiford]

Tom de Vries <Tom_deVries@mentor.com> writes:
> +/* Collect hard register usage for the current function.  */
> +
> +static void
> +collect_fn_hard_reg_usage (void)
> +{
> +  rtx insn;
> +  int i;
> +  struct cgraph_node *node;
> +
> +  if (!flag_use_caller_save)
> +    return;
> +
> +  node = cgraph_get_node (current_function_decl);
> +  gcc_assert (node != NULL);
> +
> +  gcc_assert (!node->function_used_regs_initialized);
> +  node->function_used_regs_initialized = 1;
> +
> +  for (insn = get_insns (); insn != NULL_RTX; insn = next_insn (insn))
> +    {
> +      HARD_REG_SET insn_used_regs;
> +
> +      if (!NONDEBUG_INSN_P (insn))
> +	continue;
> +
> +      find_all_hard_reg_sets (insn, &insn_used_regs, false);
> +
> +      if (CALL_P (insn)
> +	  && (!targetm.call_fusage_contains_non_callee_clobbers ()
> +	      || !get_call_reg_set_usage (insn, &insn_used_regs, call_used_reg_set)))

If the uses of flag_use_caller_save also check
call_fusage_contains_non_callee_clobbers, would it be better to test
them both together here too, rather than waiting to see a call?

> +  /* Be conservative - mark fixed and global registers as used.  */
> +  IOR_HARD_REG_SET (node->function_used_regs, fixed_reg_set);
> +  for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
> +    if (global_regs[i])
> +      SET_HARD_REG_BIT (node->function_used_regs, i);

The loop isn't needed; all globals are fixed.

Thanks again for working on this.

Richard

Re: Add call_fusage_contains_non_callee_clobbers hook

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 734 bytes --]

On 23-04-14 17:10, Richard Sandiford wrote:
> FWIW I think this should be a plain bool rather than a function,
> like delay_sched2 etc.
>

Vladimir,

I've reimplemented the hook using DEFHOOKPOD instead of DEFHOOK, to make it a 
plain bool.

OK for trunk?

Thanks,
- Tom

2013-04-29  Radovan Obradovic  <robradovic@mips.com>
             Tom de Vries  <tom@codesourcery.com>

         * target.def (call_fusage_contains_non_callee_clobbers): New DEFHOOKPOD.
         * doc/tm.texi.in (@node Stack and Calling): Add Miscellaneous Register
         Hooks to @menu.
         (@node Miscellaneous Register Hooks): New node.
         (@hook TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS): New hook.
         * doc/tm.texi: Regenerate.



[-- Attachment #2: 0005-Add-call_fusage_contains_non_callee_clobbers-hook.patch --]
[-- Type: text/x-patch, Size: 3692 bytes --]

diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index b8ca17e..f06113d 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -3091,6 +3091,7 @@ This describes the stack layout and calling conventions.
 * Profiling::
 * Tail Calls::
 * Stack Smashing Protection::
+* Miscellaneous Register Hooks::
 @end menu
 
 @node Frame Layout
@@ -5016,6 +5017,21 @@ normally defined in @file{libgcc2.c}.
 Whether this target supports splitting the stack when the options described in @var{opts} have been passed.  This is called after options have been parsed, so the target may reject splitting the stack in some configurations.  The default version of this hook returns false.  If @var{report} is true, this function may issue a warning or error; if @var{report} is false, it must simply return a value
 @end deftypefn
 
+@node Miscellaneous Register Hooks
+@subsection Miscellaneous register hooks
+@cindex miscellaneous register hooks
+
+@deftypevr {Target Hook} bool TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS
+set to true if all the calls in the current function contain clobbers in
+CALL_INSN_FUNCTION_USAGE for the registers that are clobbered by the call
+rather than by the callee, and are not already set or clobbered in the call
+pattern.  Examples of such registers are registers used in PLTs and stubs,
+and temporary registers used in the call instruction but not present in the
+rtl pattern.  Another way to formulate it is the registers not present in the
+rtl pattern that are clobbered by the call assuming the callee does not
+clobber any register.  The default version of this hook is set to false.
+@end deftypevr
+
 @node Varargs
 @section Implementing the Varargs Macros
 @cindex varargs implementation
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index d793d26..8991c3c 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -2720,6 +2720,7 @@ This describes the stack layout and calling conventions.
 * Profiling::
 * Tail Calls::
 * Stack Smashing Protection::
+* Miscellaneous Register Hooks::
 @end menu
 
 @node Frame Layout
@@ -3985,6 +3986,12 @@ the function prologue.  Normally, the profiling code comes after.
 
 @hook TARGET_SUPPORTS_SPLIT_STACK
 
+@node Miscellaneous Register Hooks
+@subsection Miscellaneous register hooks
+@cindex miscellaneous register hooks
+
+@hook TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS
+
 @node Varargs
 @section Implementing the Varargs Macros
 @cindex varargs implementation
diff --git a/gcc/target.def b/gcc/target.def
index 3a64cd1..5787e13 100644
--- a/gcc/target.def
+++ b/gcc/target.def
@@ -5130,6 +5130,22 @@ FRAME_POINTER_REGNUM, ARG_POINTER_REGNUM, and the PIC_OFFSET_TABLE_REGNUM.",
  void, (bitmap regs),
  hook_void_bitmap)
 
+/* Targets should define this target hook to mark that non-callee clobbers are
+   present in CALL_INSN_FUNCTION_USAGE for all the calls in the current
+   function.  */
+DEFHOOKPOD
+(call_fusage_contains_non_callee_clobbers,
+ "set to true if all the calls in the current function contain clobbers in\n\
+CALL_INSN_FUNCTION_USAGE for the registers that are clobbered by the call\n\
+rather than by the callee, and are not already set or clobbered in the call\n\
+pattern.  Examples of such registers are registers used in PLTs and stubs,\n\
+and temporary registers used in the call instruction but not present in the\n\
+rtl pattern.  Another way to formulate it is the registers not present in the\n\
+rtl pattern that are clobbered by the call assuming the callee does not\n\
+clobber any register.  The default version of this hook is set to false.",
+ bool, 
+ false)
+
 /* Fill in additional registers set up by prologue into a regset.  */
 DEFHOOK
 (set_up_by_prologue,

Re: Add clobber_reg

[Eric Botcazou]

> Richard Sandiford mentioned here (
> http://gcc.gnu.org/ml/gcc-patches/2014-04/msg00870.html ):
> ...
> Although we really should have a utility function like use_reg, but for
> clobbers, so that the above would become:
> 
>    clobber_reg (&CALL_INSN_FUNCTION_USAGE (insn), gen_rtx_REG (word_mode,
> 18)); ...
> 
> 
> I've implemented a patch that adds clobber_reg and clobber_reg_mode, similar
> to use_reg and use_reg_mode.

Good idea indeed.

> 2014-04-18  Tom de Vries  <tom@codesourcery.com>
> 
> 	* expr.c (clobber_reg_mode): New function.
> 	* expr.h (clobber_reg): New function.

OK, thanks.

-- 
Eric Botcazou

Re: Add post_expand_call_insn hook

[Eric Botcazou]

> The hook is called right after expansion of calls, and allows a target to do
> additional processing, such as f.i. adding clobbers to
> CALL_INSN_FUNCTION_USAGE.
> 
> Instead of using the hook, we could add code to the preparation statements
> operand of the different call expands, but that requires those expands not
> to use the rtl template, and generate all the rtl through c code. Which
> requires a rewrite of the call expands in case of Aarch64.

If Aarch64 is the only problematic back-end, then it should be changed to do 
what the other back-ends already do to use use_reg.  IMO adding such a bogus 
hook should be the very last resort solution.

-- 
Eric Botcazou

-fuse-caller-save - Enable for MIPS

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1124 bytes --]

On 22-04-14 17:05, Tom de Vries wrote:
> I've updated the fuse-caller-save patch series to model non-callee call clobbers
> in CALL_INSN_FUNCTION_USAGE.
>

Richard,

this patch enables the fuse-caller-save optimization for MIPS.

It adds the $6 clobber in CALL_INSN_FUNCTION_USAGE when required, and sets 
TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS to true.

I've done a minimal rebuild for mips, ran the fuse-caller-save testcase and 
checked with -dP that I can find $6 in the C_I_F_U.

ok for trunk if a full MIPS build and test cycle succeeds?

Thanks,
- Tom

2014-01-12  Radovan Obradovic  <robradovic@mips.com>
             Tom de Vries  <tom@codesourcery.com>

	* config/mips/mips.h (POST_CALL_TMP_REG): Define.
	* config/mips/mips.c (mips_split_call): Use POST_CALL_TMP_REG.
	(TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS): Redefine to true.
	(mips_expand_call): Add POST_CALL_TMP_REG clobber.
	* config/mips/mips.md (define_expand "untyped_call"): Add
	POST_CALL_TMP_REG clobber.

	* gcc.target/mips/mips.exp: Add use-caller-save to -ffoo/-fno-foo
	options.
	* gcc.target/mips/fuse-caller-save.c: New test.

[-- Attachment #2: fuse-caller-save-Enable-for-MIPS.patch --]
[-- Type: text/x-patch, Size: 5192 bytes --]

diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index 45256e9..b61cd44 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -7027,11 +7027,17 @@ mips_expand_call (enum mips_call_type type, rtx result, rtx addr,
 {
   rtx orig_addr, pattern, insn;
   int fp_code;
+  rtx post_call_tmp_reg = gen_rtx_REG (word_mode, POST_CALL_TMP_REG);
 
   fp_code = aux == 0 ? 0 : (int) GET_MODE (aux);
   insn = mips16_build_call_stub (result, &addr, args_size, fp_code);
   if (insn)
     {
+      if (TARGET_EXPLICIT_RELOCS
+	  && TARGET_CALL_CLOBBERED_GP
+	  && !find_reg_note (insn, REG_NORETURN, 0))
+	clobber_reg (&CALL_INSN_FUNCTION_USAGE (insn), post_call_tmp_reg);
+
       gcc_assert (!lazy_p && type == MIPS_CALL_NORMAL);
       return insn;
     }
@@ -7087,7 +7093,13 @@ mips_expand_call (enum mips_call_type type, rtx result, rtx addr,
       pattern = fn (result, addr, args_size);
     }
 
-  return mips_emit_call_insn (pattern, orig_addr, addr, lazy_p);
+  insn = mips_emit_call_insn (pattern, orig_addr, addr, lazy_p);
+  if (TARGET_EXPLICIT_RELOCS
+      && TARGET_CALL_CLOBBERED_GP
+      && !find_reg_note (insn, REG_NORETURN, 0))
+    clobber_reg (&CALL_INSN_FUNCTION_USAGE (insn), post_call_tmp_reg);
+
+  return insn;
 }
 
 /* Split call instruction INSN into a $gp-clobbering call and
@@ -7099,10 +7111,8 @@ mips_split_call (rtx insn, rtx call_pattern)
 {
   emit_call_insn (call_pattern);
   if (!find_reg_note (insn, REG_NORETURN, 0))
-    /* Pick a temporary register that is suitable for both MIPS16 and
-       non-MIPS16 code.  $4 and $5 are used for returning complex double
-       values in soft-float code, so $6 is the first suitable candidate.  */
-    mips_restore_gp_from_cprestore_slot (gen_rtx_REG (Pmode, GP_ARG_FIRST + 2));
+    mips_restore_gp_from_cprestore_slot (gen_rtx_REG (Pmode,
+						      POST_CALL_TMP_REG));
 }
 
 /* Return true if a call to DECL may need to use JALX.  */
@@ -19134,6 +19144,9 @@ mips_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update)
 #undef TARGET_ATOMIC_ASSIGN_EXPAND_FENV
 #define TARGET_ATOMIC_ASSIGN_EXPAND_FENV mips_atomic_assign_expand_fenv
 
+#undef TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS
+#define TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS true
+
 struct gcc_target targetm = TARGET_INITIALIZER;
 \f
 #include "gt-mips.h"
diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h
index b25865b..8c5498a 100644
--- a/gcc/config/mips/mips.h
+++ b/gcc/config/mips/mips.h
@@ -2212,6 +2212,11 @@ enum reg_class
 #define FP_ARG_FIRST (FP_REG_FIRST + 12)
 #define FP_ARG_LAST  (FP_ARG_FIRST + MAX_ARGS_IN_REGISTERS - 1)
 
+/* Temporary register that is used after a call, and suitable for both
+   MIPS16 and non-MIPS16 code.  $4 and $5 are used for returning complex double
+   values in soft-float code, so $6 is the first suitable candidate.  */
+#define POST_CALL_TMP_REG (GP_ARG_FIRST + 2)
+
 /* 1 if N is a possible register number for function argument passing.
    We have no FP argument registers when soft-float.  When FP registers
    are 32 bits, we can't directly reference the odd numbered ones.  */
diff --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md
index f914ab6..e333c42 100644
--- a/gcc/config/mips/mips.md
+++ b/gcc/config/mips/mips.md
@@ -6787,8 +6787,15 @@
   ""
 {
   int i;
+  rtx insn;
+  rtx post_call_tmp_reg = gen_rtx_REG (word_mode, POST_CALL_TMP_REG);
 
-  emit_call_insn (GEN_CALL (operands[0], const0_rtx, NULL, const0_rtx));
+  insn = emit_call_insn (GEN_CALL (operands[0], const0_rtx, NULL, const0_rtx));
+
+  if (TARGET_EXPLICIT_RELOCS
+      && TARGET_CALL_CLOBBERED_GP
+      && !find_reg_note (insn, REG_NORETURN, 0))
+    clobber_reg (&CALL_INSN_FUNCTION_USAGE (insn), post_call_tmp_reg);
 
   for (i = 0; i < XVECLEN (operands[2], 0); i++)
     {
diff --git a/gcc/testsuite/gcc.target/mips/fuse-caller-save.c b/gcc/testsuite/gcc.target/mips/fuse-caller-save.c
new file mode 100644
index 0000000..1fd6c7d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/fuse-caller-save.c
@@ -0,0 +1,30 @@
+/* { dg-do compile } */
+/* { dg-options "-fuse-caller-save" } */
+/* { dg-skip-if "" { *-*-* }  { "*" } { "-Os" } } */
+/* Testing -fuse-caller-save optimization option.  */
+
+static int __attribute__((noinline)) NOCOMPRESSION
+bar (int x)
+{
+  return x + 3;
+}
+
+int __attribute__((noinline)) NOCOMPRESSION
+foo (int y)
+{
+  return y + bar (y);
+}
+
+int NOCOMPRESSION
+main (void)
+{
+  return !(foo (5) == 13);
+}
+
+/* Check that there are only 2 stack-saves: r31 in main and foo.  */
+
+/* Check that there only 2 sw/sd.  */
+/* { dg-final { scan-assembler-times "(?n)s\[wd\]\t\\\$.*,.*\\(\\\$sp\\)" 2 } } */
+
+/* Check that the first caller-save register is unused.  */
+/* { dg-final { scan-assembler-not "\\\$16" } } */
diff --git a/gcc/testsuite/gcc.target/mips/mips.exp b/gcc/testsuite/gcc.target/mips/mips.exp
index 8c72cff..6ad8160 100644
--- a/gcc/testsuite/gcc.target/mips/mips.exp
+++ b/gcc/testsuite/gcc.target/mips/mips.exp
@@ -305,6 +305,7 @@ foreach option {
     tree-vectorize
     unroll-all-loops
     unroll-loops
+    use-caller-save
 } {
     lappend mips_option_groups $option "-f(no-|)$option"
 }

Re: -fuse-caller-save - Enable for MIPS

[Richard Sandiford]

Tom de Vries <Tom_deVries@mentor.com> writes:
> diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
> index 45256e9..b61cd44 100644
> --- a/gcc/config/mips/mips.c
> +++ b/gcc/config/mips/mips.c
> @@ -7027,11 +7027,17 @@ mips_expand_call (enum mips_call_type type, rtx result, rtx addr,
>  {
>    rtx orig_addr, pattern, insn;
>    int fp_code;
> +  rtx post_call_tmp_reg = gen_rtx_REG (word_mode, POST_CALL_TMP_REG);
>  
>    fp_code = aux == 0 ? 0 : (int) GET_MODE (aux);
>    insn = mips16_build_call_stub (result, &addr, args_size, fp_code);
>    if (insn)
>      {
> +      if (TARGET_EXPLICIT_RELOCS
> +	  && TARGET_CALL_CLOBBERED_GP
> +	  && !find_reg_note (insn, REG_NORETURN, 0))
> +	clobber_reg (&CALL_INSN_FUNCTION_USAGE (insn), post_call_tmp_reg);
> +

I think this condition should go in mips_emit_call_insn instead,
so that we don't have 4 instances of it.  untyped_call could then
use mips_expand_call as well.

Until now there was no real downside to using $6 for non-MIPS16 code.
Now that there is, it would probably be worth making it:

+#define POST_CALL_TMP_REG \
  (TARGET_MIPS16 ? GP_ARG_FIRST + 2 : PIC_OFFSET_TABLE_REGNUM)

and only adding the clobber for MIPS16.

> diff --git a/gcc/testsuite/gcc.target/mips/fuse-caller-save.c b/gcc/testsuite/gcc.target/mips/fuse-caller-save.c
> new file mode 100644
> index 0000000..1fd6c7d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/mips/fuse-caller-save.c
> @@ -0,0 +1,30 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fuse-caller-save" } */
> +/* { dg-skip-if "" { *-*-* }  { "*" } { "-Os" } } */

I might have asked this before, sorry, but why this skip?  Please add a brief
comment (in the string, if short enough).

> +/* Testing -fuse-caller-save optimization option.  */
> +
> +static int __attribute__((noinline)) NOCOMPRESSION
> +bar (int x)
> +{
> +  return x + 3;
> +}
> +
> +int __attribute__((noinline)) NOCOMPRESSION
> +foo (int y)
> +{
> +  return y + bar (y);
> +}
> +
> +int NOCOMPRESSION
> +main (void)
> +{
> +  return !(foo (5) == 13);
> +}
> +
> +/* Check that there are only 2 stack-saves: r31 in main and foo.  */
> +
> +/* Check that there only 2 sw/sd.  */
> +/* { dg-final { scan-assembler-times "(?n)s\[wd\]\t\\\$.*,.*\\(\\\$sp\\)" 2 } } */
> +
> +/* Check that the first caller-save register is unused.  */
> +/* { dg-final { scan-assembler-not "\\\$16" } } */

It'd be good to avoid NOCOMPRESSION because the only case that really
needs the temporary register is MIPS16.  Please try putting the test
in a header file and reusing it for three tests, one each of MIPS16,
microMIPS and uncompressed.

Thanks,
Richard

Re: -fuse-caller-save - Enable for MIPS

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 3001 bytes --]

On 25-04-14 15:22, Richard Sandiford wrote:
> Tom de Vries <Tom_deVries@mentor.com> writes:
>> diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
>> index 45256e9..b61cd44 100644
>> --- a/gcc/config/mips/mips.c
>> +++ b/gcc/config/mips/mips.c
>> @@ -7027,11 +7027,17 @@ mips_expand_call (enum mips_call_type type, rtx result, rtx addr,
>>   {
>>     rtx orig_addr, pattern, insn;
>>     int fp_code;
>> +  rtx post_call_tmp_reg = gen_rtx_REG (word_mode, POST_CALL_TMP_REG);
>>
>>     fp_code = aux == 0 ? 0 : (int) GET_MODE (aux);
>>     insn = mips16_build_call_stub (result, &addr, args_size, fp_code);
>>     if (insn)
>>       {
>> +      if (TARGET_EXPLICIT_RELOCS
>> +	  && TARGET_CALL_CLOBBERED_GP
>> +	  && !find_reg_note (insn, REG_NORETURN, 0))
>> +	clobber_reg (&CALL_INSN_FUNCTION_USAGE (insn), post_call_tmp_reg);
>> +
>
> I think this condition should go in mips_emit_call_insn instead,
> so that we don't have 4 instances of it.  untyped_call could then
> use mips_expand_call as well.
>

Richard,

Done.

> Until now there was no real downside to using $6 for non-MIPS16 code.
> Now that there is, it would probably be worth making it:
>
> +#define POST_CALL_TMP_REG \
>    (TARGET_MIPS16 ? GP_ARG_FIRST + 2 : PIC_OFFSET_TABLE_REGNUM)
>
> and only adding the clobber for MIPS16.
>

Done.

>> diff --git a/gcc/testsuite/gcc.target/mips/fuse-caller-save.c b/gcc/testsuite/gcc.target/mips/fuse-caller-save.c
>> new file mode 100644
>> index 0000000..1fd6c7d
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/mips/fuse-caller-save.c
>> @@ -0,0 +1,30 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-fuse-caller-save" } */
>> +/* { dg-skip-if "" { *-*-* }  { "*" } { "-Os" } } */
>
> I might have asked this before, sorry, but why this skip?  Please add a brief
> comment (in the string, if short enough).

I've reduced the amount of skips a bit, and added a comment why they are skipped.

>
>> +/* Testing -fuse-caller-save optimization option.  */
>> +
>> +static int __attribute__((noinline)) NOCOMPRESSION
>> +bar (int x)
>> +{
>> +  return x + 3;
>> +}
>> +
>> +int __attribute__((noinline)) NOCOMPRESSION
>> +foo (int y)
>> +{
>> +  return y + bar (y);
>> +}
>> +
>> +int NOCOMPRESSION
>> +main (void)
>> +{
>> +  return !(foo (5) == 13);
>> +}
>> +
>> +/* Check that there are only 2 stack-saves: r31 in main and foo.  */
>> +
>> +/* Check that there only 2 sw/sd.  */
>> +/* { dg-final { scan-assembler-times "(?n)s\[wd\]\t\\\$.*,.*\\(\\\$sp\\)" 2 } } */
>> +
>> +/* Check that the first caller-save register is unused.  */
>> +/* { dg-final { scan-assembler-not "\\\$16" } } */
>
> It'd be good to avoid NOCOMPRESSION because the only case that really
> needs the temporary register is MIPS16.  Please try putting the test
> in a header file and reusing it for three tests, one each of MIPS16,
> microMIPS and uncompressed.
>

Done, I think, I'm not 100% sure I understood what you wanted me to do here.

build and reg-tested on MIPS.

OK for trunk?

Thanks,
- Tom


[-- Attachment #2: 0006-fuse-caller-save-Enable-for-MIPS.patch --]
[-- Type: text/x-patch, Size: 9703 bytes --]

2014-01-12  Radovan Obradovic  <robradovic@mips.com>
            Tom de Vries  <tom@codesourcery.com>

	* config/mips/mips-protos.h (mips_emit_call_insn): Declare.
	* config/mips/mips.h (POST_CALL_TMP_REG): Define.
	* config/mips/mips.c (mips_emit_call_insn): Remove static.  Use
	last_call_insn.  Add POST_CALL_TMP_REG clobber
	 (mips_split_call): Use POST_CALL_TMP_REG.
	(TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS): Redefine to true.
	* config/mips/mips.md (define_expand "untyped_call"): Use
	mips_emit_call_insn.

	* gcc.target/mips/mips.exp: Add use-caller-save to -ffoo/-fno-foo
	options.
	* gcc.target/mips/fuse-caller-save.h: New include file.
	* gcc.target/mips/fuse-caller-save.c: New test.
	* gcc.target/mips/fuse-caller-save-mips16.c: Same.
	* gcc.target/mips/fuse-caller-save-micromips.c: Same.
---
 gcc/config/mips/mips-protos.h                      |  1 +
 gcc/config/mips/mips.c                             | 24 ++++++++++++++++------
 gcc/config/mips/mips.h                             |  7 +++++++
 gcc/config/mips/mips.md                            |  4 +++-
 .../gcc.target/mips/fuse-caller-save-micromips.c   | 17 +++++++++++++++
 .../gcc.target/mips/fuse-caller-save-mip16.c       | 17 +++++++++++++++
 gcc/testsuite/gcc.target/mips/fuse-caller-save.c   | 17 +++++++++++++++
 gcc/testsuite/gcc.target/mips/fuse-caller-save.h   | 17 +++++++++++++++
 gcc/testsuite/gcc.target/mips/mips.exp             |  1 +
 9 files changed, 98 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/mips/fuse-caller-save-micromips.c
 create mode 100644 gcc/testsuite/gcc.target/mips/fuse-caller-save-mip16.c
 create mode 100644 gcc/testsuite/gcc.target/mips/fuse-caller-save.c
 create mode 100644 gcc/testsuite/gcc.target/mips/fuse-caller-save.h

diff --git a/gcc/config/mips/mips-protos.h b/gcc/config/mips/mips-protos.h
index 3d59b7b..19ea919 100644
--- a/gcc/config/mips/mips-protos.h
+++ b/gcc/config/mips/mips-protos.h
@@ -232,6 +232,7 @@ extern bool mips_use_pic_fn_addr_reg_p (const_rtx);
 extern rtx mips_expand_call (enum mips_call_type, rtx, rtx, rtx, rtx, bool);
 extern void mips_split_call (rtx, rtx);
 extern bool mips_get_pic_call_symbol (rtx *, int);
+extern rtx mips_emit_call_insn (rtx, rtx, rtx, bool);
 extern void mips_expand_fcc_reload (rtx, rtx, rtx);
 extern void mips_set_return_address (rtx, rtx);
 extern bool mips_move_by_pieces_p (unsigned HOST_WIDE_INT, unsigned int);
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index 45256e9..5b7dd3f 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -2814,12 +2814,13 @@ mips_force_temporary (rtx dest, rtx value)
    ADDR is the legitimized form, and LAZY_P is true if the call
    address is lazily-bound.  */
 
-static rtx
+rtx
 mips_emit_call_insn (rtx pattern, rtx orig_addr, rtx addr, bool lazy_p)
 {
   rtx insn, reg;
 
-  insn = emit_call_insn (pattern);
+  emit_call_insn (pattern);
+  insn = last_call_insn ();
 
   if (TARGET_MIPS16 && mips_use_pic_fn_addr_reg_p (orig_addr))
     {
@@ -2843,6 +2844,16 @@ mips_emit_call_insn (rtx pattern, rtx orig_addr, rtx addr, bool lazy_p)
 	       gen_rtx_REG (Pmode, GOT_VERSION_REGNUM));
       emit_insn (gen_update_got_version ());
     }
+
+  if (TARGET_MIPS16
+      && TARGET_EXPLICIT_RELOCS
+      && TARGET_CALL_CLOBBERED_GP
+      && !find_reg_note (insn, REG_NORETURN, 0))
+    {
+      rtx post_call_tmp_reg = gen_rtx_REG (word_mode, POST_CALL_TMP_REG);
+      clobber_reg (&CALL_INSN_FUNCTION_USAGE (insn), post_call_tmp_reg);
+    }
+
   return insn;
 }
 \f
@@ -7099,10 +7110,8 @@ mips_split_call (rtx insn, rtx call_pattern)
 {
   emit_call_insn (call_pattern);
   if (!find_reg_note (insn, REG_NORETURN, 0))
-    /* Pick a temporary register that is suitable for both MIPS16 and
-       non-MIPS16 code.  $4 and $5 are used for returning complex double
-       values in soft-float code, so $6 is the first suitable candidate.  */
-    mips_restore_gp_from_cprestore_slot (gen_rtx_REG (Pmode, GP_ARG_FIRST + 2));
+    mips_restore_gp_from_cprestore_slot (gen_rtx_REG (Pmode,
+						      POST_CALL_TMP_REG));
 }
 
 /* Return true if a call to DECL may need to use JALX.  */
@@ -19134,6 +19143,9 @@ mips_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update)
 #undef TARGET_ATOMIC_ASSIGN_EXPAND_FENV
 #define TARGET_ATOMIC_ASSIGN_EXPAND_FENV mips_atomic_assign_expand_fenv
 
+#undef TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS
+#define TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS true
+
 struct gcc_target targetm = TARGET_INITIALIZER;
 \f
 #include "gt-mips.h"
diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h
index b25865b..2a1cfff 100644
--- a/gcc/config/mips/mips.h
+++ b/gcc/config/mips/mips.h
@@ -2212,6 +2212,13 @@ enum reg_class
 #define FP_ARG_FIRST (FP_REG_FIRST + 12)
 #define FP_ARG_LAST  (FP_ARG_FIRST + MAX_ARGS_IN_REGISTERS - 1)
 
+/* Temporary register that is used after a call.  $4 and $5 are used for
+   returning complex double values in soft-float code, so $6 is the first
+   suitable candidate for !TARGET_MIPS16.  For TARGET_MIPS16, we use
+   PIC_OFFSET_TABLE_REGNUM instead.  */
+#define POST_CALL_TMP_REG \
+  (TARGET_MIPS16 ? GP_ARG_FIRST + 2 : PIC_OFFSET_TABLE_REGNUM)
+
 /* 1 if N is a possible register number for function argument passing.
    We have no FP argument registers when soft-float.  When FP registers
    are 32 bits, we can't directly reference the odd numbered ones.  */
diff --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md
index f914ab6..6254376 100644
--- a/gcc/config/mips/mips.md
+++ b/gcc/config/mips/mips.md
@@ -6787,8 +6787,10 @@
   ""
 {
   int i;
+  rtx insn, call;
 
-  emit_call_insn (GEN_CALL (operands[0], const0_rtx, NULL, const0_rtx));
+  call = GEN_CALL (operands[0], const0_rtx, NULL, const0_rtx);
+  insn = mips_emit_call_insn (call, operands[0], operands[0], false); 
 
   for (i = 0; i < XVECLEN (operands[2], 0); i++)
     {
diff --git a/gcc/testsuite/gcc.target/mips/fuse-caller-save-micromips.c b/gcc/testsuite/gcc.target/mips/fuse-caller-save-micromips.c
new file mode 100644
index 0000000..9fbd8a4
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/fuse-caller-save-micromips.c
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-fuse-caller-save (-mmicromips)" } */
+/* The scan of the sp-relative saves will fail for -O0 and -O1.
+   For -flto, scans will fail because there's no code in the .s file.  */
+/* { dg-skip-if "" { *-*-* }  { "-O0" "-O1" "-flto"} } */
+/* Testing -fuse-caller-save optimization option.  */
+
+#define ATTRIBUTE MICROMIPS
+#include "fuse-caller-save.h"
+
+/* Check that there are only 2 stack-saves: r31 in main and foo.  */
+
+/* Check that there only 2 sw/sd.  */
+/* { dg-final { scan-assembler-times "(?n)s\[wd\]\t\\\$.*,.*\\(\\\$sp\\)" 2 } } */
+
+/* Check that the first caller-save register is unused.  */
+/* { dg-final { scan-assembler-not "\\\$16" } } */
diff --git a/gcc/testsuite/gcc.target/mips/fuse-caller-save-mip16.c b/gcc/testsuite/gcc.target/mips/fuse-caller-save-mip16.c
new file mode 100644
index 0000000..d083394
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/fuse-caller-save-mip16.c
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-fuse-caller-save (-mips16)" } */
+/* The scan of the sp-relative saves will fail for -O0 and -O1.
+   For -flto, scans will fail because there's no code in the .s file.  */
+/* { dg-skip-if "" { *-*-* }  { "-O0" "-O1" "-flto"} } */
+/* Testing -fuse-caller-save optimization option.  */
+
+#define ATTRIBUTE MIPS16
+#include "fuse-caller-save.h"
+
+/* Check that there are only 2 stack-saves: r31 in main and foo.  */
+
+/* Check that there only 2 sw/sd.  */
+/* { dg-final { scan-assembler-times "(?n)s\[wd\]\t\\\$.*,.*\\(\\\$sp\\)" 2 } } */
+
+/* Check that the first caller-save register is unused.  */
+/* { dg-final { scan-assembler-not "\\\$16" } } */
diff --git a/gcc/testsuite/gcc.target/mips/fuse-caller-save.c b/gcc/testsuite/gcc.target/mips/fuse-caller-save.c
new file mode 100644
index 0000000..1ad0023
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/fuse-caller-save.c
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-fuse-caller-save" } */
+/* The scan of the sp-relative saves will fail for -O0 and -O1.
+   For -flto, scans will fail because there's no code in the .s file.  */
+/* { dg-skip-if "" { *-*-* }  { "-O0" "-O1" "-flto"} } */
+/* Testing -fuse-caller-save optimization option.  */
+
+#define ATTRIBUTE NOCOMPRESSION
+#include "fuse-caller-save.h"
+
+/* Check that there are only 2 stack-saves: r31 in main and foo.  */
+
+/* Check that there only 2 sw/sd.  */
+/* { dg-final { scan-assembler-times "(?n)s\[wd\]\t\\\$.*,.*\\(\\\$sp\\)" 2 } } */
+
+/* Check that the first caller-save register is unused.  */
+/* { dg-final { scan-assembler-not "\\\$16" } } */
diff --git a/gcc/testsuite/gcc.target/mips/fuse-caller-save.h b/gcc/testsuite/gcc.target/mips/fuse-caller-save.h
new file mode 100644
index 0000000..edf6039
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/fuse-caller-save.h
@@ -0,0 +1,17 @@
+static int __attribute__((noinline)) ATTRIBUTE
+bar (int x)
+{
+  return x + 3;
+}
+
+int __attribute__((noinline)) ATTRIBUTE
+foo (int y)
+{
+  return y + bar (y);
+}
+
+int ATTRIBUTE
+main (void)
+{
+  return !(foo (5) == 13);
+}
diff --git a/gcc/testsuite/gcc.target/mips/mips.exp b/gcc/testsuite/gcc.target/mips/mips.exp
index 8c72cff..6ad8160 100644
--- a/gcc/testsuite/gcc.target/mips/mips.exp
+++ b/gcc/testsuite/gcc.target/mips/mips.exp
@@ -305,6 +305,7 @@ foreach option {
     tree-vectorize
     unroll-all-loops
     unroll-loops
+    use-caller-save
 } {
     lappend mips_option_groups $option "-f(no-|)$option"
 }
-- 
1.8.3.2

Re: -fuse-caller-save - Collect register usage information

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 774 bytes --]

Eric,
Honza,

This patch adds analysis in pass_final to track which hard registers are set or
clobbered by the function body, and stores that information in a
struct cgraph_node, to be used in the fuse-caller-save optmization.

This is the updated version of the previously approved patch
submitted here (http://gcc.gnu.org/ml/gcc-patches/2013-03/msg01320.html ).
The changes are:
- using a new hook call_fusage_contains_non_callee_clobbers,
- incorporating minor review comments from Richard Sandiford
   ( http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01436.html ).

As part of the fuse-caller-save patch series, bootstrapped and reg-tested on 
x86_64, and build and reg-tested on MIPS.

Eric, non-cgraph part OK for trunk?

Honza, cgraph part OK for trunk?

Thanks,
- Tom

[-- Attachment #2: 0002-fuse-caller-save-Collect-register-usage-information.patch --]
[-- Type: text/x-patch, Size: 5786 bytes --]

2013-04-29  Radovan Obradovic  <robradovic@mips.com>
            Tom de Vries  <tom@codesourcery.com>

	* cgraph.h (struct cgraph_node): Add function_used_regs,
	function_used_regs_initialized and function_used_regs_valid fields.
	* final.c: Move include of hard-reg-set.h to before rtl.h to declare
	find_all_hard_reg_sets.
	(collect_fn_hard_reg_usage, get_call_fndecl, get_call_cgraph_node)
	(get_call_reg_set_usage): New function.
	(rest_of_handle_final): Use collect_fn_hard_reg_usage.
---
 gcc/cgraph.h |   7 ++++
 gcc/final.c  | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 gcc/regs.h   |   4 ++
 3 files changed, 127 insertions(+), 1 deletion(-)

diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index 84fc1d9..bbbbd1f 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -408,6 +408,13 @@ public:
   /* Time profiler: first run of function.  */
   int tp_first_run;
 
+  /* Call unsaved hard registers really used by the corresponding
+     function (including ones used by functions called by the
+     function).  */
+  HARD_REG_SET function_used_regs;
+  /* Set if function_used_regs is valid.  */
+  unsigned function_used_regs_valid: 1;
+
   /* Set when decl is an abstract function pointed to by the
      ABSTRACT_DECL_ORIGIN of a reachable function.  */
   unsigned used_as_abstract_origin : 1;
diff --git a/gcc/final.c b/gcc/final.c
index 8c6f6ee..7b79059 100644
--- a/gcc/final.c
+++ b/gcc/final.c
@@ -49,6 +49,7 @@ along with GCC; see the file COPYING3.  If not see
 
 #include "tree.h"
 #include "varasm.h"
+#include "hard-reg-set.h"
 #include "rtl.h"
 #include "tm_p.h"
 #include "regs.h"
@@ -57,7 +58,6 @@ along with GCC; see the file COPYING3.  If not see
 #include "recog.h"
 #include "conditions.h"
 #include "flags.h"
-#include "hard-reg-set.h"
 #include "output.h"
 #include "except.h"
 #include "function.h"
@@ -223,6 +223,7 @@ static int alter_cond (rtx);
 static int final_addr_vec_align (rtx);
 #endif
 static int align_fuzz (rtx, rtx, int, unsigned);
+static void collect_fn_hard_reg_usage (void);
 \f
 /* Initialize data in final at the beginning of a compilation.  */
 
@@ -4426,6 +4427,7 @@ rest_of_handle_final (void)
   assemble_start_function (current_function_decl, fnname);
   final_start_function (get_insns (), asm_out_file, optimize);
   final (get_insns (), asm_out_file, optimize);
+  collect_fn_hard_reg_usage ();
   final_end_function ();
 
   /* The IA-64 ".handlerdata" directive must be issued before the ".endp"
@@ -4724,3 +4726,116 @@ make_pass_clean_state (gcc::context *ctxt)
 {
   return new pass_clean_state (ctxt);
 }
+
+/* Collect hard register usage for the current function.  */
+
+static void
+collect_fn_hard_reg_usage (void)
+{
+  rtx insn;
+  int i;
+  struct cgraph_node *node;
+
+  if (!flag_use_caller_save
+      || !targetm.call_fusage_contains_non_callee_clobbers)
+    return;
+
+  node = cgraph_get_node (current_function_decl);
+  gcc_assert (node != NULL);
+
+  for (insn = get_insns (); insn != NULL_RTX; insn = next_insn (insn))
+    {
+      HARD_REG_SET insn_used_regs;
+
+      if (!NONDEBUG_INSN_P (insn))
+	continue;
+
+      find_all_hard_reg_sets (insn, &insn_used_regs, false);
+
+      if (CALL_P (insn)
+	  && !get_call_reg_set_usage (insn, &insn_used_regs, call_used_reg_set))
+	{
+	  CLEAR_HARD_REG_SET (node->function_used_regs);
+	  return;
+	}
+
+      IOR_HARD_REG_SET (node->function_used_regs, insn_used_regs);
+    }
+
+  /* Be conservative - mark fixed and global registers as used.  */
+  IOR_HARD_REG_SET (node->function_used_regs, fixed_reg_set);
+  for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
+    if (global_regs[i])
+      SET_HARD_REG_BIT (node->function_used_regs, i);
+
+#ifdef STACK_REGS
+  /* Handle STACK_REGS conservatively, since the df-framework does not
+     provide accurate information for them.  */
+
+  for (i = FIRST_STACK_REG; i <= LAST_STACK_REG; i++)
+    SET_HARD_REG_BIT (node->function_used_regs, i);
+#endif
+
+  node->function_used_regs_valid = 1;
+}
+
+/* Get the declaration of the function called by INSN.  */
+
+static tree
+get_call_fndecl (rtx insn)
+{
+  rtx note, datum;
+
+  if (!flag_use_caller_save)
+    return NULL_TREE;
+
+  note = find_reg_note (insn, REG_CALL_DECL, NULL_RTX);
+  if (note == NULL_RTX)
+    return NULL_TREE;
+
+  datum = XEXP (note, 0);
+  if (datum != NULL_RTX)
+    return SYMBOL_REF_DECL (datum);
+
+  return NULL_TREE;
+}
+
+static struct cgraph_node *
+get_call_cgraph_node (rtx insn)
+{
+  tree fndecl;
+
+  if (insn == NULL_RTX)
+    return NULL;
+
+  fndecl = get_call_fndecl (insn);
+  if (fndecl == NULL_TREE
+      || !targetm.binds_local_p (fndecl))
+    return NULL;
+
+  return cgraph_get_node (fndecl);
+}
+
+/* Find hard registers used by function call instruction INSN, and return them
+   in REG_SET.  Return DEFAULT_SET in REG_SET if not found.  */
+
+bool
+get_call_reg_set_usage (rtx insn, HARD_REG_SET *reg_set,
+			HARD_REG_SET default_set)
+{
+  if (flag_use_caller_save
+      && targetm.call_fusage_contains_non_callee_clobbers)
+    {
+      struct cgraph_node *node = get_call_cgraph_node (insn);
+      if (node != NULL
+	  && node->function_used_regs_valid)
+	{
+	  COPY_HARD_REG_SET (*reg_set, node->function_used_regs);
+	  AND_HARD_REG_SET (*reg_set, default_set);
+	  return true;
+	}
+    }
+
+  COPY_HARD_REG_SET (*reg_set, default_set);
+  return false;
+}
diff --git a/gcc/regs.h b/gcc/regs.h
index 006caca..44cc005 100644
--- a/gcc/regs.h
+++ b/gcc/regs.h
@@ -421,4 +421,8 @@ range_in_hard_reg_set_p (const HARD_REG_SET set, unsigned regno, int nregs)
   return true;
 }
 
+/* Get registers used by given function call instruction.  */
+extern bool get_call_reg_set_usage (rtx insn, HARD_REG_SET *reg_set,
+				    HARD_REG_SET default_set);
+
 #endif /* GCC_REGS_H */
-- 
1.8.3.2

Re: -fuse-caller-save - Enable for MIPS

[Richard Sandiford]

Tom de Vries <Tom_deVries@mentor.com> writes:
> 2014-01-12  Radovan Obradovic  <robradovic@mips.com>
>             Tom de Vries  <tom@codesourcery.com>
>
> 	* config/mips/mips-protos.h (mips_emit_call_insn): Declare.
> 	* config/mips/mips.h (POST_CALL_TMP_REG): Define.
> 	* config/mips/mips.c (mips_emit_call_insn): Remove static.  Use
> 	last_call_insn.  Add POST_CALL_TMP_REG clobber
> 	 (mips_split_call): Use POST_CALL_TMP_REG.
> 	(TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS): Redefine to true.
> 	* config/mips/mips.md (define_expand "untyped_call"): Use
> 	mips_emit_call_insn.
>
> 	* gcc.target/mips/mips.exp: Add use-caller-save to -ffoo/-fno-foo
> 	options.
> 	* gcc.target/mips/fuse-caller-save.h: New include file.
> 	* gcc.target/mips/fuse-caller-save.c: New test.
> 	* gcc.target/mips/fuse-caller-save-mips16.c: Same.
> 	* gcc.target/mips/fuse-caller-save-micromips.c: Same.

Sorry, a couple of things, but this is looking pretty good:

>  mips_emit_call_insn (rtx pattern, rtx orig_addr, rtx addr, bool lazy_p)
>  {
>    rtx insn, reg;
>  
> -  insn = emit_call_insn (pattern);
> +  emit_call_insn (pattern);
> +  insn = last_call_insn ();
>  
>    if (TARGET_MIPS16 && mips_use_pic_fn_addr_reg_p (orig_addr))
>      {

This change isn't necessary; emit_call_insn is defined to return a CALL_INSN.

> @@ -2843,6 +2844,16 @@ mips_emit_call_insn (rtx pattern, rtx orig_addr, rtx addr, bool lazy_p)
>  	       gen_rtx_REG (Pmode, GOT_VERSION_REGNUM));
>        emit_insn (gen_update_got_version ());
>      }
> +
> +  if (TARGET_MIPS16
> +      && TARGET_EXPLICIT_RELOCS
> +      && TARGET_CALL_CLOBBERED_GP
> +      && !find_reg_note (insn, REG_NORETURN, 0))
> +    {
> +      rtx post_call_tmp_reg = gen_rtx_REG (word_mode, POST_CALL_TMP_REG);
> +      clobber_reg (&CALL_INSN_FUNCTION_USAGE (insn), post_call_tmp_reg);
> +    }

The REG_NORETURN note won't be around yet, so we might as well drop
that line.  I'm not sure how useful it would be anyway since values
are never live across a noreturn call.

> +/* Temporary register that is used after a call.  $4 and $5 are used for

Might as well make it "...used when restoring $gp after a call", now that
it's not as obvious from context.

> +   returning complex double values in soft-float code, so $6 is the first
> +   suitable candidate for !TARGET_MIPS16.  For TARGET_MIPS16, we use
> +   PIC_OFFSET_TABLE_REGNUM instead.  */

!TARGET_MIPS16 and TARGET_MIPS16 are the wrong way around:

   suitable candidate for TARGET_MIPS16.  For !TARGET_MIPS16 we can use
   $gp itself as the temporary.  */

> +/* The scan of the sp-relative saves will fail for -O0 and -O1.
> +   For -flto, scans will fail because there's no code in the .s file.  */
> +/* { dg-skip-if "" { *-*-* }  { "-O0" "-O1" "-flto"} } */

The -flto thing is handled automatically by the testsuite
(see force_conventional_output_for) so that one should be left out.

I'm a bit surprised that it doesn't work at -O1 for a simple test
like this though.  What goes wrong?

Thanks,
Richard

Re: -fuse-caller-save - Enable for MIPS

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 3519 bytes --]

On 27-04-14 12:27, Richard Sandiford wrote:
> Tom de Vries <Tom_deVries@mentor.com> writes:
>> 2014-01-12  Radovan Obradovic  <robradovic@mips.com>
>>              Tom de Vries  <tom@codesourcery.com>
>>
>> 	* config/mips/mips-protos.h (mips_emit_call_insn): Declare.
>> 	* config/mips/mips.h (POST_CALL_TMP_REG): Define.
>> 	* config/mips/mips.c (mips_emit_call_insn): Remove static.  Use
>> 	last_call_insn.  Add POST_CALL_TMP_REG clobber
>> 	 (mips_split_call): Use POST_CALL_TMP_REG.
>> 	(TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS): Redefine to true.
>> 	* config/mips/mips.md (define_expand "untyped_call"): Use
>> 	mips_emit_call_insn.
>>
>> 	* gcc.target/mips/mips.exp: Add use-caller-save to -ffoo/-fno-foo
>> 	options.
>> 	* gcc.target/mips/fuse-caller-save.h: New include file.
>> 	* gcc.target/mips/fuse-caller-save.c: New test.
>> 	* gcc.target/mips/fuse-caller-save-mips16.c: Same.
>> 	* gcc.target/mips/fuse-caller-save-micromips.c: Same.
>
> Sorry, a couple of things, but this is looking pretty good:
>
>>   mips_emit_call_insn (rtx pattern, rtx orig_addr, rtx addr, bool lazy_p)
>>   {
>>     rtx insn, reg;
>>
>> -  insn = emit_call_insn (pattern);
>> +  emit_call_insn (pattern);
>> +  insn = last_call_insn ();
>>
>>     if (TARGET_MIPS16 && mips_use_pic_fn_addr_reg_p (orig_addr))
>>       {
>
> This change isn't necessary; emit_call_insn is defined to return a CALL_INSN.
>

I dropped this change, as well as the change in the untyped_call expand, I 
realized it's unnecessary.

>> @@ -2843,6 +2844,16 @@ mips_emit_call_insn (rtx pattern, rtx orig_addr, rtx addr, bool lazy_p)
>>   	       gen_rtx_REG (Pmode, GOT_VERSION_REGNUM));
>>         emit_insn (gen_update_got_version ());
>>       }
>> +
>> +  if (TARGET_MIPS16
>> +      && TARGET_EXPLICIT_RELOCS
>> +      && TARGET_CALL_CLOBBERED_GP
>> +      && !find_reg_note (insn, REG_NORETURN, 0))
>> +    {
>> +      rtx post_call_tmp_reg = gen_rtx_REG (word_mode, POST_CALL_TMP_REG);
>> +      clobber_reg (&CALL_INSN_FUNCTION_USAGE (insn), post_call_tmp_reg);
>> +    }
>
> The REG_NORETURN note won't be around yet, so we might as well drop
> that line.  I'm not sure how useful it would be anyway since values
> are never live across a noreturn call.
>

Done.

>> +/* Temporary register that is used after a call.  $4 and $5 are used for
>
> Might as well make it "...used when restoring $gp after a call", now that
> it's not as obvious from context.
>
>> +   returning complex double values in soft-float code, so $6 is the first
>> +   suitable candidate for !TARGET_MIPS16.  For TARGET_MIPS16, we use
>> +   PIC_OFFSET_TABLE_REGNUM instead.  */
>
> !TARGET_MIPS16 and TARGET_MIPS16 are the wrong way around:
>
>     suitable candidate for TARGET_MIPS16.  For !TARGET_MIPS16 we can use
>     $gp itself as the temporary.  */
>

Fixed, thanks for catching that.

>> +/* The scan of the sp-relative saves will fail for -O0 and -O1.
>> +   For -flto, scans will fail because there's no code in the .s file.  */
>> +/* { dg-skip-if "" { *-*-* }  { "-O0" "-O1" "-flto"} } */
>
> The -flto thing is handled automatically by the testsuite
> (see force_conventional_output_for) so that one should be left out.
>

Ah, I see. Removed.

> I'm a bit surprised that it doesn't work at -O1 for a simple test
> like this though.  What goes wrong?
>

AFAIU now the problem is that the optimization doesn't trigger for -O0 and -01, 
because the register allocator behaves more conservatively.

OK for trunk, if re-testing succeeds?

Thanks,
- Tom


[-- Attachment #2: 0006-fuse-caller-save-Enable-for-MIPS.patch --]
[-- Type: text/x-patch, Size: 8838 bytes --]

2014-01-12  Radovan Obradovic  <robradovic@mips.com>
            Tom de Vries  <tom@codesourcery.com>

	* config/mips/mips-protos.h (mips_emit_call_insn): Declare.
	* config/mips/mips.h (POST_CALL_TMP_REG): Define.
	* config/mips/mips.c (mips_emit_call_insn): Remove static.  Add
	POST_CALL_TMP_REG clobber.
	(mips_split_call): Use POST_CALL_TMP_REG.
	(TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS): Redefine to true.

	* gcc.target/mips/mips.exp: Add use-caller-save to -ffoo/-fno-foo
	options.
	* gcc.target/mips/fuse-caller-save.h: New include file.
	* gcc.target/mips/fuse-caller-save.c: New test.
	* gcc.target/mips/fuse-caller-save-mips16.c: Same.
	* gcc.target/mips/fuse-caller-save-micromips.c: Same.
---
 gcc/config/mips/mips-protos.h                        |  1 +
 gcc/config/mips/mips.c                               | 20 +++++++++++++++-----
 gcc/config/mips/mips.h                               |  7 +++++++
 .../gcc.target/mips/fuse-caller-save-micromips.c     | 17 +++++++++++++++++
 .../gcc.target/mips/fuse-caller-save-mip16.c         | 17 +++++++++++++++++
 gcc/testsuite/gcc.target/mips/fuse-caller-save.c     | 17 +++++++++++++++++
 gcc/testsuite/gcc.target/mips/fuse-caller-save.h     | 17 +++++++++++++++++
 gcc/testsuite/gcc.target/mips/mips.exp               |  1 +
 8 files changed, 92 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/mips/fuse-caller-save-micromips.c
 create mode 100644 gcc/testsuite/gcc.target/mips/fuse-caller-save-mip16.c
 create mode 100644 gcc/testsuite/gcc.target/mips/fuse-caller-save.c
 create mode 100644 gcc/testsuite/gcc.target/mips/fuse-caller-save.h

diff --git a/gcc/config/mips/mips-protos.h b/gcc/config/mips/mips-protos.h
index 3d59b7b..19ea919 100644
--- a/gcc/config/mips/mips-protos.h
+++ b/gcc/config/mips/mips-protos.h
@@ -232,6 +232,7 @@ extern bool mips_use_pic_fn_addr_reg_p (const_rtx);
 extern rtx mips_expand_call (enum mips_call_type, rtx, rtx, rtx, rtx, bool);
 extern void mips_split_call (rtx, rtx);
 extern bool mips_get_pic_call_symbol (rtx *, int);
+extern rtx mips_emit_call_insn (rtx, rtx, rtx, bool);
 extern void mips_expand_fcc_reload (rtx, rtx, rtx);
 extern void mips_set_return_address (rtx, rtx);
 extern bool mips_move_by_pieces_p (unsigned HOST_WIDE_INT, unsigned int);
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index 45256e9..b80c1b4 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -2814,7 +2814,7 @@ mips_force_temporary (rtx dest, rtx value)
    ADDR is the legitimized form, and LAZY_P is true if the call
    address is lazily-bound.  */
 
-static rtx
+rtx
 mips_emit_call_insn (rtx pattern, rtx orig_addr, rtx addr, bool lazy_p)
 {
   rtx insn, reg;
@@ -2843,6 +2843,15 @@ mips_emit_call_insn (rtx pattern, rtx orig_addr, rtx addr, bool lazy_p)
 	       gen_rtx_REG (Pmode, GOT_VERSION_REGNUM));
       emit_insn (gen_update_got_version ());
     }
+
+  if (TARGET_MIPS16
+      && TARGET_EXPLICIT_RELOCS
+      && TARGET_CALL_CLOBBERED_GP)
+    {
+      rtx post_call_tmp_reg = gen_rtx_REG (word_mode, POST_CALL_TMP_REG);
+      clobber_reg (&CALL_INSN_FUNCTION_USAGE (insn), post_call_tmp_reg);
+    }
+
   return insn;
 }
 \f
@@ -7099,10 +7108,8 @@ mips_split_call (rtx insn, rtx call_pattern)
 {
   emit_call_insn (call_pattern);
   if (!find_reg_note (insn, REG_NORETURN, 0))
-    /* Pick a temporary register that is suitable for both MIPS16 and
-       non-MIPS16 code.  $4 and $5 are used for returning complex double
-       values in soft-float code, so $6 is the first suitable candidate.  */
-    mips_restore_gp_from_cprestore_slot (gen_rtx_REG (Pmode, GP_ARG_FIRST + 2));
+    mips_restore_gp_from_cprestore_slot (gen_rtx_REG (Pmode,
+						      POST_CALL_TMP_REG));
 }
 
 /* Return true if a call to DECL may need to use JALX.  */
@@ -19134,6 +19141,9 @@ mips_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update)
 #undef TARGET_ATOMIC_ASSIGN_EXPAND_FENV
 #define TARGET_ATOMIC_ASSIGN_EXPAND_FENV mips_atomic_assign_expand_fenv
 
+#undef TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS
+#define TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS true
+
 struct gcc_target targetm = TARGET_INITIALIZER;
 \f
 #include "gt-mips.h"
diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h
index b25865b..14fa8fd 100644
--- a/gcc/config/mips/mips.h
+++ b/gcc/config/mips/mips.h
@@ -2212,6 +2212,13 @@ enum reg_class
 #define FP_ARG_FIRST (FP_REG_FIRST + 12)
 #define FP_ARG_LAST  (FP_ARG_FIRST + MAX_ARGS_IN_REGISTERS - 1)
 
+/* Temporary register that is used when restoring $gp after a call.  $4 and $5
+   are used for returning complex double values in soft-float code, so $6 is the
+   first suitable candidate for TARGET_MIPS16.  For !TARGET_MIPS16 we can use
+   $gp itself as the temporary.  */
+#define POST_CALL_TMP_REG \
+  (TARGET_MIPS16 ? GP_ARG_FIRST + 2 : PIC_OFFSET_TABLE_REGNUM)
+
 /* 1 if N is a possible register number for function argument passing.
    We have no FP argument registers when soft-float.  When FP registers
    are 32 bits, we can't directly reference the odd numbered ones.  */
diff --git a/gcc/testsuite/gcc.target/mips/fuse-caller-save-micromips.c b/gcc/testsuite/gcc.target/mips/fuse-caller-save-micromips.c
new file mode 100644
index 0000000..6ad01c7
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/fuse-caller-save-micromips.c
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-fuse-caller-save (-mmicromips)" } */
+/* At -O0 and -O1, the register allocator behaves more conservatively, and
+   the fuse-caller-save optimization doesnt' trigger.  */
+/* { dg-skip-if "" { *-*-* }  { "-O0" "-O1" } } */
+/* Testing -fuse-caller-save optimization option.  */
+
+#define ATTRIBUTE MICROMIPS
+#include "fuse-caller-save.h"
+
+/* Check that there are only 2 stack-saves: r31 in main and foo.  */
+
+/* Check that there only 2 sw/sd.  */
+/* { dg-final { scan-assembler-times "(?n)s\[wd\]\t\\\$.*,.*\\(\\\$sp\\)" 2 } } */
+
+/* Check that the first caller-save register is unused.  */
+/* { dg-final { scan-assembler-not "\\\$16" } } */
diff --git a/gcc/testsuite/gcc.target/mips/fuse-caller-save-mip16.c b/gcc/testsuite/gcc.target/mips/fuse-caller-save-mip16.c
new file mode 100644
index 0000000..a7c6cf4
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/fuse-caller-save-mip16.c
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-fuse-caller-save (-mips16)" } */
+/* At -O0 and -O1, the register allocator behaves more conservatively, and
+   the fuse-caller-save optimization doesnt' trigger.  */
+/* { dg-skip-if "" { *-*-* }  { "-O0" "-O1" } } */
+/* Testing -fuse-caller-save optimization option.  */
+
+#define ATTRIBUTE MIPS16
+#include "fuse-caller-save.h"
+
+/* Check that there are only 2 stack-saves: r31 in main and foo.  */
+
+/* Check that there only 2 sw/sd.  */
+/* { dg-final { scan-assembler-times "(?n)s\[wd\]\t\\\$.*,.*\\(\\\$sp\\)" 2 } } */
+
+/* Check that the first caller-save register is unused.  */
+/* { dg-final { scan-assembler-not "\\\$16" } } */
diff --git a/gcc/testsuite/gcc.target/mips/fuse-caller-save.c b/gcc/testsuite/gcc.target/mips/fuse-caller-save.c
new file mode 100644
index 0000000..72c08fe
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/fuse-caller-save.c
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-fuse-caller-save" } */
+/* At -O0 and -O1, the register allocator behaves more conservatively, and
+   the fuse-caller-save optimization doesnt' trigger.  */
+/* { dg-skip-if "" { *-*-* }  { "-O0" "-O1" } } */
+/* Testing -fuse-caller-save optimization option.  */
+
+#define ATTRIBUTE NOCOMPRESSION
+#include "fuse-caller-save.h"
+
+/* Check that there are only 2 stack-saves: r31 in main and foo.  */
+
+/* Check that there only 2 sw/sd.  */
+/* { dg-final { scan-assembler-times "(?n)s\[wd\]\t\\\$.*,.*\\(\\\$sp\\)" 2 } } */
+
+/* Check that the first caller-save register is unused.  */
+/* { dg-final { scan-assembler-not "\\\$16" } } */
diff --git a/gcc/testsuite/gcc.target/mips/fuse-caller-save.h b/gcc/testsuite/gcc.target/mips/fuse-caller-save.h
new file mode 100644
index 0000000..edf6039
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/fuse-caller-save.h
@@ -0,0 +1,17 @@
+static int __attribute__((noinline)) ATTRIBUTE
+bar (int x)
+{
+  return x + 3;
+}
+
+int __attribute__((noinline)) ATTRIBUTE
+foo (int y)
+{
+  return y + bar (y);
+}
+
+int ATTRIBUTE
+main (void)
+{
+  return !(foo (5) == 13);
+}
diff --git a/gcc/testsuite/gcc.target/mips/mips.exp b/gcc/testsuite/gcc.target/mips/mips.exp
index 8c72cff..6ad8160 100644
--- a/gcc/testsuite/gcc.target/mips/mips.exp
+++ b/gcc/testsuite/gcc.target/mips/mips.exp
@@ -305,6 +305,7 @@ foreach option {
     tree-vectorize
     unroll-all-loops
     unroll-loops
+    use-caller-save
 } {
     lappend mips_option_groups $option "-f(no-|)$option"
 }
-- 
1.8.3.2

Re: -fuse-caller-save - Enable for MIPS

[Richard Sandiford]

Tom de Vries <Tom_deVries@mentor.com> writes:
> On 27-04-14 12:27, Richard Sandiford wrote:
>> Tom de Vries <Tom_deVries@mentor.com> writes:
>>>   mips_emit_call_insn (rtx pattern, rtx orig_addr, rtx addr, bool lazy_p)
>>>   {
>>>     rtx insn, reg;
>>>
>>> -  insn = emit_call_insn (pattern);
>>> +  emit_call_insn (pattern);
>>> +  insn = last_call_insn ();
>>>
>>>     if (TARGET_MIPS16 && mips_use_pic_fn_addr_reg_p (orig_addr))
>>>       {
>>
>> This change isn't necessary; emit_call_insn is defined to return a CALL_INSN.
>>
>
> I dropped this change, as well as the change in the untyped_call expand, I 
> realized it's unnecessary.

Why was the untyped_call part unnecessary?

>> I'm a bit surprised that it doesn't work at -O1 for a simple test
>> like this though.  What goes wrong?
>>
>
> AFAIU now the problem is that the optimization doesn't trigger for -O0
> and -01, because the register allocator behaves more conservatively.

Hmm, is that just because -fcaller-saves is -O2 and above?  If so,
should -fuse-caller-save imply -fcaller-saves?

Thanks,
Richard

Re: -fuse-caller-save - Enable for MIPS

[Tom de Vries]

On 28-04-14 12:26, Richard Sandiford wrote:
> Tom de Vries <Tom_deVries@mentor.com> writes:
>> On 27-04-14 12:27, Richard Sandiford wrote:
>>> Tom de Vries <Tom_deVries@mentor.com> writes:
>>>>    mips_emit_call_insn (rtx pattern, rtx orig_addr, rtx addr, bool lazy_p)
>>>>    {
>>>>      rtx insn, reg;
>>>>
>>>> -  insn = emit_call_insn (pattern);
>>>> +  emit_call_insn (pattern);
>>>> +  insn = last_call_insn ();
>>>>
>>>>      if (TARGET_MIPS16 && mips_use_pic_fn_addr_reg_p (orig_addr))
>>>>        {
>>>
>>> This change isn't necessary; emit_call_insn is defined to return a CALL_INSN.
>>>
>>
>> I dropped this change, as well as the change in the untyped_call expand, I
>> realized it's unnecessary.
>
> Why was the untyped_call part unnecessary?
>

The define_expand "untyped_call" uses GEN_CALL, which uses define_expand "call", 
which uses mips_expand_call, which uses mips_emit_call_insn, which adds the 
required clobbers.

>>> I'm a bit surprised that it doesn't work at -O1 for a simple test
>>> like this though.  What goes wrong?
>>>
>>
>> AFAIU now the problem is that the optimization doesn't trigger for -O0
>> and -01, because the register allocator behaves more conservatively.
>
> Hmm, is that just because -fcaller-saves is -O2 and above?
>  If so,
> should -fuse-caller-save imply -fcaller-saves?
>
> Thanks,
> Richard
>

Re: -fuse-caller-save - Enable for MIPS

[Tom de Vries]

On 28-04-14 12:47, Tom de Vries wrote:
> Hmm, is that just because -fcaller-saves is -O2 and above?

For -O1, after adding -fcaller-saves the optimization triggers, and the 
test-cases passes.

For -O0, adding -fcaller-saves doesn't make a difference, the optimization 
doesn't trigger.

> If so,
> should -fuse-caller-save imply -fcaller-saves?

I don't think it's strictly necessary, but I can make a patch if required.

Thanks,
- Tom

Re: -fuse-caller-save - Enable for MIPS

[Richard Sandiford]

Tom de Vries <Tom_deVries@mentor.com> writes:
> On 28-04-14 12:26, Richard Sandiford wrote:
>> Tom de Vries <Tom_deVries@mentor.com> writes:
>>> On 27-04-14 12:27, Richard Sandiford wrote:
>>>> Tom de Vries <Tom_deVries@mentor.com> writes:
>>>>>    mips_emit_call_insn (rtx pattern, rtx orig_addr, rtx addr, bool lazy_p)
>>>>>    {
>>>>>      rtx insn, reg;
>>>>>
>>>>> -  insn = emit_call_insn (pattern);
>>>>> +  emit_call_insn (pattern);
>>>>> +  insn = last_call_insn ();
>>>>>
>>>>>      if (TARGET_MIPS16 && mips_use_pic_fn_addr_reg_p (orig_addr))
>>>>>        {
>>>>
>>>> This change isn't necessary; emit_call_insn is defined to return a
>>>> CALL_INSN.
>>>>
>>>
>>> I dropped this change, as well as the change in the untyped_call expand, I
>>> realized it's unnecessary.
>>
>> Why was the untyped_call part unnecessary?
>>
>
> The define_expand "untyped_call" uses GEN_CALL, which uses
> define_expand "call", which uses mips_expand_call, which uses
> mips_emit_call_insn, which adds the required clobbers.

Ah, yeah.  In that case please keep mips_emit_call_insn static.

OK with that change, although please remove -O1 if the agreement
is that "-O1 -fuse-call-save" should work.

Thanks,
Richard

Re: -fuse-caller-save - Enable for MIPS

[Richard Sandiford]

Tom de Vries <Tom_deVries@mentor.com> writes:
>> If so,
>> should -fuse-caller-save imply -fcaller-saves?
>
> I don't think it's strictly necessary, but I can make a patch if required.

Implying -fcaller-saves seems better to me, since "-O -fuse-caller-save"
looks like it should enable the new optimisation.  It's not my call though.

Thanks,
Richard

Re: Add post_expand_call_insn hook

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1287 bytes --]

On 24-04-14 17:13, Eric Botcazou wrote:
>> The hook is called right after expansion of calls, and allows a target to do
>> additional processing, such as f.i. adding clobbers to
>> CALL_INSN_FUNCTION_USAGE.
>>
>> Instead of using the hook, we could add code to the preparation statements
>> operand of the different call expands, but that requires those expands not
>> to use the rtl template, and generate all the rtl through c code. Which
>> requires a rewrite of the call expands in case of Aarch64.
>
> If Aarch64 is the only problematic back-end, then it should be changed to do
> what the other back-ends already do to use use_reg.  IMO adding such a bogus
> hook should be the very last resort solution.
>

Eric,

I've written this concept patch, which tries to address the same problem, but in 
a different (and I hope more generic) way.

It adds a post-emission C-code operand to define_expand.

As an example of how this could be useful, for the define_expand of call and 
call_value in the arm target, I'm using the new operand to do the post-emit call 
processing done currently in arm_emit_call_insn. This allows us to eliminate the 
call_internal and call_value_internal define_expands, and simplifies the call 
and call_value define_expands.

Any comments?

Thanks,
- Tom

[-- Attachment #2: Add-operand-for-post-emission-processing-to-define_expand.patch --]
[-- Type: text/x-patch, Size: 5572 bytes --]

diff --git a/gcc/config/arm/arm-protos.h b/gcc/config/arm/arm-protos.h
index 74645ee..506791a 100644
--- a/gcc/config/arm/arm-protos.h
+++ b/gcc/config/arm/arm-protos.h
@@ -126,7 +126,7 @@ extern int arm_const_double_inline_cost (rtx);
 extern bool arm_const_double_by_parts (rtx);
 extern bool arm_const_double_by_immediates (rtx);
 extern const char *fp_immediate_constant (rtx);
-extern void arm_emit_call_insn (rtx, rtx);
+extern void arm_post_emit_call_insn (rtx, rtx);
 extern const char *output_call (rtx *);
 extern const char *output_call_mem (rtx *);
 void arm_emit_movpair (rtx, rtx);
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 09b5c52..e36deac 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -17602,16 +17602,14 @@ vfp_emit_fstmd (int base_reg, int count)
   return count * 8;
 }
 
-/* Emit a call instruction with pattern PAT.  ADDR is the address of
-   the call target.  */
+/* Process a call instruction with pattern PAT after emission.  ADDR is the
+   address of the call target.  */
 
 void
-arm_emit_call_insn (rtx pat, rtx addr)
+arm_post_emit_call_insn (rtx pat, rtx addr)
 {
   rtx insn;
 
-  insn = emit_call_insn (pat);
-
   /* The PIC register is live on entry to VxWorks PIC PLT entries.
      If the call might use such an entry, add a use of the PIC register
      to the instruction's CALL_INSN_FUNCTION_USAGE.  */
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 8a949b9..45019ae 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -9082,7 +9082,7 @@
   "TARGET_EITHER"
   "
   {
-    rtx callee, pat;
+    rtx callee;
     
     /* In an untyped call, we can get NULL for operand 2.  */
     if (operands[2] == NULL_RTX)
@@ -9097,18 +9097,13 @@
 	: !REG_P (callee))
       XEXP (operands[0], 0) = force_reg (Pmode, callee);
 
-    pat = gen_call_internal (operands[0], operands[1], operands[2]);
-    arm_emit_call_insn (pat, XEXP (operands[0], 0));
-    DONE;
+  }"
+  []
+  "{
+    arm_post_emit_call_insn (_val, XEXP (operands[0], 0));
   }"
 )
 
-(define_expand "call_internal"
-  [(parallel [(call (match_operand 0 "memory_operand" "")
-	            (match_operand 1 "general_operand" ""))
-	      (use (match_operand 2 "" ""))
-	      (clobber (reg:SI LR_REGNUM))])])
-
 (define_insn "*call_reg_armv5"
   [(call (mem:SI (match_operand:SI 0 "s_register_operand" "r"))
          (match_operand 1 "" ""))
@@ -9191,7 +9186,7 @@
   "TARGET_EITHER"
   "
   {
-    rtx pat, callee;
+    rtx callee;
     
     /* In an untyped call, we can get NULL for operand 2.  */
     if (operands[3] == 0)
@@ -9205,21 +9200,13 @@
 	? arm_is_long_call_p (SYMBOL_REF_DECL (callee))
 	: !REG_P (callee))
       XEXP (operands[1], 0) = force_reg (Pmode, callee);
-
-    pat = gen_call_value_internal (operands[0], operands[1],
-				   operands[2], operands[3]);
-    arm_emit_call_insn (pat, XEXP (operands[1], 0));
-    DONE;
+  }"
+  []
+  "{
+    arm_post_emit_call_insn (_val, XEXP (operands[1], 0));
   }"
 )
 
-(define_expand "call_value_internal"
-  [(parallel [(set (match_operand       0 "" "")
-	           (call (match_operand 1 "memory_operand" "")
-		         (match_operand 2 "general_operand" "")))
-	      (use (match_operand 3 "" ""))
-	      (clobber (reg:SI LR_REGNUM))])])
-
 (define_insn "*call_value_reg_armv5"
   [(set (match_operand 0 "" "")
         (call (mem:SI (match_operand:SI 1 "s_register_operand" "r"))
diff --git a/gcc/genemit.c b/gcc/genemit.c
index faaa610..aff27f6 100644
--- a/gcc/genemit.c
+++ b/gcc/genemit.c
@@ -422,7 +422,8 @@ gen_expand (rtx expand)
   /* If we don't have any C code to write, only one insn is being written,
      and no MATCH_DUPs are present, we can just return the desired insn
      like we do for a DEFINE_INSN.  This saves memory.  */
-  if ((XSTR (expand, 3) == 0 || *XSTR (expand, 3) == '\0')
+  if (((XSTR (expand, 3) == 0 || *XSTR (expand, 3) == '\0'))
+      && (XSTR (expand, 5) == 0 || *XSTR (expand, 5) == '\0')
       && stats.max_opno >= stats.max_dup_opno
       && XVECLEN (expand, 1) == 1)
     {
@@ -525,6 +526,23 @@ gen_expand (rtx expand)
 
   printf ("  _val = get_insns ();\n");
   printf ("  end_sequence ();\n");
+
+  if (XSTR (expand, 5) && *XSTR (expand, 5))
+    {
+      printf ("  {\n");
+      if (stats.num_operand_vars > 0)
+	printf ("    rtx operands[%d];\n", stats.num_operand_vars);
+
+      /* Output code to copy the arguments into `operands'.  */
+      for (i = 0; i < stats.num_generator_args; i++)
+	printf ("    operands[%d] = operand%d;\n", i, i);
+
+      print_md_ptr_loc (XSTR (expand, 5));
+      printf ("%s\n", XSTR (expand, 5));
+
+      printf ("  }\n");
+    }
+
   printf ("  return _val;\n}\n\n");
 }
 
diff --git a/gcc/rtl.def b/gcc/rtl.def
index 56418c7..655f752 100644
--- a/gcc/rtl.def
+++ b/gcc/rtl.def
@@ -933,8 +933,10 @@ DEF_RTL_EXPR(DEFINE_PEEPHOLE2, "define_peephole2", "EsES", RTX_EXTRA)
 	elements of `recog_data.operand' for use by the vector of
 	insn-patterns.
 	(`operands' is an alias here for `recog_data.operand').
-   5th: optionally, a vector of attributes for this expand.  */
-DEF_RTL_EXPR(DEFINE_EXPAND, "define_expand", "sEssV", RTX_EXTRA)
+   5th: optionally, a vector of attributes for this expand.
+   6th operand: Extra C code to execute after generating the insns.  This code
+        is not executed if DONE (or FAIL) is used in the 4th operand.  */
+DEF_RTL_EXPR(DEFINE_EXPAND, "define_expand", "sEssVs", RTX_EXTRA)
 
 /* Define a requirement for delay slots.
    1st operand: Condition involving insn attributes that, if true,
-- 
1.8.3.2

Re: Add post_expand_call_insn hook

[Richard Henderson]

On 04/29/2014 01:59 AM, Tom de Vries wrote:
> On 24-04-14 17:13, Eric Botcazou wrote:
>>> The hook is called right after expansion of calls, and allows a target to do
>>> additional processing, such as f.i. adding clobbers to
>>> CALL_INSN_FUNCTION_USAGE.
>>>
>>> Instead of using the hook, we could add code to the preparation statements
>>> operand of the different call expands, but that requires those expands not
>>> to use the rtl template, and generate all the rtl through c code. Which
>>> requires a rewrite of the call expands in case of Aarch64.
>>
>> If Aarch64 is the only problematic back-end, then it should be changed to do
>> what the other back-ends already do to use use_reg.  IMO adding such a bogus
>> hook should be the very last resort solution.
>>
> 
> Eric,
> 
> I've written this concept patch, which tries to address the same problem, but
> in a different (and I hope more generic) way.
> 
> It adds a post-emission C-code operand to define_expand.
> 
> As an example of how this could be useful, for the define_expand of call and
> call_value in the arm target, I'm using the new operand to do the post-emit
> call processing done currently in arm_emit_call_insn. This allows us to
> eliminate the call_internal and call_value_internal define_expands, and
> simplifies the call and call_value define_expands.


Is this patch really any better?  I can't see that it is myself.  It seems to
me that the existing mechanism to emit the call, then append to FUNCTION_USAGE
is perfectly clear.  This new argument to define_expand seems less clear.

What are you trying to fix, anyway?


r~

Re: Add post_expand_call_insn hook

[Tom de Vries]

On 29-04-14 20:56, Richard Henderson wrote:
>> I've written this concept patch, which tries to address the same problem, but
>> >in a different (and I hope more generic) way.
>> >
>> >It adds a post-emission C-code operand to define_expand.
>> >
>> >As an example of how this could be useful, for the define_expand of call and
>> >call_value in the arm target, I'm using the new operand to do the post-emit
>> >call processing done currently in arm_emit_call_insn. This allows us to
>> >eliminate the call_internal and call_value_internal define_expands, and
>> >simplifies the call and call_value define_expands.
>
> Is this patch really any better?  I can't see that it is myself.  It seems to
> me that the existing mechanism to emit the call, then append to FUNCTION_USAGE
> is perfectly clear.  This new argument to define_expand seems less clear.
>
> What are you trying to fix, anyway?

Richard,

In arm.md, the define_expand "call" rtl template is not used, because DONE is 
used in the preparation statements operand. The DONE is there, because we need a 
handle to the emitted insn to do post-emit processing.

The post-emission C-code operand that this patch introduces provides a handle to 
the emitted insn, which means we no longer need to use an explicit emit and DONE 
to get that handle.  And without that DONE, we can use the rtl template of 
define_expand "call", and no longer need the call_internal.

So we eliminate an define_expand (which is shorter, and removes duplicate code), 
and deal with expansion inside a single define_expand (which is clearer). To me, 
those are the benefits.

I agree the existing mechanism is perfectly clear.

Still I think that a 'finalization statements' operand is as clear as a 
'preparation statements' operand.

Thanks,
- Tom

Re: Add call_fusage_contains_non_callee_clobbers hook

[Vladimir Makarov]

On 2014-04-24, 3:12 AM, Tom de Vries wrote:
> On 23-04-14 17:10, Richard Sandiford wrote:
>> FWIW I think this should be a plain bool rather than a function,
>> like delay_sched2 etc.
>>
>
> Vladimir,
>
> I've reimplemented the hook using DEFHOOKPOD instead of DEFHOOK, to make
> it a plain bool.
>
> OK for trunk?
>

Yes, it is ok if it was not approved yet and sorry for the delay with 
the answer.

> 2013-04-29  Radovan Obradovic  <robradovic@mips.com>
>              Tom de Vries  <tom@codesourcery.com>
>
>          * target.def (call_fusage_contains_non_callee_clobbers): New
> DEFHOOKPOD.
>          * doc/tm.texi.in (@node Stack and Calling): Add Miscellaneous
> Register
>          Hooks to @menu.
>          (@node Miscellaneous Register Hooks): New node.
>          (@hook TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS): New hook.
>          * doc/tm.texi: Regenerate.
>
>

[PING] -fuse-caller-save - Collect register usage information

[Tom de Vries]

On 26-04-14 14:51, Tom de Vries wrote:
> Eric,
> Honza,
>
> This patch adds analysis in pass_final to track which hard registers are set or
> clobbered by the function body, and stores that information in a
> struct cgraph_node, to be used in the fuse-caller-save optmization.
>
> This is the updated version of the previously approved patch
> submitted here (http://gcc.gnu.org/ml/gcc-patches/2013-03/msg01320.html ).
> The changes are:
> - using a new hook call_fusage_contains_non_callee_clobbers,
> - incorporating minor review comments from Richard Sandiford
>    ( http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01436.html ).
>
> As part of the fuse-caller-save patch series, bootstrapped and reg-tested on
> x86_64, and build and reg-tested on MIPS.
>
> Eric, non-cgraph part OK for trunk?
>
> Honza, cgraph part OK for trunk?
>

Ping. If this patch is approved and committed, I can commit the other approved 
fuse-caller-save patches and enable the optimization for MIPS.

Thanks,
- Tom

Re: -fuse-caller-save - Collect register usage information

[Eric Botcazou]

> This is the updated version of the previously approved patch
> submitted here (http://gcc.gnu.org/ml/gcc-patches/2013-03/msg01320.html ).
> The changes are:
> - using a new hook call_fusage_contains_non_callee_clobbers,
> - incorporating minor review comments from Richard Sandiford
>    ( http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01436.html ).
> 
> As part of the fuse-caller-save patch series, bootstrapped and reg-tested on
> x86_64, and build and reg-tested on MIPS.
> 
> Eric, non-cgraph part OK for trunk?

I think we should consider creating a new rule: for every target hook added, 
another must be first removed...

So this call_fusage_contains_non_callee_clobbers is essentially only a stop 
gap measure for the ports that haven't been changed yet?  If so, please add a 
??? comment at the beginning of collect_fn_hard_reg_usage:

  /* ??? To be removed when all the ports have been fixed.  */
  if (!targetm.call_fusage_contains_non_callee_clobbers)

and invoke collect_fn_hard_reg_usage from rest_of_handle_final only when 
flag_use_caller_save is true.

Why do you need to retest them in get_call_reg_set_usage and get_call_fndecl?

-- 
Eric Botcazou

Re: -fuse-caller-save - Collect register usage information

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1739 bytes --]

On 17-05-14 12:51, Eric Botcazou wrote:
>> This is the updated version of the previously approved patch
>> submitted here (http://gcc.gnu.org/ml/gcc-patches/2013-03/msg01320.html ).
>> The changes are:
>> - using a new hook call_fusage_contains_non_callee_clobbers,
>> - incorporating minor review comments from Richard Sandiford
>>     ( http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01436.html ).
>>
>> As part of the fuse-caller-save patch series, bootstrapped and reg-tested on
>> x86_64, and build and reg-tested on MIPS.
>>
>> Eric, non-cgraph part OK for trunk?
>

Eric,

thanks for the review.

> I think we should consider creating a new rule: for every target hook added,
> another must be first removed...
>
> So this call_fusage_contains_non_callee_clobbers is essentially only a stop
> gap measure for the ports that haven't been changed yet?

I think so.

 > If so, please add a
> ??? comment at the beginning of collect_fn_hard_reg_usage:
>
>    /* ??? To be removed when all the ports have been fixed.  */
>    if (!targetm.call_fusage_contains_non_callee_clobbers)
>

Done.

> and invoke collect_fn_hard_reg_usage from rest_of_handle_final only when
> flag_use_caller_save is true.
>

Done.

> Why do you need to retest them in get_call_reg_set_usage and get_call_fndecl?
>

The test for flag_use_caller-save in get_call_fndecl was unnecessary, I've 
removed it.

The test in get_call_reg_set_usage for flag_use_caller_save and the hook is 
strictly speaking not necessary. But it's the interface function to retrieve the 
collected register usage information, so it seems a good location to do an 
early-out. I've left it in for now.

Bootstrapped and reg-tested on x86_64.

non-cgraph part OK for trunk?

Thanks,
  - Tom

[-- Attachment #2: 0001-fuse-caller-save-Collect-register-usage-information.patch --]
[-- Type: text/x-patch, Size: 5793 bytes --]

2014-05-19  Radovan Obradovic  <robradovic@mips.com>
            Tom de Vries  <tom@codesourcery.com>

	* cgraph.h (struct cgraph_node): Add function_used_regs,
	function_used_regs_initialized and function_used_regs_valid fields.
	* final.c: Move include of hard-reg-set.h to before rtl.h to declare
	find_all_hard_reg_sets.
	(collect_fn_hard_reg_usage, get_call_fndecl, get_call_cgraph_node)
	(get_call_reg_set_usage): New function.
	(rest_of_handle_final): Use collect_fn_hard_reg_usage.
---
 gcc/cgraph.h |   7 ++++
 gcc/final.c  | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 gcc/regs.h   |   4 +++
 3 files changed, 125 insertions(+), 1 deletion(-)

diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index 84fc1d9..bbbbd1f 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -408,6 +408,13 @@ public:
   /* Time profiler: first run of function.  */
   int tp_first_run;
 
+  /* Call unsaved hard registers really used by the corresponding
+     function (including ones used by functions called by the
+     function).  */
+  HARD_REG_SET function_used_regs;
+  /* Set if function_used_regs is valid.  */
+  unsigned function_used_regs_valid: 1;
+
   /* Set when decl is an abstract function pointed to by the
      ABSTRACT_DECL_ORIGIN of a reachable function.  */
   unsigned used_as_abstract_origin : 1;
diff --git a/gcc/final.c b/gcc/final.c
index 3271430..e747b80 100644
--- a/gcc/final.c
+++ b/gcc/final.c
@@ -49,6 +49,7 @@ along with GCC; see the file COPYING3.  If not see
 
 #include "tree.h"
 #include "varasm.h"
+#include "hard-reg-set.h"
 #include "rtl.h"
 #include "tm_p.h"
 #include "regs.h"
@@ -57,7 +58,6 @@ along with GCC; see the file COPYING3.  If not see
 #include "recog.h"
 #include "conditions.h"
 #include "flags.h"
-#include "hard-reg-set.h"
 #include "output.h"
 #include "except.h"
 #include "function.h"
@@ -224,6 +224,7 @@ static int alter_cond (rtx);
 static int final_addr_vec_align (rtx);
 #endif
 static int align_fuzz (rtx, rtx, int, unsigned);
+static void collect_fn_hard_reg_usage (void);
 \f
 /* Initialize data in final at the beginning of a compilation.  */
 
@@ -4442,6 +4443,8 @@ rest_of_handle_final (void)
   assemble_start_function (current_function_decl, fnname);
   final_start_function (get_insns (), asm_out_file, optimize);
   final (get_insns (), asm_out_file, optimize);
+  if (flag_use_caller_save)
+    collect_fn_hard_reg_usage ();
   final_end_function ();
 
   /* The IA-64 ".handlerdata" directive must be issued before the ".endp"
@@ -4740,3 +4743,113 @@ make_pass_clean_state (gcc::context *ctxt)
 {
   return new pass_clean_state (ctxt);
 }
+
+/* Collect hard register usage for the current function.  */
+
+static void
+collect_fn_hard_reg_usage (void)
+{
+  rtx insn;
+  int i;
+  struct cgraph_node *node;
+
+  /* ??? To be removed when all the ports have been fixed.  */
+  if (!targetm.call_fusage_contains_non_callee_clobbers)
+    return;
+
+  node = cgraph_get_node (current_function_decl);
+  gcc_assert (node != NULL);
+
+  for (insn = get_insns (); insn != NULL_RTX; insn = next_insn (insn))
+    {
+      HARD_REG_SET insn_used_regs;
+
+      if (!NONDEBUG_INSN_P (insn))
+	continue;
+
+      find_all_hard_reg_sets (insn, &insn_used_regs, false);
+
+      if (CALL_P (insn)
+	  && !get_call_reg_set_usage (insn, &insn_used_regs, call_used_reg_set))
+	{
+	  CLEAR_HARD_REG_SET (node->function_used_regs);
+	  return;
+	}
+
+      IOR_HARD_REG_SET (node->function_used_regs, insn_used_regs);
+    }
+
+  /* Be conservative - mark fixed and global registers as used.  */
+  IOR_HARD_REG_SET (node->function_used_regs, fixed_reg_set);
+  for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
+    if (global_regs[i])
+      SET_HARD_REG_BIT (node->function_used_regs, i);
+
+#ifdef STACK_REGS
+  /* Handle STACK_REGS conservatively, since the df-framework does not
+     provide accurate information for them.  */
+
+  for (i = FIRST_STACK_REG; i <= LAST_STACK_REG; i++)
+    SET_HARD_REG_BIT (node->function_used_regs, i);
+#endif
+
+  node->function_used_regs_valid = 1;
+}
+
+/* Get the declaration of the function called by INSN.  */
+
+static tree
+get_call_fndecl (rtx insn)
+{
+  rtx note, datum;
+
+  note = find_reg_note (insn, REG_CALL_DECL, NULL_RTX);
+  if (note == NULL_RTX)
+    return NULL_TREE;
+
+  datum = XEXP (note, 0);
+  if (datum != NULL_RTX)
+    return SYMBOL_REF_DECL (datum);
+
+  return NULL_TREE;
+}
+
+static struct cgraph_node *
+get_call_cgraph_node (rtx insn)
+{
+  tree fndecl;
+
+  if (insn == NULL_RTX)
+    return NULL;
+
+  fndecl = get_call_fndecl (insn);
+  if (fndecl == NULL_TREE
+      || !targetm.binds_local_p (fndecl))
+    return NULL;
+
+  return cgraph_get_node (fndecl);
+}
+
+/* Find hard registers used by function call instruction INSN, and return them
+   in REG_SET.  Return DEFAULT_SET in REG_SET if not found.  */
+
+bool
+get_call_reg_set_usage (rtx insn, HARD_REG_SET *reg_set,
+			HARD_REG_SET default_set)
+{
+  if (flag_use_caller_save
+      && targetm.call_fusage_contains_non_callee_clobbers)
+    {
+      struct cgraph_node *node = get_call_cgraph_node (insn);
+      if (node != NULL
+	  && node->function_used_regs_valid)
+	{
+	  COPY_HARD_REG_SET (*reg_set, node->function_used_regs);
+	  AND_HARD_REG_SET (*reg_set, default_set);
+	  return true;
+	}
+    }
+
+  COPY_HARD_REG_SET (*reg_set, default_set);
+  return false;
+}
diff --git a/gcc/regs.h b/gcc/regs.h
index 006caca..44cc005 100644
--- a/gcc/regs.h
+++ b/gcc/regs.h
@@ -421,4 +421,8 @@ range_in_hard_reg_set_p (const HARD_REG_SET set, unsigned regno, int nregs)
   return true;
 }
 
+/* Get registers used by given function call instruction.  */
+extern bool get_call_reg_set_usage (rtx insn, HARD_REG_SET *reg_set,
+				    HARD_REG_SET default_set);
+
 #endif /* GCC_REGS_H */
-- 
1.9.1

Re: -fuse-caller-save - Collect register usage information

[Eric Botcazou]

> The test in get_call_reg_set_usage for flag_use_caller_save and the hook is
> strictly speaking not necessary. But it's the interface function to retrieve
> the collected register usage information, so it seems a good location to do
> an early-out. I've left it in for now.

But the test for targetm.call_fusage_contains_non_callee_clobbers seems to be 
useless in practice if -fuse-caller-save isn't the default, so I'd remove it.

OK with that change and a head comment for get_call_cgraph_node, unless Jan 
wants to move get_call_fndecl and get_call_cgraph_node to cgraph.c.

-- 
Eric Botcazou

Re: -fuse-caller-save - Collect register usage information

[Jan Hubicka]

> 2013-04-29  Radovan Obradovic  <robradovic@mips.com>
>             Tom de Vries  <tom@codesourcery.com>
> 
> 	* cgraph.h (struct cgraph_node): Add function_used_regs,
> 	function_used_regs_initialized and function_used_regs_valid fields.
> 	* final.c: Move include of hard-reg-set.h to before rtl.h to declare
> 	find_all_hard_reg_sets.
> 	(collect_fn_hard_reg_usage, get_call_fndecl, get_call_cgraph_node)
> 	(get_call_reg_set_usage): New function.
> 	(rest_of_handle_final): Use collect_fn_hard_reg_usage.
> ---
>  gcc/cgraph.h |   7 ++++
>  gcc/final.c  | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  gcc/regs.h   |   4 ++
>  3 files changed, 127 insertions(+), 1 deletion(-)
> 
> diff --git a/gcc/cgraph.h b/gcc/cgraph.h
> index 84fc1d9..bbbbd1f 100644
> --- a/gcc/cgraph.h
> +++ b/gcc/cgraph.h
> @@ -408,6 +408,13 @@ public:
>    /* Time profiler: first run of function.  */
>    int tp_first_run;
>  
> +  /* Call unsaved hard registers really used by the corresponding
> +     function (including ones used by functions called by the
> +     function).  */
> +  HARD_REG_SET function_used_regs;
> +  /* Set if function_used_regs is valid.  */
> +  unsigned function_used_regs_valid: 1;
> +

This approach seems resonable, but please move the data into cgraph_rtl_info
substructure and access it via cgraph_rtl_info.  (I have plan to move it off
cgraph_node, so it doesn't consume memory at WPA stage)

The cgraph bits are OK with this change.

I am sorry for late reply,
Honza

Re: -fuse-caller-save - Collect register usage information

[Jan Hubicka]

> +/* Get the declaration of the function called by INSN.  */
> +
> +static tree
> +get_call_fndecl (rtx insn)
> +{
> +  rtx note, datum;
> +
> +  note = find_reg_note (insn, REG_CALL_DECL, NULL_RTX);
> +  if (note == NULL_RTX)
> +    return NULL_TREE;
> +
> +  datum = XEXP (note, 0);
> +  if (datum != NULL_RTX)
> +    return SYMBOL_REF_DECL (datum);
> +
> +  return NULL_TREE;
> +}
> +
> +static struct cgraph_node *
> +get_call_cgraph_node (rtx insn)
> +{
> +  tree fndecl;
> +
> +  if (insn == NULL_RTX)
> +    return NULL;
> +
> +  fndecl = get_call_fndecl (insn);
> +  if (fndecl == NULL_TREE
> +      || !targetm.binds_local_p (fndecl))
> +    return NULL;
> +
> +  return cgraph_get_node (fndecl);
> +}

So far we do not have any RTL code in cgraph*.c, so lets keep it here until we
get some other uses for it.  Then I think it can go to rtlanal.

get_call_cgraph_node is missing description and probably should emhatize the
fact that it returns NULL for call targets that can be overwritten.
You want to test decl_binds_to_current_def_p instead of binds_local_p.
Consider hidden weak symbol - that one binds local, but it can be rewritten by
other implementation at linktime.

With this change and after the data are moved to rtl info, the patch is OK (given that 
Eric already approved the RTL bits I can't)

Note that this patch will interfere rather badly with Martin's work to order functions
in execution order, since it wants to have callee before caller.  I wonder if we can't
add support for GAS section fragments in this case to order functions in a way we want.

Honza

Re: -fuse-caller-save - Collect register usage information

[Bill Schmidt]

Tom, the final version of this patch that you committed breaks bootstrap
on powerpc64le-linux-gnu.  The problem is that all uses of the variable
i are guarded by #ifdef STACK_REGS, but the declaration of i is
unconditional.  We get an unused variable warning that becomes an error
during stage 3.

Thanks,
Bill

On Mon, 2014-05-19 at 16:30 +0200, Tom de Vries wrote:
> On 17-05-14 12:51, Eric Botcazou wrote:
> >> This is the updated version of the previously approved patch
> >> submitted here (http://gcc.gnu.org/ml/gcc-patches/2013-03/msg01320.html ).
> >> The changes are:
> >> - using a new hook call_fusage_contains_non_callee_clobbers,
> >> - incorporating minor review comments from Richard Sandiford
> >>     ( http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01436.html ).
> >>
> >> As part of the fuse-caller-save patch series, bootstrapped and reg-tested on
> >> x86_64, and build and reg-tested on MIPS.
> >>
> >> Eric, non-cgraph part OK for trunk?
> >
> 
> Eric,
> 
> thanks for the review.
> 
> > I think we should consider creating a new rule: for every target hook added,
> > another must be first removed...
> >
> > So this call_fusage_contains_non_callee_clobbers is essentially only a stop
> > gap measure for the ports that haven't been changed yet?
> 
> I think so.
> 
>  > If so, please add a
> > ??? comment at the beginning of collect_fn_hard_reg_usage:
> >
> >    /* ??? To be removed when all the ports have been fixed.  */
> >    if (!targetm.call_fusage_contains_non_callee_clobbers)
> >
> 
> Done.
> 
> > and invoke collect_fn_hard_reg_usage from rest_of_handle_final only when
> > flag_use_caller_save is true.
> >
> 
> Done.
> 
> > Why do you need to retest them in get_call_reg_set_usage and get_call_fndecl?
> >
> 
> The test for flag_use_caller-save in get_call_fndecl was unnecessary, I've 
> removed it.
> 
> The test in get_call_reg_set_usage for flag_use_caller_save and the hook is 
> strictly speaking not necessary. But it's the interface function to retrieve the 
> collected register usage information, so it seems a good location to do an 
> early-out. I've left it in for now.
> 
> Bootstrapped and reg-tested on x86_64.
> 
> non-cgraph part OK for trunk?
> 
> Thanks,
>   - Tom

Re: -fuse-caller-save - Collect register usage information

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 463 bytes --]

On 29-05-14 00:42, Bill Schmidt wrote:
> Tom, the final version of this patch that you committed breaks bootstrap
> on powerpc64le-linux-gnu.  The problem is that all uses of the variable
> i are guarded by #ifdef STACK_REGS, but the declaration of i is
> unconditional.  We get an unused variable warning that becomes an error
> during stage 3.
>

Bill,

thanks for letting me know.

I've bootstrapped attached patch on x86_64, and committed it.

Thanks,
- Tom


[-- Attachment #2: 0001-Fix-bootstrap-on-powerpc64le-linux-gnu.patch --]
[-- Type: text/x-patch, Size: 468 bytes --]

2014-05-29  Tom de Vries  <tom@codesourcery.com>

	* final.c (collect_fn_hard_reg_usage): Guard variable declaration
	with #ifdef STACK_REGS.

diff --git a/gcc/final.c b/gcc/final.c
index a345fe7..c32e177 100644
--- a/gcc/final.c
+++ b/gcc/final.c
@@ -4750,7 +4750,9 @@ static void
 collect_fn_hard_reg_usage (void)
 {
   rtx insn;
+#ifdef STACK_REGS
   int i;
+#endif
   struct cgraph_rtl_info *node;
 
   /* ??? To be removed when all the ports have been fixed.  */

Re: -fuse-caller-save - Collect register usage information

[Richard Henderson]

On 05/19/2014 07:30 AM, Tom de Vries wrote:
> +  for (insn = get_insns (); insn != NULL_RTX; insn = next_insn (insn))
> +    {
> +      HARD_REG_SET insn_used_regs;
> +
> +      if (!NONDEBUG_INSN_P (insn))
> +	continue;
> +
> +      find_all_hard_reg_sets (insn, &insn_used_regs, false);
> +
> +      if (CALL_P (insn)
> +	  && !get_call_reg_set_usage (insn, &insn_used_regs, call_used_reg_set))
> +	{
> +	  CLEAR_HARD_REG_SET (node->function_used_regs);
> +	  return;
> +	}
> +
> +      IOR_HARD_REG_SET (node->function_used_regs, insn_used_regs);
> +    }

As an aside, wouldn't it work out better if we collect into a local variable
instead of writing to memory here in node->function_used_regs each time?  But
not the main point...

Let's suppose that we've got a rather large function, with only local calls for
which we can acquire usage.  Let's suppose that even one of those callees
further calls something else, such that insn_used_regs == call_used_reg_set.

We fill node->function_used_regs immediately, but keep scanning the rest of the
large function.


> +
> +  /* Be conservative - mark fixed and global registers as used.  */
> +  IOR_HARD_REG_SET (node->function_used_regs, fixed_reg_set);
> +  for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
> +    if (global_regs[i])
> +      SET_HARD_REG_BIT (node->function_used_regs, i);
> +
> +#ifdef STACK_REGS
> +  /* Handle STACK_REGS conservatively, since the df-framework does not
> +     provide accurate information for them.  */
> +
> +  for (i = FIRST_STACK_REG; i <= LAST_STACK_REG; i++)
> +    SET_HARD_REG_BIT (node->function_used_regs, i);
> +#endif
> +
> +  node->function_used_regs_valid = 1;

Wouldn't it be better to compare the collected function_used_regs; if it
contains all of call_used_reg_set, decline to set function_used_regs_valid.
That way, we'll early exit from the above loop whenever we see that we can't
improve over the default call-clobber set.

Although perhaps function_used_regs_valid is no longer the best name in that
case...


r~

Re: -fuse-caller-save - Collect register usage information

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 921 bytes --]

On 19-06-14 07:13, Richard Henderson wrote:
> On 05/19/2014 07:30 AM, Tom de Vries wrote:
>> >+  for (insn = get_insns (); insn != NULL_RTX; insn = next_insn (insn))
>> >+    {
>> >+      HARD_REG_SET insn_used_regs;
>> >+
>> >+      if (!NONDEBUG_INSN_P (insn))
>> >+	continue;
>> >+
>> >+      find_all_hard_reg_sets (insn, &insn_used_regs, false);
>> >+
>> >+      if (CALL_P (insn)
>> >+	  && !get_call_reg_set_usage (insn, &insn_used_regs, call_used_reg_set))
>> >+	{
>> >+	  CLEAR_HARD_REG_SET (node->function_used_regs);
>> >+	  return;
>> >+	}
>> >+
>> >+      IOR_HARD_REG_SET (node->function_used_regs, insn_used_regs);
>> >+    }
> As an aside, wouldn't it work out better if we collect into a local variable
> instead of writing to memory here in node->function_used_regs each time?

Richard,

Agreed. This patch implements that. I'll bootstrap and reg-test on x86_64 and 
commit as obvious.

Thanks,
- Tom



[-- Attachment #2: 0006-Use-function_used_regs-variable-in-collect_fn_hard_r.patch --]
[-- Type: text/x-patch, Size: 1813 bytes --]

2014-06-19  Tom de Vries  <tom@codesourcery.com>

	* final.c (collect_fn_hard_reg_usage): Add and use variable
	function_used_regs.

diff --git a/gcc/final.c b/gcc/final.c
index 4f08073..e39930d 100644
--- a/gcc/final.c
+++ b/gcc/final.c
@@ -4760,13 +4760,13 @@ collect_fn_hard_reg_usage (void)
   int i;
 #endif
   struct cgraph_rtl_info *node;
+  HARD_REG_SET function_used_regs;
 
   /* ??? To be removed when all the ports have been fixed.  */
   if (!targetm.call_fusage_contains_non_callee_clobbers)
     return;
 
-  node = cgraph_rtl_info (current_function_decl);
-  gcc_assert (node != NULL);
+  CLEAR_HARD_REG_SET (function_used_regs);
 
   for (insn = get_insns (); insn != NULL_RTX; insn = next_insn (insn))
     {
@@ -4779,25 +4779,26 @@ collect_fn_hard_reg_usage (void)
 
       if (CALL_P (insn)
 	  && !get_call_reg_set_usage (insn, &insn_used_regs, call_used_reg_set))
-	{
-	  CLEAR_HARD_REG_SET (node->function_used_regs);
-	  return;
-	}
+	return;
 
-      IOR_HARD_REG_SET (node->function_used_regs, insn_used_regs);
+      IOR_HARD_REG_SET (function_used_regs, insn_used_regs);
     }
 
   /* Be conservative - mark fixed and global registers as used.  */
-  IOR_HARD_REG_SET (node->function_used_regs, fixed_reg_set);
+  IOR_HARD_REG_SET (function_used_regs, fixed_reg_set);
 
 #ifdef STACK_REGS
   /* Handle STACK_REGS conservatively, since the df-framework does not
      provide accurate information for them.  */
 
   for (i = FIRST_STACK_REG; i <= LAST_STACK_REG; i++)
-    SET_HARD_REG_BIT (node->function_used_regs, i);
+    SET_HARD_REG_BIT (function_used_regs, i);
 #endif
 
+  node = cgraph_rtl_info (current_function_decl);
+  gcc_assert (node != NULL);
+
+  COPY_HARD_REG_SET (node->function_used_regs, function_used_regs);
   node->function_used_regs_valid = 1;
 }
 
-- 
1.9.1

Re: -fuse-caller-save - Collect register usage information

[Richard Henderson]

On 06/19/2014 05:39 AM, Tom de Vries wrote:
> 
> 2014-06-19  Tom de Vries  <tom@codesourcery.com>
> 
> 	* final.c (collect_fn_hard_reg_usage): Add and use variable
> 	function_used_regs.

Looks good, thanks.


r~

Re: -fuse-caller-save - Collect register usage information

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 2316 bytes --]

On 19-06-14 07:13, Richard Henderson wrote:
> On 05/19/2014 07:30 AM, Tom de Vries wrote:
>> +  for (insn = get_insns (); insn != NULL_RTX; insn = next_insn (insn))
>> +    {
>> +      HARD_REG_SET insn_used_regs;
>> +
>> +      if (!NONDEBUG_INSN_P (insn))
>> +	continue;
>> +
>> +      find_all_hard_reg_sets (insn, &insn_used_regs, false);
>> +
>> +      if (CALL_P (insn)
>> +	  && !get_call_reg_set_usage (insn, &insn_used_regs, call_used_reg_set))
>> +	{
>> +	  CLEAR_HARD_REG_SET (node->function_used_regs);
>> +	  return;
>> +	}
>> +
>> +      IOR_HARD_REG_SET (node->function_used_regs, insn_used_regs);
>> +    }

<SNIP>

> Let's suppose that we've got a rather large function, with only local calls for
> which we can acquire usage.  Let's suppose that even one of those callees
> further calls something else, such that insn_used_regs == call_used_reg_set.
>
> We fill node->function_used_regs immediately, but keep scanning the rest of the
> large function.
>
>
>> +
>> +  /* Be conservative - mark fixed and global registers as used.  */
>> +  IOR_HARD_REG_SET (node->function_used_regs, fixed_reg_set);
>> +  for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
>> +    if (global_regs[i])
>> +      SET_HARD_REG_BIT (node->function_used_regs, i);
>> +
>> +#ifdef STACK_REGS
>> +  /* Handle STACK_REGS conservatively, since the df-framework does not
>> +     provide accurate information for them.  */
>> +
>> +  for (i = FIRST_STACK_REG; i <= LAST_STACK_REG; i++)
>> +    SET_HARD_REG_BIT (node->function_used_regs, i);
>> +#endif
>> +
>> +  node->function_used_regs_valid = 1;
>
> Wouldn't it be better to compare the collected function_used_regs; if it
> contains all of call_used_reg_set, decline to set function_used_regs_valid.
> That way, we'll early exit from the above loop whenever we see that we can't
> improve over the default call-clobber set.
>

Richard,

Agreed.  Attached patch implements this (on top of the minor rewrite of 
https://gcc.gnu.org/ml/gcc-patches/2014-06/msg01535.html ).

> Although perhaps function_used_regs_valid is no longer the best name in that
> case...
>

I think the name is still ok.  The field function_used_regs_valid just states 
that the function_used_regs field is valid and can be used.


OK for trunk if bootstrap and reg-test on x86_64 is ok ?

Thanks,
- Tom

[-- Attachment #2: 0007-Don-t-save-function_used_regs-if-it-contains-all-cal.patch --]
[-- Type: text/x-patch, Size: 707 bytes --]

2014-06-19  Tom de Vries  <tom@codesourcery.com>

	* final.c (collect_fn_hard_reg_usage): Don't save function_used_regs if
	it contains all call_used_regs.

diff --git a/gcc/final.c b/gcc/final.c
index e39930d..e67e84b 100644
--- a/gcc/final.c
+++ b/gcc/final.c
@@ -4795,6 +4795,11 @@ collect_fn_hard_reg_usage (void)
     SET_HARD_REG_BIT (function_used_regs, i);
 #endif
 
+  /* The information we have gathered is only interesting if it exposes a
+     register from the call_used_regs that is not used in this function.  */
+  if (hard_reg_set_subset_p (call_used_reg_set, function_used_regs))
+    return;
+
   node = cgraph_rtl_info (current_function_decl);
   gcc_assert (node != NULL);
 
-- 
1.9.1

Re: -fuse-caller-save - Collect register usage information

[Richard Henderson]

On 06/19/2014 09:06 AM, Tom de Vries wrote:
> 
> 2014-06-19  Tom de Vries  <tom@codesourcery.com>
> 
> 	* final.c (collect_fn_hard_reg_usage): Don't save function_used_regs if
> 	it contains all call_used_regs.

Ok.


r~

Re: -fuse-caller-save - Collect register usage information

[Jan Hubicka]

> On 06/19/2014 09:06 AM, Tom de Vries wrote:
> > 
> > 2014-06-19  Tom de Vries  <tom@codesourcery.com>
> > 
> > 	* final.c (collect_fn_hard_reg_usage): Don't save function_used_regs if
> > 	it contains all call_used_regs.
> 
> Ok.

When we now have way to represent different reg usages for functions, what would be best
way to make local functions to default into saving some SSE registers on x86/x86-64?

Honza

Re: -fuse-caller-save - Collect register usage information

[Richard Henderson]

On 06/19/2014 12:36 PM, Jan Hubicka wrote:
>> On 06/19/2014 09:06 AM, Tom de Vries wrote:
>>>
>>> 2014-06-19  Tom de Vries  <tom@codesourcery.com>
>>>
>>> 	* final.c (collect_fn_hard_reg_usage): Don't save function_used_regs if
>>> 	it contains all call_used_regs.
>>
>> Ok.
> 
> When we now have way to represent different reg usages for functions, what would be best
> way to make local functions to default into saving some SSE registers on x86/x86-64?

I wouldn't do that at all.  Leave all sse registers call-clobbered.  This way
you don't need to have different entry points (or one possibly less efficient
entry point) when a function is used both locally and globally.

What I would investigate is how to use this hard reg usage data in the register
allocator.  If we know that the callee only uses xmm0-xmm4, then we can keep
xmm5-xmm15 live across the call.


r~

Re: -fuse-caller-save - Collect register usage information

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1177 bytes --]

On 19-06-14 21:45, Richard Henderson wrote:
> On 06/19/2014 12:36 PM, Jan Hubicka wrote:
>>> On 06/19/2014 09:06 AM, Tom de Vries wrote:
>>>>
>>>> 2014-06-19  Tom de Vries  <tom@codesourcery.com>
>>>>
>>>> 	* final.c (collect_fn_hard_reg_usage): Don't save function_used_regs if
>>>> 	it contains all call_used_regs.
>>>
>>> Ok.
>>
>> When we now have way to represent different reg usages for functions, what would be best
>> way to make local functions to default into saving some SSE registers on x86/x86-64?
>
> I wouldn't do that at all.  Leave all sse registers call-clobbered.  This way
> you don't need to have different entry points (or one possibly less efficient
> entry point) when a function is used both locally and globally.
>
> What I would investigate is how to use this hard reg usage data in the register
> allocator.  If we know that the callee only uses xmm0-xmm4, then we can keep
> xmm5-xmm15 live across the call.
>

AFAIU, what you describe here already works. This patch contains a version of 
the fuse-caller-save test with xmm registers. The callee bar only uses xmm0, and 
caller foo keeps xmm1 live across the call.

OK for trunk?

Thanks,
- Tom


[-- Attachment #2: 0008-Add-xmm-register-version-of-fuse-caller-save-testcas.patch --]
[-- Type: text/x-patch, Size: 2001 bytes --]

2014-07-13  Tom de Vries  <tom@codesourcery.com>

	* gcc.target/i386/fuse-caller-save-xmm-run.c: New test.
	* gcc.target/i386/fuse-caller-save-xmm.c: New test.

diff --git a/gcc/testsuite/gcc.target/i386/fuse-caller-save-xmm-run.c b/gcc/testsuite/gcc.target/i386/fuse-caller-save-xmm-run.c
new file mode 100644
index 0000000..17385fa
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/fuse-caller-save-xmm-run.c
@@ -0,0 +1,34 @@
+/* { dg-do run } */
+/* { dg-options "-O2 -msse -fuse-caller-save" } */
+
+typedef double v2df __attribute__((vector_size (16)));
+
+static v2df __attribute__((noinline))
+bar (v2df a)
+{ 
+  return a + (v2df){ 3.0, 3.0 };
+}
+
+v2df __attribute__((noinline))
+foo (v2df y)
+{
+  return y + bar (y);
+}
+
+int
+main (void)
+{
+  int success;
+  union {
+    v2df v;
+    double d[2];
+  } u;
+
+  u.v = foo ((v2df){ 5.0, 5.0});
+  success = (u.d[0] == 13.0
+	     && u.d[1] == 13.0);
+
+  return !success;
+}
+
+
diff --git a/gcc/testsuite/gcc.target/i386/fuse-caller-save-xmm.c b/gcc/testsuite/gcc.target/i386/fuse-caller-save-xmm.c
new file mode 100644
index 0000000..de1ca63
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/fuse-caller-save-xmm.c
@@ -0,0 +1,38 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -msse -fuse-caller-save" } */
+
+typedef double v2df __attribute__((vector_size (16)));
+
+static v2df __attribute__((noinline))
+bar (v2df a)
+{ 
+  return a + (v2df){ 3.0, 3.0 };
+}
+
+v2df __attribute__((noinline))
+foo (v2df y)
+{
+  return y + bar (y);
+}
+
+int
+main (void)
+{
+  int success;
+  union {
+    v2df v;
+    double d[2];
+  } u;
+
+  u.v = foo ((v2df){ 5.0, 5.0});
+  success = (u.d[0] == 13.0
+	     && u.d[1] == 13.0);
+
+  return !success;
+}
+
+/* { dg-final { scan-assembler-not "movaps\t%xmm1, \\(%rsp\\)" } } */
+/* { dg-final { scan-assembler-not "movapd\t\\(%rsp\\), %xmm1" } } */
+/* { dg-final { scan-assembler-times ".cfi_def_cfa_offset 16" 1 } } */
+/* { dg-final { scan-assembler-times ".cfi_def_cfa_offset 32" 1 } } */
+

Re: -fuse-caller-save - Collect register usage information

[Jan Hubicka]

> >>When we now have way to represent different reg usages for functions, what would be best
> >>way to make local functions to default into saving some SSE registers on x86/x86-64?
> >
> >I wouldn't do that at all.  Leave all sse registers call-clobbered.  This way
> >you don't need to have different entry points (or one possibly less efficient
> >entry point) when a function is used both locally and globally.
> >
> >What I would investigate is how to use this hard reg usage data in the register
> >allocator.  If we know that the callee only uses xmm0-xmm4, then we can keep
> >xmm5-xmm15 live across the call.

What I was shooting for whas to
  1) make it possible to declare by attribute that some registers are not call clobbered
  2) have more sane default when the call usage data are not available - either because of LTO partitioning
     or because the callee was not copmiled yet.
> >
> 
> AFAIU, what you describe here already works. This patch contains a
> version of the fuse-caller-save test with xmm registers. The callee
> bar only uses xmm0, and caller foo keeps xmm1 live across the call.
> 
> OK for trunk?
> 
> Thanks,
> - Tom
> 

> 2014-07-13  Tom de Vries  <tom@codesourcery.com>
> 
> 	* gcc.target/i386/fuse-caller-save-xmm-run.c: New test.
> 	* gcc.target/i386/fuse-caller-save-xmm.c: New test.

OK
Honza
> 
> diff --git a/gcc/testsuite/gcc.target/i386/fuse-caller-save-xmm-run.c b/gcc/testsuite/gcc.target/i386/fuse-caller-save-xmm-run.c
> new file mode 100644
> index 0000000..17385fa
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/fuse-caller-save-xmm-run.c
> @@ -0,0 +1,34 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2 -msse -fuse-caller-save" } */
> +
> +typedef double v2df __attribute__((vector_size (16)));
> +
> +static v2df __attribute__((noinline))
> +bar (v2df a)
> +{ 
> +  return a + (v2df){ 3.0, 3.0 };
> +}
> +
> +v2df __attribute__((noinline))
> +foo (v2df y)
> +{
> +  return y + bar (y);
> +}
> +
> +int
> +main (void)
> +{
> +  int success;
> +  union {
> +    v2df v;
> +    double d[2];
> +  } u;
> +
> +  u.v = foo ((v2df){ 5.0, 5.0});
> +  success = (u.d[0] == 13.0
> +	     && u.d[1] == 13.0);
> +
> +  return !success;
> +}
> +
> +
> diff --git a/gcc/testsuite/gcc.target/i386/fuse-caller-save-xmm.c b/gcc/testsuite/gcc.target/i386/fuse-caller-save-xmm.c
> new file mode 100644
> index 0000000..de1ca63
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/fuse-caller-save-xmm.c
> @@ -0,0 +1,38 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -msse -fuse-caller-save" } */
> +
> +typedef double v2df __attribute__((vector_size (16)));
> +
> +static v2df __attribute__((noinline))
> +bar (v2df a)
> +{ 
> +  return a + (v2df){ 3.0, 3.0 };
> +}
> +
> +v2df __attribute__((noinline))
> +foo (v2df y)
> +{
> +  return y + bar (y);
> +}
> +
> +int
> +main (void)
> +{
> +  int success;
> +  union {
> +    v2df v;
> +    double d[2];
> +  } u;
> +
> +  u.v = foo ((v2df){ 5.0, 5.0});
> +  success = (u.d[0] == 13.0
> +	     && u.d[1] == 13.0);
> +
> +  return !success;
> +}
> +
> +/* { dg-final { scan-assembler-not "movaps\t%xmm1, \\(%rsp\\)" } } */
> +/* { dg-final { scan-assembler-not "movapd\t\\(%rsp\\), %xmm1" } } */
> +/* { dg-final { scan-assembler-times ".cfi_def_cfa_offset 16" 1 } } */
> +/* { dg-final { scan-assembler-times ".cfi_def_cfa_offset 32" 1 } } */
> +

Re: -fuse-caller-save - Collect register usage information

[Mike Stump]

On Apr 23, 2014, at 3:41 AM, Tom de Vries <Tom_deVries@mentor.com> wrot
> On 22-04-14 17:05, Tom de Vries wrote:
>> I've updated the fuse-caller-save patch series to model non-callee call clobbers
>> in CALL_INSN_FUNCTION_USAGE.

> @item -fuse-caller-save
> Use caller save registers for allocation if those registers are not used by
> any called function.  In that case it is not necessary to save and restore
> them around calls.  This is only possible if called functions are part of
> same compilation unit as current function and they are compiled before it.

So, I hate the name of the option, and the documentation seems wrong to me.  It doesn’t use the caller saved registers for allocation, it uses the call clobbered registers for allocation.  Or, one could say it uses the callee saved registers for allocation.

Re: -fuse-caller-save - Collect register usage information

[Eric Botcazou]

> So, I hate the name of the option, and the documentation seems wrong to me. 
> It doesn’t use the caller saved registers for allocation, it uses the call
> clobbered registers for allocation.  Or, one could say it uses the callee
> saved registers for allocation.

Seconded, the description is a bit confusing and "caller saved"/"callee saved" 
should be avoided IMO, "call clobbered"/"call saved" is much clearer.

-- 
Eric Botcazou

Re: -fuse-caller-save - Collect register usage information

[Tom de Vries]

On 11-10-14 12:46, Eric Botcazou wrote:
>> So, I hate the name of the option, and the documentation seems wrong to me.
>> It doesn’t use the caller saved registers for allocation, it uses the call
>> clobbered registers for allocation.  Or, one could say it uses the callee
>> saved registers for allocation.
>

Eric,
Mike,

thanks for the comments.

I know the following definition of caller-vs-callee saved registers:
- at function entry, a caller-save register can be used immediately, without
   needing to save it first. However, in order to store a value in it that is
   live across calls, it needs to be saved and restored around each call (hence
   the term caller-save).
- at function entry, a callee-save register first needs to be saved before it
   can be used (hence the name callee-save). However, that means that it can be
   used to store a value that is live across calls, without further need for
   saving/restoring.

So, AFAIU, call clobbered corresponds with caller-save, not with callee saved. 
So I'd say the documentation is in fact correct.

Having said that, in my mind, what is confusing about the name 
-fuse-caller-save, is that in fact the caller-save registers are already used in 
register allocation. It's just that they're used across calls without the need 
to save them, but -fuse-caller-save-across-calls-without-saving-if-possible is 
not such a good option name.

Another thing that - in my mind - is confusing is that there's an option 
fcaller-saves which controls behaviour for caller-save registers:
- for -fno-caller-saves, caller-save registers are not used across calls
- for -fcaller-saves, caller-save registers are used across calls
The name is similar to -fuse-caller-save, and it won't be clear from just the 
names what the difference is.

> Seconded, the description is a bit confusing and "caller saved"/"callee saved"
> should be avoided IMO, "call clobbered"/"call saved" is much clearer.
>

I have no objection to go with another terminology in the documentation. But 
before going into a patch, let's settle on the option name.

As for the name, I'm not sure just changing terminology will make things 
clearer, in other words, I'm not sure fuse-call-clobbered is any clearer than 
fuse-caller-save.

I've pondered the name -fipa-ira, but I rejected that earlier because that might 
suggest actual register allocation at the interprocedural scope, while this is 
only register allocation at the scope of a single procedure, taking some 
interprocedural information into account. Furthermore, it's not only ira that 
uses the interprocedural information.

So, let's a generate a list of option names.
-fuse-caller-save
-fuse-call-clobbered
-fprecise-call-clobbers
-foptimize-call-clobbers
-fprune-call-clobbers
-freduce-call-clobbers
-fcall-clobbers-ipa

Any preferences, alternatives?

Thanks,
- Tom

Re: -fuse-caller-save - Collect register usage information

[Eric Botcazou]

> Having said that, in my mind, what is confusing about the name
> -fuse-caller-save, is that in fact the caller-save registers are already
> used in register allocation. It's just that they're used across calls
> without the need to save them, but
> -fuse-caller-save-across-calls-without-saving-if-possible is not such a
> good option name.

Agreed.

> Another thing that - in my mind - is confusing is that there's an option
> fcaller-saves which controls behaviour for caller-save registers:
> - for -fno-caller-saves, caller-save registers are not used across calls
> - for -fcaller-saves, caller-save registers are used across calls
> The name is similar to -fuse-caller-save, and it won't be clear from just
> the names what the difference is.

OK, so the existing -fcaller-saves is in fact -fuse-caller-saves, which means 
that we should really find a better name for yours. :-)

> I've pondered the name -fipa-ira, but I rejected that earlier because that
> might suggest actual register allocation at the interprocedural scope,
> while this is only register allocation at the scope of a single procedure,
> taking some interprocedural information into account. Furthermore, it's not
> only ira that uses the interprocedural information.
> 
> So, let's a generate a list of option names.
> -fuse-caller-save
> -fuse-call-clobbered
> -fprecise-call-clobbers
> -foptimize-call-clobbers
> -fprune-call-clobbers
> -freduce-call-clobbers
> -fcall-clobbers-ipa
> 
> Any preferences, alternatives?

Given the existing -fcaller-saves, I'd keep "caller-saves" in the name, so 
something along the lines of -foptimize-caller-saves or -fipa-caller-saves.

-- 
Eric Botcazou

Re: -fuse-caller-save - Collect register usage information

[Mike Stump]

On Oct 16, 2014, at 2:28 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> So I'd say the documentation is in fact correct.

Agreed.  I was confused.  Apparently my mind likes to think in volatile and non-volatile (call-clobbered and non-call-clobbered) and I managed to map it wrong in my mind and not catch that fact I mapped them wrong.

> So, let's a generate a list of option names.

> Any preferences, alternatives?

So, since I was confused, my primary reason for not liking the option name was misguided.

Re: -fuse-caller-save - Collect register usage information

[Tom de Vries]

On 16-10-14 23:46, Eric Botcazou wrote:
>> Having said that, in my mind, what is confusing about the name
>> -fuse-caller-save, is that in fact the caller-save registers are already
>> used in register allocation. It's just that they're used across calls
>> without the need to save them, but
>> -fuse-caller-save-across-calls-without-saving-if-possible is not such a
>> good option name.
>
> Agreed.
>
>> Another thing that - in my mind - is confusing is that there's an option
>> fcaller-saves which controls behaviour for caller-save registers:
>> - for -fno-caller-saves, caller-save registers are not used across calls
>> - for -fcaller-saves, caller-save registers are used across calls
>> The name is similar to -fuse-caller-save, and it won't be clear from just
>> the names what the difference is.
>
> OK, so the existing -fcaller-saves is in fact -fuse-caller-saves,

Right, in the sense that a caller-save is the save of caller-save register, as 
opposed to short for a caller-save register, which is how it's used in 
-fuse-caller-save.

> which means
> that we should really find a better name for yours. :-)
>

Agreed :)

>> I've pondered the name -fipa-ira, but I rejected that earlier because that
>> might suggest actual register allocation at the interprocedural scope,
>> while this is only register allocation at the scope of a single procedure,
>> taking some interprocedural information into account. Furthermore, it's not
>> only ira that uses the interprocedural information.
>>
>> So, let's a generate a list of option names.
>> -fuse-caller-save
>> -fuse-call-clobbered
>> -fprecise-call-clobbers
>> -foptimize-call-clobbers
>> -fprune-call-clobbers
>> -freduce-call-clobbers
>> -fcall-clobbers-ipa
>>
>> Any preferences, alternatives?
>
> Given the existing -fcaller-saves, I'd keep "caller-saves" in the name, so
> something along the lines of -foptimize-caller-saves or -fipa-caller-saves.
>

Let's look at the effect of the option (after the recent fix for PR61605) on 
gcc.target/i386/fuse-calller-save.c:
...
  foo:
  .LFB1:
  	.cfi_startproc
-	pushq	%rbx
-	.cfi_def_cfa_offset 16
-	.cfi_offset 3, -16
-	movl	%edi, %ebx
  	call	bar
-	addl	%ebx, %eax
-	popq	%rbx
-	.cfi_def_cfa_offset 8
+	addl	%edi, %eax
  	ret
  	.cfi_endproc
  .LFE1:
...
So, the effect is: instead of using a callee-save register, we use a caller-save 
register to store a value that's live over a call, without needing to add a 
caller-save, as would be normally the case.

If I see an option -foptimize-caller-saves, I'd expect the effect to be that 
without, there are some caller-saves and with, there are less. This is not the 
case in the diff above. Nevertheless, if we'd have a case where we already have 
caller-saves, that would be indeed the observed effect. I'm just trying to point 
out that the optimization does more than just removing caller-saves.

The optimization, at it's core, can be regarded as removing superfluous clobbers 
from calls, and everything else is derived from that:
- if a caller-save register is not clobbered by a call, then there's no need
   for a caller-save before that call, so it's cheaper to use across that call
   than a callee-save register.
   (which explains what we see in the diff)
- if a caller-save register is live across a call, and is not clobbered by a
   call, then there's no need for a caller-save, and it can be removed.
   (which explains what we see in case we have an example where there are
    actual caller-saves without the optimization, and less so with the
    optimization)

I'm starting to lean towards -foptimize-call-clobbers or similar.

Thanks,
- Tom

Re: -fuse-caller-save - Collect register usage information

[Richard Biener]

On Fri, Oct 17, 2014 at 12:47 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> On 16-10-14 23:46, Eric Botcazou wrote:
>>>
>>> Having said that, in my mind, what is confusing about the name
>>> -fuse-caller-save, is that in fact the caller-save registers are already
>>> used in register allocation. It's just that they're used across calls
>>> without the need to save them, but
>>> -fuse-caller-save-across-calls-without-saving-if-possible is not such a
>>> good option name.
>>
>>
>> Agreed.
>>
>>> Another thing that - in my mind - is confusing is that there's an option
>>> fcaller-saves which controls behaviour for caller-save registers:
>>> - for -fno-caller-saves, caller-save registers are not used across calls
>>> - for -fcaller-saves, caller-save registers are used across calls
>>> The name is similar to -fuse-caller-save, and it won't be clear from just
>>> the names what the difference is.
>>
>>
>> OK, so the existing -fcaller-saves is in fact -fuse-caller-saves,
>
>
> Right, in the sense that a caller-save is the save of caller-save register,
> as opposed to short for a caller-save register, which is how it's used in
> -fuse-caller-save.
>
>> which means
>> that we should really find a better name for yours. :-)
>>
>
> Agreed :)
>
>>> I've pondered the name -fipa-ira, but I rejected that earlier because
>>> that
>>> might suggest actual register allocation at the interprocedural scope,
>>> while this is only register allocation at the scope of a single
>>> procedure,
>>> taking some interprocedural information into account. Furthermore, it's
>>> not
>>> only ira that uses the interprocedural information.
>>>
>>> So, let's a generate a list of option names.
>>> -fuse-caller-save
>>> -fuse-call-clobbered
>>> -fprecise-call-clobbers
>>> -foptimize-call-clobbers
>>> -fprune-call-clobbers
>>> -freduce-call-clobbers
>>> -fcall-clobbers-ipa
>>>
>>> Any preferences, alternatives?
>>
>>
>> Given the existing -fcaller-saves, I'd keep "caller-saves" in the name, so
>> something along the lines of -foptimize-caller-saves or
>> -fipa-caller-saves.
>>
>
> Let's look at the effect of the option (after the recent fix for PR61605) on
> gcc.target/i386/fuse-calller-save.c:
> ...
>  foo:
>  .LFB1:
>         .cfi_startproc
> -       pushq   %rbx
> -       .cfi_def_cfa_offset 16
> -       .cfi_offset 3, -16
> -       movl    %edi, %ebx
>         call    bar
> -       addl    %ebx, %eax
> -       popq    %rbx
> -       .cfi_def_cfa_offset 8
> +       addl    %edi, %eax
>         ret
>         .cfi_endproc
>  .LFE1:
> ...
> So, the effect is: instead of using a callee-save register, we use a
> caller-save register to store a value that's live over a call, without
> needing to add a caller-save, as would be normally the case.
>
> If I see an option -foptimize-caller-saves, I'd expect the effect to be that
> without, there are some caller-saves and with, there are less. This is not
> the case in the diff above. Nevertheless, if we'd have a case where we
> already have caller-saves, that would be indeed the observed effect. I'm
> just trying to point out that the optimization does more than just removing
> caller-saves.
>
> The optimization, at it's core, can be regarded as removing superfluous
> clobbers from calls, and everything else is derived from that:
> - if a caller-save register is not clobbered by a call, then there's no need
>   for a caller-save before that call, so it's cheaper to use across that
> call
>   than a callee-save register.
>   (which explains what we see in the diff)
> - if a caller-save register is live across a call, and is not clobbered by a
>   call, then there's no need for a caller-save, and it can be removed.
>   (which explains what we see in case we have an example where there are
>    actual caller-saves without the optimization, and less so with the
>    optimization)
>
> I'm starting to lean towards -foptimize-call-clobbers or similar.

Well, it is really some form of IPA driven register allocation.  Whether
you want to call it -fipa-ra or not is another question - but if we had
such option then enabling it with that option would be fine.

Also users may have no idea what call vs callee clobbers are, but
IPA RA may be a term that is more widely known (or at least google
can come up with something for you).

So - I like -fipa-ra more.

I can't see the obvious difference between -foptimize-caller-saves
and -foptimize-call-clobbers (for the latter -fipa-call-clobbers would
be more to the point?)

Richard.

> Thanks,
> - Tom

Re: -fuse-caller-save - Collect register usage information

[Jeff Law]

On 10/17/14 05:00, Richard Biener wrote:
>>
>> I'm starting to lean towards -foptimize-call-clobbers or similar.
>
> Well, it is really some form of IPA driven register allocation.  Whether
> you want to call it -fipa-ra or not is another question - but if we had
> such option then enabling it with that option would be fine.
>
> Also users may have no idea what call vs callee clobbers are, but
> IPA RA may be a term that is more widely known (or at least google
> can come up with something for you).
>
> So - I like -fipa-ra more.
Similarly.  At the heart of the matter is we're utilizing information 
about the callee's behaviour to improve the code we generate in the 
caller.  That's clearly in IPA's domain IMHO.



Jeff

Re: -fuse-caller-save - Collect register usage information

[Mike Stump]

On Oct 17, 2014, at 8:50 AM, Jeff Law <law@redhat.com> wrote:
>> So - I like -fipa-ra more.
> Similarly.

Yeah, I was going to say I liked the ipa tag in there some place but didn’t cause I didn’t want to bikeshed, but, since a few others like that, dogpiling seems ok.  :-)

Re: -fuse-caller-save - Collect register usage information

[Eric Botcazou]

> Let's look at the effect of the option (after the recent fix for PR61605) on
> gcc.target/i386/fuse-calller-save.c:
> ...
>   foo:
>   .LFB1:
>   	.cfi_startproc
> -	pushq	%rbx
> -	.cfi_def_cfa_offset 16
> -	.cfi_offset 3, -16
> -	movl	%edi, %ebx
>   	call	bar
> -	addl	%ebx, %eax
> -	popq	%rbx
> -	.cfi_def_cfa_offset 8
> +	addl	%edi, %eax
>   	ret
>   	.cfi_endproc
>   .LFE1:
> ...
> So, the effect is: instead of using a callee-save register, we use a
> caller-save register to store a value that's live over a call, without
> needing to add a caller-save, as would be normally the case.
> 
> If I see an option -foptimize-caller-saves, I'd expect the effect to be that
> without, there are some caller-saves and with, there are less. This is not
> the case in the diff above.

To me it is, "movl %edi, %ebx"/"addl %ebx, %eax" is a caller-save/restore.

> I'm starting to lean towards -foptimize-call-clobbers or similar.

Yes, that's also a good name and was my initial preference.  But you pointed 
out the existing -fcaller-saves:

`-fcaller-saves'
     Enable allocation of values to registers that are clobbered by
     function calls, by emitting extra instructions to save and restore
     the registers around such calls.  Such allocation is done only
     when it seems to result in better code.

so -foptimize-caller-saves can be understood as optimizing out the "extra 
instructions to save and restore  the registers around such calls" and, thus, 
as having a direct relationship with -fcaller-saves.

-- 
Eric Botcazou

Re: -fuse-caller-save - Collect register usage information

[Tom de Vries]

On 17-10-14 21:24, Eric Botcazou wrote:
>> Let's look at the effect of the option (after the recent fix for PR61605) on
>> gcc.target/i386/fuse-calller-save.c:
>> ...
>>    foo:
>>    .LFB1:
>>    	.cfi_startproc
>> -	pushq	%rbx
>> -	.cfi_def_cfa_offset 16
>> -	.cfi_offset 3, -16
>> -	movl	%edi, %ebx
>>    	call	bar
>> -	addl	%ebx, %eax
>> -	popq	%rbx
>> -	.cfi_def_cfa_offset 8
>> +	addl	%edi, %eax
>>    	ret
>>    	.cfi_endproc
>>    .LFE1:
>> ...
>> So, the effect is: instead of using a callee-save register, we use a
>> caller-save register to store a value that's live over a call, without
>> needing to add a caller-save, as would be normally the case.
>>
>> If I see an option -foptimize-caller-saves, I'd expect the effect to be that
>> without, there are some caller-saves and with, there are less. This is not
>> the case in the diff above.
>
> To me it is, "movl %edi, %ebx"/"addl %ebx, %eax" is a caller-save/restore.
>

I agree that it can look like that. But the insn 'movl %edi, %ebx' is generated 
by assign_parm_setup_reg at expand. AFAIU, the purpose is to decouple the value 
of the argument and its uses from the register it's passed in.

The definition of -fcaller-saves below explains why insn 'movl %edi, %ebx' is 
not a caller-save: because it's not generated before a call, but rather at the 
start of a function. This seems to be confirmed by the fact that the insn 'movl 
%edi, %ebx' is still generated with -fno-caller-saves.

>> I'm starting to lean towards -foptimize-call-clobbers or similar.
>
> Yes, that's also a good name and was my initial preference.  But you pointed
> out the existing -fcaller-saves:
>
> `-fcaller-saves'
>       Enable allocation of values to registers that are clobbered by
>       function calls, by emitting extra instructions to save and restore
>       the registers around such calls.  Such allocation is done only
>       when it seems to result in better code.
>
> so -foptimize-caller-saves can be understood as optimizing out the "extra
> instructions to save and restore  the registers around such calls"  and, thus,
> as having a direct relationship with -fcaller-saves.

Agree.

But, given the preference of a number of others for fipa-ra, could you live with 
that?

Thanks,
- Tom

Re: -fuse-caller-save - Collect register usage information

[Eric Botcazou]

> But, given the preference of a number of others for fipa-ra, could you live
> with that?

Yes, IMO that's too vague a name but still better than the existing one. :-)

-- 
Eric Botcazou