Re: [PATCH v1 0/6] Add DLL import/export implementation to AArch64

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

Evgeny Karpov <Evgeny.Karpov@microsoft.com> writes:
> Richard and Uros, could you please review the changes for v2?
> Additionally, we have detected an issue with GCC GC in winnt-dll.cc. The fix will be included in v2.

Would it be possible to have a more "purposeful" name than
CMODEL_IS_NOT_LARGE_OR_MEDIUM_PIC?  What's the property of
large and medium PIC that needs to be handled differently?

It'd be good to have the macro be a positive test rather than a negative
test, so that we don't end up with !IS_NOT_FOO when testing for FOO.

Otherwise it looks good to me.

I never fully reviewed 1/6 or 6/6, sorry.  My main comment there is
that it would be good to avoid including config/mingw/winnt.h and
config/mingw/winnt-dll.h in config/aarch64/aarch64-protos.h (or in
other common AArch64 code).  It's OK for common AArch64 code to
have hooks that can be filled in by OS-specific headers, but there
shouldn't be OS-specific includes or code in the common files themselves.

>>> -ix86_handle_selectany_attribute (tree *node, tree name, tree, int,
>>> +mingw_handle_selectany_attribute (tree *node, tree name, tree, int,
>>>  				 bool *no_add_attrs)
>
>> please reindent the parameters for the new name length.
>
> Richard, could you please clarify how it should be done?

The "bool" on the second line should be directly under the "tree"
on the first line (so one extra space before "bool").

Thanks,
Richard


> Thanks!
>
> Regards,
> Evgeny
>
>
> ---
>  gcc/config/aarch64/cygming.h   |  6 +++++
>  gcc/config/i386/cygming.h      |  6 +++++
>  gcc/config/i386/i386-expand.cc |  6 +++--
>  gcc/config/i386/i386-expand.h  |  2 --
>  gcc/config/i386/i386.cc        | 42 ++++++++++------------------------
>  gcc/config/i386/i386.h         |  2 ++
>  gcc/config/mingw/winnt-dll.cc  |  8 ++-----
>  gcc/config/mingw/winnt-dll.h   |  2 +-
>  8 files changed, 33 insertions(+), 41 deletions(-)
>
> diff --git a/gcc/config/aarch64/cygming.h b/gcc/config/aarch64/cygming.h
> index 4beebf9e093..0ff475754e0 100644
> --- a/gcc/config/aarch64/cygming.h
> +++ b/gcc/config/aarch64/cygming.h
> @@ -183,4 +183,10 @@ still needed for compilation.  */
>  #undef MAX_OFILE_ALIGNMENT
>  #define MAX_OFILE_ALIGNMENT (8192 * 8)
>  
> +#define CMODEL_IS_NOT_LARGE_OR_MEDIUM_PIC 0
> +
> +#define HAVE_64BIT_POINTERS 1
> +
> +#define GOT_ALIAS_SET mingw_GOT_alias_set ()
> +
>  #endif
> diff --git a/gcc/config/i386/cygming.h b/gcc/config/i386/cygming.h
> index ee01e6bb6ce..cd240533dbc 100644
> --- a/gcc/config/i386/cygming.h
> +++ b/gcc/config/i386/cygming.h
> @@ -469,3 +469,9 @@ do {						\
>  #ifndef HAVE_GAS_ALIGNED_COMM
>  # define HAVE_GAS_ALIGNED_COMM 0
>  #endif
> +
> +#define CMODEL_IS_NOT_LARGE_OR_MEDIUM_PIC ix86_cmodel != CM_LARGE_PIC && ix86_cmodel != CM_MEDIUM_PIC
> +
> +#define HAVE_64BIT_POINTERS TARGET_64BIT_DEFAULT
> +
> +#define GOT_ALIAS_SET mingw_GOT_alias_set ()
> diff --git a/gcc/config/i386/i386-expand.cc b/gcc/config/i386/i386-expand.cc
> index fb460e30d0a..267d0ba257b 100644
> --- a/gcc/config/i386/i386-expand.cc
> +++ b/gcc/config/i386/i386-expand.cc
> @@ -408,11 +408,12 @@ ix86_expand_move (machine_mode mode, rtx operands[])
>  				 : UNSPEC_GOT));
>  	  op1 = gen_rtx_CONST (Pmode, op1);
>  	  op1 = gen_const_mem (Pmode, op1);
> -	  set_mem_alias_set (op1, ix86_GOT_alias_set ());
> +	  set_mem_alias_set (op1, GOT_ALIAS_SET);
>  	}
>        else
>  	{
> -	  tmp = ix86_legitimize_pe_coff_symbol (op1, addend != NULL_RTX);
> +#if TARGET_PECOFF
> +	  tmp = legitimize_pe_coff_symbol (op1, addend != NULL_RTX);
>  	  if (tmp)
>  	    {
>  	      op1 = tmp;
> @@ -424,6 +425,7 @@ ix86_expand_move (machine_mode mode, rtx operands[])
>  	      op1 = operands[1];
>  	      break;
>  	    }
> +#endif
>  	}
>  
>        if (addend)
> diff --git a/gcc/config/i386/i386-expand.h b/gcc/config/i386/i386-expand.h
> index a8c20993954..5e02df1706d 100644
> --- a/gcc/config/i386/i386-expand.h
> +++ b/gcc/config/i386/i386-expand.h
> @@ -34,9 +34,7 @@ struct expand_vec_perm_d
>  };
>  
>  rtx legitimize_tls_address (rtx x, enum tls_model model, bool for_mov);
> -alias_set_type ix86_GOT_alias_set (void);
>  rtx legitimize_pic_address (rtx orig, rtx reg);
> -rtx ix86_legitimize_pe_coff_symbol (rtx addr, bool inreg);
>  
>  bool insn_defines_reg (unsigned int regno1, unsigned int regno2,
>  		       rtx_insn *insn);
> diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc
> index 66845b30446..ee3a59ed498 100644
> --- a/gcc/config/i386/i386.cc
> +++ b/gcc/config/i386/i386.cc
> @@ -11807,30 +11807,6 @@ constant_address_p (rtx x)
>  }
>  
>
>  
> -#if TARGET_PECOFF
> -rtx ix86_legitimize_pe_coff_symbol (rtx addr, bool inreg)
> -{
> -  return legitimize_pe_coff_symbol (addr, inreg);
> -}
> -
> -alias_set_type
> -ix86_GOT_alias_set (void)
> -{
> -  return mingw_GOT_alias_set ();
> -}
> -#else
> -rtx ix86_legitimize_pe_coff_symbol (rtx addr, bool inreg)
> -{
> -  return NULL_RTX;
> -}
> -
> -alias_set_type
> -ix86_GOT_alias_set (void)
> -{
> -  return -1;
> -}
> -#endif
> -
>  /* Return a legitimate reference for ORIG (an address) using the
>     register REG.  If REG is 0, a new pseudo is generated.
>  
> @@ -11867,9 +11843,11 @@ legitimize_pic_address (rtx orig, rtx reg)
>  
>    if (TARGET_64BIT && TARGET_DLLIMPORT_DECL_ATTRIBUTES)
>      {
> -      rtx tmp = ix86_legitimize_pe_coff_symbol (addr, true);
> +#if TARGET_PECOFF
> +      rtx tmp = legitimize_pe_coff_symbol (addr, true);
>        if (tmp)
>          return tmp;
> +#endif
>      }
>  
>    if (TARGET_64BIT && legitimate_pic_address_disp_p (addr))
> @@ -11912,9 +11890,11 @@ legitimize_pic_address (rtx orig, rtx reg)
>  	      on VxWorks, see gotoff_operand.  */
>  	   || (TARGET_VXWORKS_RTP && GET_CODE (addr) == LABEL_REF))
>      {
> -      rtx tmp = ix86_legitimize_pe_coff_symbol (addr, true);
> +#if TARGET_PECOFF
> +      rtx tmp = legitimize_pe_coff_symbol (addr, true);
>        if (tmp)
>          return tmp;
> +#endif
>  
>        /* For x64 PE-COFF there is no GOT table,
>  	 so we use address directly.  */
> @@ -11929,7 +11909,7 @@ legitimize_pic_address (rtx orig, rtx reg)
>  				    UNSPEC_GOTPCREL);
>  	  new_rtx = gen_rtx_CONST (Pmode, new_rtx);
>  	  new_rtx = gen_const_mem (Pmode, new_rtx);
> -	  set_mem_alias_set (new_rtx, ix86_GOT_alias_set ());
> +	  set_mem_alias_set (new_rtx, GOT_ALIAS_SET);
>  	}
>        else
>  	{
> @@ -11951,7 +11931,7 @@ legitimize_pic_address (rtx orig, rtx reg)
>  	    new_rtx = gen_rtx_PLUS (Pmode, pic_offset_table_rtx, new_rtx);
>  
>  	  new_rtx = gen_const_mem (Pmode, new_rtx);
> -	  set_mem_alias_set (new_rtx, ix86_GOT_alias_set ());
> +	  set_mem_alias_set (new_rtx, GOT_ALIAS_SET);
>  	}
>  
>        new_rtx = copy_to_suggested_reg (new_rtx, reg, Pmode);
> @@ -12328,7 +12308,7 @@ legitimize_tls_address (rtx x, enum tls_model model, bool for_mov)
>        if (pic)
>  	off = gen_rtx_PLUS (tp_mode, pic, off);
>        off = gen_const_mem (tp_mode, off);
> -      set_mem_alias_set (off, ix86_GOT_alias_set ());
> +      set_mem_alias_set (off, GOT_ALIAS_SET);
>  
>        if (TARGET_64BIT || TARGET_ANY_GNU_TLS)
>  	{
> @@ -12526,9 +12506,11 @@ ix86_legitimize_address (rtx x, rtx, machine_mode mode)
>  
>    if (TARGET_DLLIMPORT_DECL_ATTRIBUTES)
>      {
> -      rtx tmp = ix86_legitimize_pe_coff_symbol (x, true);
> +#if TARGET_PECOFF
> +      rtx tmp = legitimize_pe_coff_symbol (x, true);
>        if (tmp)
>          return tmp;
> +#endif
>      }
>  
>    if (flag_pic && SYMBOLIC_CONST (x))
> diff --git a/gcc/config/i386/i386.h b/gcc/config/i386/i386.h
> index 969391d3013..2b3d0c8db75 100644
> --- a/gcc/config/i386/i386.h
> +++ b/gcc/config/i386/i386.h
> @@ -2261,6 +2261,8 @@ extern int const svr4_debugger_register_map[FIRST_PSEUDO_REGISTER];
>  /* Which processor to tune code generation for.  These must be in sync
>     with processor_cost_table in i386-options.cc.  */
>  
> +#define GOT_ALIAS_SET -1
> +
>  enum processor_type
>  {
>    PROCESSOR_GENERIC = 0,
> diff --git a/gcc/config/mingw/winnt-dll.cc b/gcc/config/mingw/winnt-dll.cc
> index 591e89cb5c9..c91e95225d3 100644
> --- a/gcc/config/mingw/winnt-dll.cc
> +++ b/gcc/config/mingw/winnt-dll.cc
> @@ -1,6 +1,6 @@
>  /* Expand a SYMBOL into its corresponding dllimport, far-address,
>  or refptr symbol.
> -Copyright (C) 2024 Free Software Foundation, Inc.
> +Copyright (C) 1988-2024 Free Software Foundation, Inc.
>  
>  GCC is free software; you can redistribute it and/or modify it under
>  the terms of the GNU General Public License as published by the Free
> @@ -206,13 +206,9 @@ legitimize_pe_coff_symbol (rtx addr, bool inreg)
>  	}
>      }
>  
> -#if !defined (TARGET_AARCH64_MS_ABI)
> -
> -  if (ix86_cmodel != CM_LARGE_PIC && ix86_cmodel != CM_MEDIUM_PIC)
> +  if (CMODEL_IS_NOT_LARGE_OR_MEDIUM_PIC)
>      return NULL_RTX;
>  
> -#endif
> -
>    if (GET_CODE (addr) == SYMBOL_REF
>        && !is_imported_p (addr)
>        && SYMBOL_REF_EXTERNAL_P (addr)
> diff --git a/gcc/config/mingw/winnt-dll.h b/gcc/config/mingw/winnt-dll.h
> index 19c16e747a2..0877f10d67e 100644
> --- a/gcc/config/mingw/winnt-dll.h
> +++ b/gcc/config/mingw/winnt-dll.h
> @@ -23,4 +23,4 @@ extern bool is_imported_p (rtx x);
>  extern alias_set_type mingw_GOT_alias_set (void);
>  extern rtx legitimize_pe_coff_symbol (rtx addr, bool inreg);
>  
> -#endif
> \ No newline at end of file
> +#endif