Re: [PATCH] Make sibcall argument overlap check less pessimistic (PR middle-end/50074)

[Eric Botcazou <ebotcazou at adacore dot com>]

> Here is an attempt to make the check more complete (e.g.
> the change wouldn't see overlap if addr was PLUS of two REGs,
> where one of the REGs was based on internal_arg_pointer, etc.)
> and less pessimistic.  As tree-tailcall.c doesn't allow tail calls
> from functions that have address of any of the caller's parameters
> taken, IMHO it is enough to look for internal_arg_pointer based
> pseudos initialized in the tail call sequence.
> This patch scans the tail call sequence and notes which pseudos
> are based on internal_arg_pointer (and what offset from
> that pointer they have) and uses that in
> mem_overlaps_already_clobbered_arg_p.

This looks reasonable, but the logic is a bit hard to follow, especially the 
double usage of internal_arg_pointer_based_reg depending on SCAN's value.
Would it be possible to split it into 2 functions that recursively call each 
other?

You should also make it clearer that internal_arg_pointer_seq_start is part of 
the caching mechanism (I wondered for some minutes whether it has anything to 
do with RTL sequences), maybe:

/* Last insn that has been scanned by internal_arg_pointer_based_reg, or
   NULL_RTX if none has been scanned yet.  */
static rtx internal_arg_pointer_insn_cache;

/* Vector indexed by REGNO - FIRST_PSEUDO_REGISTER, recording if a pseudo is
   based on crtl->args.internal_arg_pointer.  The element is NULL_RTX if the
   pseudo isn't based on it, a CONST_INT offset if the pseudo is based on it
   with fixed offset, or PC if this is with variable or unknown offset.  */
static VEC(rtx, heap) *internal_arg_pointer_pseudo_cache;


> +/* Helper function for internal_arg_pointer_based_reg, called through
> +   for_each_rtx.  Return 1 if a crtl->args.internal_arg_pointer based
> +   register is seen anywhere.  */
> +
> +static int
> +internal_arg_pointer_based_reg_1 (rtx *loc, void *data ATTRIBUTE_UNUSED)
> +{
> +  if (REG_P (*loc) && internal_arg_pointer_based_reg (*loc, false) !=
> NULL_RTX) +    return 1;
> +  if (MEM_P (*loc))
> +    return -1;
> +  return 0;
> +}

Missing comment for the MEM_P case.


> +/* If REG is based on crtl->args.internal_arg_pointer, return either
> +   a CONST_INT offset from crtl->args.internal_arg_pointer if
> +   offset from it is known constant, or PC if the offset is unknown.
> +   Return NULL_RTX if REG isn't based on crtl->args.internal_arg_pointer. 
> */ +
> +static rtx
> +internal_arg_pointer_based_reg (rtx reg, bool scan)
> +{
> +  rtx insn;
> +
> +  if (CONSTANT_P (reg))
> +    return NULL_RTX;
> +
> +  if (reg == crtl->args.internal_arg_pointer)
> +    return const0_rtx;
> +
> +  if (REG_P (reg) && REGNO (reg) < FIRST_PSEUDO_REGISTER)
> +    return NULL_RTX;

if (REG_P (reg) && HARD_REGISTER_P (reg))
  return NULL_RTX;

-- 
Eric Botcazou