[10/32] Remove global call sets: combine.c
diff mbox series

Message ID mptblvqy7k9.fsf@arm.com
State New
Headers show
Series
  • Support multiple ABIs in the same translation unit
Related show

Commit Message

Richard Sandiford Sept. 11, 2019, 7:08 p.m. UTC
There shouldn't be many cases in which a useful hard register is
live across a call before RA, so we might as well keep things simple
and invalidate partially-clobbered registers here, in case the values
they hold leak into the call-clobbered part.  In principle this is
a bug fix for TARGET_HARD_REGNO_CALL_PART_CLOBBERED targets,
but in practice it probably doesn't make a difference.


2019-09-11  Richard Sandiford  <richard.sandiford@arm.com>

gcc/
	* combine.c: Include function-abi.h.
	(record_dead_and_set_regs): Use call_insn_abi to get the ABI
	of call insns.  Invalidate partially-clobbered registers as
	well as fully-clobbered ones.

Comments

Segher Boessenkool Sept. 12, 2019, 2:18 a.m. UTC | #1
On Wed, Sep 11, 2019 at 08:08:38PM +0100, Richard Sandiford wrote:
>        hard_reg_set_iterator hrsi;
> -      EXECUTE_IF_SET_IN_HARD_REG_SET (regs_invalidated_by_call, 0, i, hrsi)
> +      EXECUTE_IF_SET_IN_HARD_REG_SET (abi.full_and_partial_reg_clobbers (),
> +				      0, i, hrsi)

So "abi" in that means calls?  It is not such a great name like that.
Since its children are very_long_names, it doesn't need to be only three
chars itself, either?


Segher
Richard Sandiford Sept. 12, 2019, 7:51 a.m. UTC | #2
Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Wed, Sep 11, 2019 at 08:08:38PM +0100, Richard Sandiford wrote:
>>        hard_reg_set_iterator hrsi;
>> -      EXECUTE_IF_SET_IN_HARD_REG_SET (regs_invalidated_by_call, 0, i, hrsi)
>> +      EXECUTE_IF_SET_IN_HARD_REG_SET (abi.full_and_partial_reg_clobbers (),
>> +				      0, i, hrsi)
>
> So "abi" in that means calls?

"abi" is the interface of the callee function, taking things like
function attributes and -fipa-ra into account.

