Re: [PATCH rs6000] remove implicit static var outputs of toc_relative_expr_p

[Segher Boessenkool <segher at kernel dot crashing dot org>]

Hi Aaron,

On Tue, Jun 27, 2017 at 11:43:57AM -0500, Aaron Sawdey wrote:
> The function toc_relative_expr_p implicitly sets two static vars
> (tocrel_base and tocrel_offset) that are declared in rs6000.c. The real
> purpose of this is to communicate between
> print_operand/print_operand_address and rs6000_output_addr_const_extra,
> which is called through the asm_out hook vector by something in the
> call tree under output_addr_const.
> 
> This patch changes toc_relative_expr_p to make tocrel_base and
> tocrel_offset be explicit const_rtx * args. All of the calls other than
> print_operand/print_operand_address are changed to have local const_rtx
> vars that are passed in.

If those locals aren't used, can you arrange to call toc_relative_expr_p
with NULL instead?  Or are they always used?

> The statics in rs6000.c are now called
> tocrel_base_oac and tocrel_offset_oac to reflect their use to
> communicate across output_addr_const, and that is now the only thing
> they are used for.

Can't say I like those names, very cryptical.  Not that I know something
better, the short names as they were weren't very nice either.

> --- gcc/config/rs6000/rs6000.c	(revision 249639)
> +++ gcc/config/rs6000/rs6000.c	(working copy)
> @@ -8628,18 +8628,25 @@
>  	  && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (base), Pmode));
>  }
>  
> -static const_rtx tocrel_base, tocrel_offset;
> +/* These are only used to pass through from print_operand/print_operand_address
> + * to rs6000_output_addr_const_extra over the intervening function 
> + * output_addr_const which is not target code.  */

No leading * in a block comment please.  (And you have a trailing space).

> +static const_rtx tocrel_base_oac, tocrel_offset_oac;
>  
>  /* Return true if OP is a toc pointer relative address (the output
>     of create_TOC_reference).  If STRICT, do not match non-split
> -   -mcmodel=large/medium toc pointer relative addresses.  */
> +   -mcmodel=large/medium toc pointer relative addresses.  Places base 
> +   and offset pieces in TOCREL_BASE and TOCREL_OFFSET respectively.  */

s/Places/Place/ (and another trailing space).

> -  tocrel_base = op;
> -  tocrel_offset = const0_rtx;
> +  *tocrel_base = op;
> +  *tocrel_offset = const0_rtx;
>    if (GET_CODE (op) == PLUS && add_cint_operand (XEXP (op, 1), GET_MODE (op)))
>      {
> -      tocrel_base = XEXP (op, 0);
> -      tocrel_offset = XEXP (op, 1);
> +      *tocrel_base = XEXP (op, 0);
> +      *tocrel_offset = XEXP (op, 1);
>      }

Maybe write this as

  if (GET_CODE (op) == PLUS && add_cint_operand (XEXP (op, 1), GET_MODE (op)))
    {
      *tocrel_base = XEXP (op, 0);
      *tocrel_offset = XEXP (op, 1);
    }
  else
    {
      *tocrel_base = op;
      *tocrel_offset = const0_rtx;
    }

or, if you allow NULL pointers,

  bool with_offset = GET_CODE (op) == PLUS
		     && add_cint_operand (XEXP (op, 1), GET_MODE (op));
  if (tocrel_base)
    *tocrel_base = with_offset ? XEXP (op, 0) : op;
  if (tocrel_offset)
    *tocrel_offset = with_offset ? XEXP (op, 1) : const0_rtx;

or such.

> -  return (GET_CODE (tocrel_base) == UNSPEC
> -	  && XINT (tocrel_base, 1) == UNSPEC_TOCREL);
> +  return (GET_CODE (*tocrel_base) == UNSPEC
> +	  && XINT (*tocrel_base, 1) == UNSPEC_TOCREL);

Well, and then you have this, so you need to assign tocrel_base to a local
as well.

>  legitimate_constant_pool_address_p (const_rtx x, machine_mode mode,
>  				    bool strict)
>  {
> -  return (toc_relative_expr_p (x, strict)
> +  const_rtx tocrel_base, tocrel_offset;
> +  return (toc_relative_expr_p (x, strict, &tocrel_base, &tocrel_offset)

For example here it seems nothing uses tocrel_base?

It is probably nicer to have a separate function for toc_relative_expr_p
and one to pull the base/offset out.  And maybe don't keep it cached for
the output function either?  It has all info it needs, right, the full
address RTX?  I don't think it is measurably slower to pull the address
apart an extra time?


Segher