public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Richard Sandiford <richard.sandiford@arm.com>
To: Evgeny Karpov <Evgeny.Karpov@microsoft.com>
Cc: "gcc-patches\@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
	 "ubizjak\@gmail.com" <ubizjak@gmail.com>,
	 "Richard Earnshaw \(lists\)" <Richard.Earnshaw@arm.com>,
	 Maxim Kuvyrkov <maxim.kuvyrkov@linaro.org>,
	 Radek Barton <radek.barton@microsoft.com>
Subject: Re: [PATCH v1 0/6] Add DLL import/export implementation to AArch64
Date: Wed, 05 Jun 2024 09:13:19 +0100	[thread overview]
Message-ID: <mpty17jvmcg.fsf@arm.com> (raw)
In-Reply-To: <DBBPR83MB061312C6CC8F13097B8F5376F8F82@DBBPR83MB0613.EURPRD83.prod.outlook.com> (Evgeny Karpov's message of "Tue, 4 Jun 2024 20:10:30 +0000")

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

      parent reply	other threads:[~2024-06-05  8:13 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-04-18 21:35 Evgeny Karpov
2024-04-18 21:41 ` [PATCH v1 1/6] Move mingw_* declarations to the mingw folder Evgeny Karpov
2024-04-18 21:43 ` [PATCH v1 2/6] Extract ix86 dllimport implementation to mingw Evgeny Karpov
2024-05-22 11:05   ` Richard Sandiford
2024-05-22 14:32     ` Evgeny Karpov
2024-05-23  8:35       ` Uros Bizjak
2024-05-23  8:41         ` Uros Bizjak
2024-05-23 17:53         ` Evgeny Karpov
2024-05-23 19:37           ` Uros Bizjak
2024-04-18 21:45 ` [PATCH v1 3/6] Rename functions for reuse in AArch64 Evgeny Karpov
2024-05-22 11:46   ` Richard Sandiford
2024-04-18 21:46 ` [PATCH v1 4/6] aarch64: Add selectany attribute handling Evgeny Karpov
2024-05-22 11:57   ` Richard Sandiford
2024-04-18 21:48 ` [PATCH v1 5/6] Adjust DLL import/export implementation for AArch64 Evgeny Karpov
2024-05-22 12:07   ` Richard Sandiford
2024-04-18 21:49 ` [PATCH v1 6/6] aarch64: Add DLL import/export to AArch64 target Evgeny Karpov
2024-06-04 20:10 ` [PATCH v1 0/6] Add DLL import/export implementation to AArch64 Evgeny Karpov
2024-06-05  6:01   ` Uros Bizjak
2024-06-05 23:41     ` Jonathan Yong
2024-06-06  9:23       ` Evgeny Karpov
2024-06-08  1:59         ` Jonathan Yong
2024-06-05  8:13   ` Richard Sandiford [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=mpty17jvmcg.fsf@arm.com \
    --to=richard.sandiford@arm.com \
    --cc=Evgeny.Karpov@microsoft.com \
    --cc=Richard.Earnshaw@arm.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=maxim.kuvyrkov@linaro.org \
    --cc=radek.barton@microsoft.com \
    --cc=ubizjak@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).