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
