Is it OK for rtx_addr_can_trap_p_1 to attempt to compute the frame layout? (was Re: [PATCH] Skip re-computing the mips frame info after reload completed)

[Richard Sandiford <rdsandiford@googlemail.com>]

[cc-ing Eric as RTL maintainer]

Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>> Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
>> > Has the patch been tested beyond just building GCC? I can do a
>> > test run for you if you don't have things set up to do one yourself.
>> 
>> I built a cross-gcc with all languages and a cross-glibc, but I have
>> not set up an emulation environment, so if you could give it a test
>> that would be highly welcome.
>
> mipsel-linux-gnu test results are the same before and after this patch.
>
> Please go ahead and commit.

I still object to this.  And it feels like the patch was posted
as though it was a new one in order to avoid answering the objections
that were raised when it was last posted:

  https://gcc.gnu.org/ml/gcc-patches/2015-12/msg02218.html

IMO the problem is that rtx_addr_can_trap_p_1 duplicates a large
bit of LRA/reload logic:

------------------------------------------------------------------------
/* Compute an approximation for the offset between the register
   FROM and TO for the current function, as it was at the start
   of the routine.  */

static HOST_WIDE_INT
get_initial_register_offset (int from, int to)
{
#ifdef ELIMINABLE_REGS
  static const struct elim_table_t
  {
    const int from;
    const int to;
  } table[] = ELIMINABLE_REGS;
  HOST_WIDE_INT offset1, offset2;
  unsigned int i, j;

  if (to == from)
    return 0;

  /* It is not safe to call INITIAL_ELIMINATION_OFFSET
     before the reload pass.  We need to give at least
     an estimation for the resulting frame size.  */
  if (! reload_completed)
    {
      offset1 = crtl->outgoing_args_size + get_frame_size ();
#if !STACK_GROWS_DOWNWARD
      offset1 = - offset1;
#endif
      if (to == STACK_POINTER_REGNUM)
	return offset1;
      else if (from == STACK_POINTER_REGNUM)
	return - offset1;
      else
	return 0;
     }

  for (i = 0; i < ARRAY_SIZE (table); i++)
      if (table[i].from == from)
	{
	  if (table[i].to == to)
	    {
	      INITIAL_ELIMINATION_OFFSET (table[i].from, table[i].to,
					  offset1);
	      return offset1;
	    }
	  for (j = 0; j < ARRAY_SIZE (table); j++)
	    {
	      if (table[j].to == to
		  && table[j].from == table[i].to)
		{
		  INITIAL_ELIMINATION_OFFSET (table[i].from, table[i].to,
					      offset1);
		  INITIAL_ELIMINATION_OFFSET (table[j].from, table[j].to,
					      offset2);
		  return offset1 + offset2;
		}
	      if (table[j].from == to
		  && table[j].to == table[i].to)
		{
		  INITIAL_ELIMINATION_OFFSET (table[i].from, table[i].to,
					      offset1);
		  INITIAL_ELIMINATION_OFFSET (table[j].from, table[j].to,
					      offset2);
		  return offset1 - offset2;
		}
	    }
	}
      else if (table[i].to == from)
	{
	  if (table[i].from == to)
	    {
	      INITIAL_ELIMINATION_OFFSET (table[i].from, table[i].to,
					  offset1);
	      return - offset1;
	    }
	  for (j = 0; j < ARRAY_SIZE (table); j++)
	    {
	      if (table[j].to == to
		  && table[j].from == table[i].from)
		{
		  INITIAL_ELIMINATION_OFFSET (table[i].from, table[i].to,
					      offset1);
		  INITIAL_ELIMINATION_OFFSET (table[j].from, table[j].to,
					      offset2);
		  return - offset1 + offset2;
		}
	      if (table[j].from == to
		  && table[j].to == table[i].from)
		{
		  INITIAL_ELIMINATION_OFFSET (table[i].from, table[i].to,
					      offset1);
		  INITIAL_ELIMINATION_OFFSET (table[j].from, table[j].to,
					      offset2);
		  return - offset1 - offset2;
		}
	    }
	}

  /* If the requested register combination was not found,
     try a different more simple combination.  */
  if (from == ARG_POINTER_REGNUM)
    return get_initial_register_offset (HARD_FRAME_POINTER_REGNUM, to);
  else if (to == ARG_POINTER_REGNUM)
    return get_initial_register_offset (from, HARD_FRAME_POINTER_REGNUM);
  else if (from == HARD_FRAME_POINTER_REGNUM)
    return get_initial_register_offset (FRAME_POINTER_REGNUM, to);
  else if (to == HARD_FRAME_POINTER_REGNUM)
    return get_initial_register_offset (from, FRAME_POINTER_REGNUM);
  else
    return 0;

#else
  HOST_WIDE_INT offset;

  if (to == from)
    return 0;

  if (reload_completed)
    {
      INITIAL_FRAME_POINTER_OFFSET (offset);
    }
  else
    {
      offset = crtl->outgoing_args_size + get_frame_size ();
#if !STACK_GROWS_DOWNWARD
      offset = - offset;
#endif
    }

  if (to == STACK_POINTER_REGNUM)
    return offset;
  else if (from == STACK_POINTER_REGNUM)
    return - offset;
  else
    return 0;

#endif
}
------------------------------------------------------------------------

Under the current interface macros like INITIAL_ELIMINATION_OFFSET
are expected to trigger the calculation of the target's frame layout
(since you need that information to answer the question).
To me it seems wrong that we're attempting to call that sort of
macro in a query routine like rtx_addr_can_trap_p_1.

The frame layout is fixed for each LRA/reload cycle.  The layout
for the last of those cycles carries forward through the rest
of compilation.

IMO we should cache the information we need at the start of each
LRA/reload cycle.  This will be more robust, because there will
be no accidental changes to global state either during or after
LRA/reload.  It will also be more efficient because
rtx_addr_can_trap_p_1 can read the cached variables rather
than calling back into the target.

Thanks,
Richard