The register sets are describing what the callee does rather than
what calls to it do.  E.g. on targets that allow linker stubs to be
inserted between calls, the scratch registers reserved for linker stubs
are still call-clobbered, even if the target of the call doesn't use
them.  (Those call clobbers are represented separately, at least when
TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS is true.  When it's
false we don't use -fipa-ra information at all.)

> It is not such a great name like that.  Since its children are
> very_long_names, it doesn't need to be only three chars itself,
> either?

OK, what name would you prefer?

Richard
Segher Boessenkool Sept. 20, 2019, 12:43 a.m. UTC | #3
Hi Richard,

Sorry this too me so long to get back to.

On Thu, Sep 12, 2019 at 08:51:59AM +0100, Richard Sandiford wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> > On Wed, Sep 11, 2019 at 08:08:38PM +0100, Richard Sandiford wrote:
> >>        hard_reg_set_iterator hrsi;
> >> -      EXECUTE_IF_SET_IN_HARD_REG_SET (regs_invalidated_by_call, 0, i, hrsi)
> >> +      EXECUTE_IF_SET_IN_HARD_REG_SET (abi.full_and_partial_reg_clobbers (),
> >> +				      0, i, hrsi)
> >
> > So "abi" in that means calls?
> 
> "abi" is the interface of the callee function, taking things like
> function attributes and -fipa-ra into account.
> 
> The register sets are describing what the callee does rather than
> what calls to it do.  E.g. on targets that allow linker stubs to be
> inserted between calls, the scratch registers reserved for linker stubs
> are still call-clobbered, even if the target of the call doesn't use
> them.  (Those call clobbers are represented separately, at least when
> TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS is true.  When it's
> false we don't use -fipa-ra information at all.)
> 
> > It is not such a great name like that.  Since its children are
> > very_long_names, it doesn't need to be only three chars itself,
> > either?
> 
> OK, what name would you prefer?

Maybe call_abi is a good name?  It's difficult to capture the subtleties
in a short enough name.  As always :-)


Segher
Richard Sandiford Sept. 25, 2019, 3:52 p.m. UTC | #4
Segher Boessenkool <segher@kernel.crashing.org> writes:
> Hi Richard,
>
> Sorry this too me so long to get back to.
>
> On Thu, Sep 12, 2019 at 08:51:59AM +0100, Richard Sandiford wrote:
>> Segher Boessenkool <segher@kernel.crashing.org> writes:
>> > On Wed, Sep 11, 2019 at 08:08:38PM +0100, Richard Sandiford wrote:
>> >>        hard_reg_set_iterator hrsi;
>> >> -      EXECUTE_IF_SET_IN_HARD_REG_SET (regs_invalidated_by_call, 0, i, hrsi)
>> >> +      EXECUTE_IF_SET_IN_HARD_REG_SET (abi.full_and_partial_reg_clobbers (),
>> >> +				      0, i, hrsi)
>> >
>> > So "abi" in that means calls?
>> 
>> "abi" is the interface of the callee function, taking things like
>> function attributes and -fipa-ra into account.
>> 
>> The register sets are describing what the callee does rather than
>> what calls to it do.  E.g. on targets that allow linker stubs to be
>> inserted between calls, the scratch registers reserved for linker stubs
>> are still call-clobbered, even if the target of the call doesn't use
>> them.  (Those call clobbers are represented separately, at least when
>> TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS is true.  When it's
>> false we don't use -fipa-ra information at all.)
>> 
>> > It is not such a great name like that.  Since its children are
>> > very_long_names, it doesn't need to be only three chars itself,
>> > either?
>> 
>> OK, what name would you prefer?
>
> Maybe call_abi is a good name?  It's difficult to capture the subtleties
> in a short enough name.  As always :-)

The formatting ended up being a bit weird with a longer name,
so how about the attached instead?

Richard


2019-09-25  Richard Sandiford  <richard.sandiford@arm.com>

gcc/
	* combine.c: Include function-abi.h.
	(record_dead_and_set_regs): Use insn_callee_abi to get the ABI
	of the target of call insns.  Invalidate partially-clobbered
	registers as well as fully-clobbered ones.

Index: gcc/combine.c
===================================================================
--- gcc/combine.c	2019-09-12 10:52:53.000000000 +0100
+++ gcc/combine.c	2019-09-25 16:50:21.772865265 +0100
@@ -105,6 +105,7 @@ Software Foundation; either version 3, o
 #include "valtrack.h"
 #include "rtl-iter.h"
 #include "print-rtl.h"
+#include "function-abi.h"
 
 /* Number of attempts to combine instructions in this function.  */
 
@@ -13464,11 +13465,21 @@ record_dead_and_set_regs (rtx_insn *insn
 
   if (CALL_P (insn))
     {
+      HARD_REG_SET callee_clobbers
+	= insn_callee_abi (insn).full_and_partial_reg_clobbers ();
       hard_reg_set_iterator hrsi;
-      EXECUTE_IF_SET_IN_HARD_REG_SET (regs_invalidated_by_call, 0, i, hrsi)
+      EXECUTE_IF_SET_IN_HARD_REG_SET (callee_clobbers, 0, i, hrsi)
 	{
 	  reg_stat_type *rsp;
 
+	  /* ??? We could try to preserve some information from the last
+	     set of register I if the call doesn't actually clobber
+	     (reg:last_set_mode I), which might be true for ABIs with
+	     partial clobbers.  However, it would be difficult to
+	     update last_set_nonzero_bits and last_sign_bit_copies
+	     to account for the part of I that actually was clobbered.
+	     It wouldn't help much anyway, since we rarely see this
+	     situation before RA.  */
 	  rsp = &reg_stat[i];
 	  rsp->last_set_invalid = 1;
 	  rsp->last_set = insn;
Segher Boessenkool Sept. 25, 2019, 4:30 p.m. UTC | #5
On Wed, Sep 25, 2019 at 04:52:14PM +0100, Richard Sandiford wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> > On Thu, Sep 12, 2019 at 08:51:59AM +0100, Richard Sandiford wrote:
> >> Segher Boessenkool <segher@kernel.crashing.org> writes:
> >> > It is not such a great name like that.  Since its children are
> >> > very_long_names, it doesn't need to be only three chars itself,
> >> > either?
> >> 
> >> OK, what name would you prefer?
> >
> > Maybe call_abi is a good name?  It's difficult to capture the subtleties
> > in a short enough name.  As always :-)
> 
> The formatting ended up being a bit weird with a longer name,
> so how about the attached instead?

That looks great, thanks!

> +	  /* ??? We could try to preserve some information from the last
> +	     set of register I if the call doesn't actually clobber
> +	     (reg:last_set_mode I), which might be true for ABIs with
> +	     partial clobbers.  However, it would be difficult to
> +	     update last_set_nonzero_bits and last_sign_bit_copies
> +	     to account for the part of I that actually was clobbered.
> +	     It wouldn't help much anyway, since we rarely see this
> +	     situation before RA.  */

I would like to completely get rid of reg_stat, and have known bits
dealt with by some DF thing instead...  It would work much better and
be much easier to use at the same time.  Also, other passes could use
it as well.

If I ever will find time to do this, I don't know :-/


Segher
Jeff Law Sept. 29, 2019, 10:32 p.m. UTC | #6
On 9/25/19 9:52 AM, Richard Sandiford wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
>> Hi Richard,
>>
>> Sorry this too me so long to get back to.
>>
>> On Thu, Sep 12, 2019 at 08:51:59AM +0100, Richard Sandiford wrote:
>>> Segher Boessenkool <segher@kernel.crashing.org> writes:
>>>> On Wed, Sep 11, 2019 at 08:08:38PM +0100, Richard Sandiford wrote:
>>>>>        hard_reg_set_iterator hrsi;
>>>>> -      EXECUTE_IF_SET_IN_HARD_REG_SET (regs_invalidated_by_call, 0, i, hrsi)
>>>>> +      EXECUTE_IF_SET_IN_HARD_REG_SET (abi.full_and_partial_reg_clobbers (),
>>>>> +				      0, i, hrsi)
>>>>
>>>> So "abi" in that means calls?
>>>
>>> "abi" is the interface of the callee function, taking things like
>>> function attributes and -fipa-ra into account.
>>>
>>> The register sets are describing what the callee does rather than
>>> what calls to it do.  E.g. on targets that allow linker stubs to be
>>> inserted between calls, the scratch registers reserved for linker stubs
>>> are still call-clobbered, even if the target of the call doesn't use
>>> them.  (Those call clobbers are represented separately, at least when
>>> TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS is true.  When it's
>>> false we don't use -fipa-ra information at all.)
>>>
>>>> It is not such a great name like that.  Since its children are
>>>> very_long_names, it doesn't need to be only three chars itself,
>>>> either?
>>>
>>> OK, what name would you prefer?
>>
>> Maybe call_abi is a good name?  It's difficult to capture the subtleties
>> in a short enough name.  As always :-)
> 
> The formatting ended up being a bit weird with a longer name,
> so how about the attached instead?
> 
> Richard
> 
> 
> 2019-09-25  Richard Sandiford  <richard.sandiford@arm.com>
> 
> gcc/
> 	* combine.c: Include function-abi.h.
> 	(record_dead_and_set_regs): Use insn_callee_abi to get the ABI
> 	of the target of call insns.  Invalidate partially-clobbered
> 	registers as well as fully-clobbered ones.
OK if Segher doesn't object.

jeff
>
Segher Boessenkool Sept. 29, 2019, 10:43 p.m. UTC | #7
On Sun, Sep 29, 2019 at 04:32:13PM -0600, Jeff Law wrote:
> On 9/25/19 9:52 AM, Richard Sandiford wrote:
> > gcc/
> > 	* combine.c: Include function-abi.h.
> > 	(record_dead_and_set_regs): Use insn_callee_abi to get the ABI
> > 	of the target of call insns.  Invalidate partially-clobbered
> > 	registers as well as fully-clobbered ones.
> OK if Segher doesn't object.

https://gcc.gnu.org/ml/gcc-patches/2019-09/msg01462.html

"That looks great, thanks!"

:-)


Segher

Patch
diff mbox series

Index: gcc/combine.c
===================================================================
--- gcc/combine.c	2019-09-09 18:58:51.448270881 +0100
+++ gcc/combine.c	2019-09-11 19:47:57.062032638 +0100
@@ -105,6 +105,7 @@  Software Foundation; either version 3, o
 #include "valtrack.h"
 #include "rtl-iter.h"
 #include "print-rtl.h"
+#include "function-abi.h"
 
 /* Number of attempts to combine instructions in this function.  */
 
@@ -13464,11 +13465,21 @@  record_dead_and_set_regs (rtx_insn *insn
 
   if (CALL_P (insn))
     {
+      function_abi abi = call_insn_abi (insn);
       hard_reg_set_iterator hrsi;
-      EXECUTE_IF_SET_IN_HARD_REG_SET (regs_invalidated_by_call, 0, i, hrsi)
+      EXECUTE_IF_SET_IN_HARD_REG_SET (abi.full_and_partial_reg_clobbers (),
+				      0, i, hrsi)
 	{
 	  reg_stat_type *rsp;
 
+	  /* ??? We could try to preserve some information from the last
+	     set of register I if the call doesn't actually clobber
+	     (reg:last_set_mode I), which might be true for ABIs with
+	     partial clobbers.  However, it would be difficult to
+	     update last_set_nonzero_bits and last_sign_bit_copies
+	     to account for the part of I that actually was clobbered.
+	     It wouldn't help much anyway, since we rarely see this
+	     situation before RA.  */
 	  rsp = &reg_stat[i];
 	  rsp->last_set_invalid = 1;
 	  rsp->last_set = insn;