Re: [PATCH] AArch64: Cleanup aarch64_classify_symbol

[Andrew Pinski <pinskia@gmail.com>]

On Wed, Apr 28, 2021 at 9:50 AM Wilco Dijkstra via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
> 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.

I thought that was changed not to use the GOT on purpose.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63874

That is if the symbol is not declared in the TU, then using the GOT is
correct thing to do.
Is the testcase gcc.target/aarch64/pr63874.c still working or is not
testing the correct thing?

Thanks,
Andrew Pinski

>
> 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.
>
> ---
>
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 82957dddbe42a7f907b2960294ac7f8abf7be2ff..08aaea70d952fd62147b7f06ba781eb55e697d3a 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -17854,7 +17854,12 @@ aarch64_classify_symbol (rtx x, HOST_WIDE_INT offset)
>
>        switch (aarch64_cmodel)
>         {
> +       case AARCH64_CMODEL_TINY_PIC:
>         case AARCH64_CMODEL_TINY:
> +         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
> @@ -17865,39 +17870,29 @@ aarch64_classify_symbol (rtx x, HOST_WIDE_INT offset)
>              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 (!(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