Re: [PATCH] AArch64: Cleanup aarch64_classify_symbol

[Richard Sandiford <richard.sandiford@arm.com>]

Wilco Dijkstra via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> Hi Richard,
>
>> Just to check: I guess this part is an optimisation, because it
>> means that we can share the GOT entry with other TUs.  Is that right?
>> I think it would be worth having a comment either way, whatever the
>> rationale.  A couple of other very minor things:
>
> It's just to make the code simpler and more orthogonal - the size of the
> sequence and sharing of GOT/literal is identical in both cases.

Hmm, OK.  I guess it makes things more consistent in that sense
(PIC vs. non-PIC).  But on the other side it's making things less
internally consistent for non-PIC, since we don't use the GOT for
anything else there.  I guess in principle there's a danger that a
custom *-elf linker script might not bother to set up the .got properly,
on the assumption that it shouldn't be needed.

But there's not much in it either way, and I agree consistency with
LLVM is a factor too.

> Here is v2 with comments/spaces fixed up:
>
>
> Use a GOT indirection for extern weak symbols instead of a literal - this is the same as
> PIC/PIE and mirrors LLVM behaviour.  Ensure PIC/PIE use the same offset limits for symbols
> that don't use the GOT.
>
> Passes bootstrap and regress. OK for commit?
>
> ChangeLog:
> 2021-04-27  Wilco Dijkstra  <wdijkstr@arm.com>
>
>         * config/aarch64/aarch64.c (aarch64_classify_symbol): Use GOT for extern weak symbols.
>         Limit symbol offsets for non-GOT symbols with PIC/PIE.

OK, thanks.

Richard

>
> ---
>
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 82957dddbe42a7f907b2960294ac7f8abf7be2ff..641c83b479e76cbcc75b299eb7ae5f634d9db7cd 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -17854,7 +17854,14 @@ aarch64_classify_symbol (rtx x, HOST_WIDE_INT offset)
>  
>        switch (aarch64_cmodel)
>  	{
> +	case AARCH64_CMODEL_TINY_PIC:
>  	case AARCH64_CMODEL_TINY:
> +	  /* With -fPIC non-local symbols use the GOT.  For orthogonality
> +	     always use the GOT for extern weak symbols.  */
> +	  if ((flag_pic || SYMBOL_REF_WEAK (x))
> +	      && !aarch64_symbol_binds_local_p (x))
> +	    return SYMBOL_TINY_GOT;
> +
>  	  /* When we retrieve symbol + offset address, we have to make sure
>  	     the offset does not cause overflow of the final address.  But
>  	     we have no way of knowing the address of symbol at compile time
> @@ -17862,42 +17869,30 @@ aarch64_classify_symbol (rtx x, HOST_WIDE_INT offset)
>  	     symbol + offset is outside the addressible range of +/-1MB in the
>  	     TINY code model.  So we limit the maximum offset to +/-64KB and
>  	     assume the offset to the symbol is not larger than +/-(1MB - 64KB).
> -	     If offset_within_block_p is true we allow larger offsets.
> -	     Furthermore force to memory if the symbol is a weak reference to
> -	     something that doesn't resolve to a symbol in this module.  */
> -
> -	  if (SYMBOL_REF_WEAK (x) && !aarch64_symbol_binds_local_p (x))
> -	    return SYMBOL_FORCE_TO_MEM;
> +	     If offset_within_block_p is true we allow larger offsets.  */
>  	  if (!(IN_RANGE (offset, -0x10000, 0x10000)
>  		|| offset_within_block_p (x, offset)))
>  	    return SYMBOL_FORCE_TO_MEM;
>  
>  	  return SYMBOL_TINY_ABSOLUTE;
>  
> +
> +	case AARCH64_CMODEL_SMALL_SPIC:
> +	case AARCH64_CMODEL_SMALL_PIC:
>  	case AARCH64_CMODEL_SMALL:
> +	  if ((flag_pic || SYMBOL_REF_WEAK (x))
> +	      && !aarch64_symbol_binds_local_p (x))
> +	    return aarch64_cmodel == AARCH64_CMODEL_SMALL_SPIC
> +		    ? SYMBOL_SMALL_GOT_28K : SYMBOL_SMALL_GOT_4G;
> +
>  	  /* Same reasoning as the tiny code model, but the offset cap here is
>  	     1MB, allowing +/-3.9GB for the offset to the symbol.  */
> -
> -	  if (SYMBOL_REF_WEAK (x) && !aarch64_symbol_binds_local_p (x))
> -	    return SYMBOL_FORCE_TO_MEM;
>  	  if (!(IN_RANGE (offset, -0x100000, 0x100000)
>  		|| offset_within_block_p (x, offset)))
>  	    return SYMBOL_FORCE_TO_MEM;
>  
>  	  return SYMBOL_SMALL_ABSOLUTE;
>  
> -	case AARCH64_CMODEL_TINY_PIC:
> -	  if (!aarch64_symbol_binds_local_p (x))
> -	    return SYMBOL_TINY_GOT;
> -	  return SYMBOL_TINY_ABSOLUTE;
> -
> -	case AARCH64_CMODEL_SMALL_SPIC:
> -	case AARCH64_CMODEL_SMALL_PIC:
> -	  if (!aarch64_symbol_binds_local_p (x))
> -	    return (aarch64_cmodel == AARCH64_CMODEL_SMALL_SPIC
> -		    ?  SYMBOL_SMALL_GOT_28K : SYMBOL_SMALL_GOT_4G);
> -	  return SYMBOL_SMALL_ABSOLUTE;
> -
>  	case AARCH64_CMODEL_LARGE:
>  	  /* This is alright even in PIC code as the constant
>  	     pool reference is always PC relative and within