Re: [PATCH], PowerPC, Patch #6, Create pc-relative addressing insns

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi Mike,

On Tue, Jul 09, 2019 at 06:30:00PM -0400, Michael Meissner wrote:
> @@ -8760,12 +8762,34 @@ rs6000_cannot_force_const_mem (machine_m
>  static bool
>  use_toc_relative_ref (rtx sym, machine_mode mode)
>  {
> -  return ((constant_pool_expr_p (sym)
> -	   && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (sym),
> -					       get_pool_mode (sym)))
> -	  || (TARGET_CMODEL == CMODEL_MEDIUM
> -	      && SYMBOL_REF_LOCAL_P (sym)
> -	      && GET_MODE_SIZE (mode) <= POWERPC64_TOC_POINTER_ALIGNMENT));
> +  if (!SYMBOL_REF_P (sym) || TARGET_PCREL || !TARGET_TOC)
> +    return false;

Why the SYMBOL_REF test?  The original didn't have any.  But its comment
says
  /* Return true iff the given SYMBOL_REF refers to a constant pool entry
     that we have put in the TOC, or for cmodel=medium, if the SYMBOL_REF
     can be addressed relative to the toc pointer.  */
so perhaps you want an assert instead.

> +
> +  if (constant_pool_expr_p (sym)
> +      && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (sym),
> +					  get_pool_mode (sym)))
> +    return true;
> +
> +  return (TARGET_CMODEL == CMODEL_MEDIUM && SYMBOL_REF_LOCAL_P (sym)
> +	  && GET_MODE_SIZE (mode) <= POWERPC64_TOC_POINTER_ALIGNMENT);

Please don't put disparate things on one line, it was fine before.

> +/* Return true iff the given SYMBOL_REF refers to a constant pool entry that we
> +   have put in the pc-relative constant area, or for cmodel=medium, if the
> +   SYMBOL_REF can be addressed via pc-relative instructions.  */
> +
> +static bool
> +use_pc_relative_ref (rtx sym)
> +{
> +  if (!SYMBOL_REF_P (sym) || !TARGET_PCREL)
> +    return false;

Same here, assert please.  Or nothing, it will ICE not much later anyway.
But don't silently return if something violates the call contract.

> -static GTY(()) alias_set_type set = -1;
> +/* Return the alias set for constants stored in either the TOC or via
> +   pc-relative addressing.  */
> +static GTY(()) alias_set_type data_alias_set = -1;

Please just make a separate alias set for pcrel.  The new name isn't as
bad as just "set" :-), but "data"?  That's not great either.  Conflating
toc and pcrel isn't a good thing.

(Variables don't "return" anything btw).

>  
>  alias_set_type
> -get_TOC_alias_set (void)
> +get_data_alias_set (void)

This name is much worse than the old one.  Just make separate functions
for TOC and for pcrel?  Or is there any reason you want to share them?


Segher