-fuse-caller-save - Collect register usage information

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

> 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.

> +/* 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

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

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~