diff mbox series

[v1,03/13] aarch64: Mark x18 register as a fixed register for MS ABI

Message ID VI1PR83MB0431809D3F653B39481E740DF8572@VI1PR83MB0431.EURPRD83.prod.outlook.com
State New
Headers
Series Add aarch64-w64-mingw32 target |

Commit Message

Evgeny Karpov Feb. 21, 2024, 6:30 p.m. UTC 
  From 72ca3f49e3eef9b18946b8d4e77019c1441e1a97 Mon Sep 17 00:00:00 2001
From: Zac Walker <zacwalker@microsoft.com>
Date: Tue, 20 Feb 2024 15:30:33 +0100
Subject: [PATCH v1 03/13] aarch64: Mark x18 register as a fixed register for
 MS ABI

Define the MS ABI for aarch64-w64-mingw32.
Adjust FIXED_REGISTERS and STATIC_CHAIN_REGNUM for different ABIs.
The X18 register is reserved on Windows for the TEB.

gcc/ChangeLog:

	* config.gcc: Define TARGET_ARM64_MS_ABI when Arm64 MS ABI is
	used.
	* config/aarch64/aarch64.h (FIXED_X18): Define if X18
	regsiter is fixed.
	(CALL_USED_X18): Define if X18 register is call used.
	(FIXED_REGISTERS): Adjust FIXED_REGISTERS for different ABIs.
	(STATIC_CHAIN_REGNUM): Define STATIC_CHAIN_REGNUM acording to
	ABI.
---
 gcc/config.gcc               |  1 +
 gcc/config/aarch64/aarch64.h | 19 ++++++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

Comments

Richard Earnshaw (lists) Feb. 22, 2024, 11:55 a.m. UTC | #1 
On 21/02/2024 18:30, Evgeny Karpov wrote:
> 
+/* X18 reserved for the TEB on Windows.  */
+#ifdef TARGET_ARM64_MS_ABI
+# define FIXED_X18 1
+# define CALL_USED_X18 0
+#else
+# define FIXED_X18 0
+# define CALL_USED_X18 1
+#endif

I'm not overly keen on ifdefs like this (and the one below), it can get quite confusing if we have to support more than a couple of ABIs.  Perhaps we could create a couple of new headers, one for the EABI (which all existing targets would then need to include) and one for the MS ABI.  Then the mingw port would use that instead of the EABI header.

An alternative is to make all this dynamic, based on the setting of the aarch64_calling_abi enum and to make the adjustments in aarch64_conditional_register_usage.

+# define CALL_USED_X18 0

Is that really correct?  If the register is really reserved, but some code modifies it anyway, this will cause the compiler to restore the old value at the end of a function; generally, for a reserved register, code that knows what it's doing would want to make permanent changes to this value.

+#ifdef TARGET_ARM64_MS_ABI
+# define STATIC_CHAIN_REGNUM		R17_REGNUM
+#else
+# define STATIC_CHAIN_REGNUM		R18_REGNUM
+#endif

If we went the enum way, we'd want something like

#define STATIC_CHAIN_REGNUM (calling_abi == AARCH64_CALLING_ABI_MS ? R17_REGNUM : R18_REGNUM)

R.
Richard Earnshaw (lists) Feb. 22, 2024, 1:11 p.m. UTC | #2 
On 21/02/2024 18:30, Evgeny Karpov wrote:
> 
+	tm_defines="${tm_defines} TARGET_ARM64_MS_ABI=1"

I missed this on first reading...

The GCC port name uses AARCH64, please use that internally rather than other names.  The only time when we should be using ARM64 is when it's needed for compatibility with other compilers and that doesn't apply here AFAICT.

R.
Evgeny Karpov Feb. 22, 2024, 3:06 p.m. UTC | #3 
Hi Richard,

Thanks for the review!

TARGET_ARM64_MS_ABI refers to the official Microsoft ARM64 ABI naming used for the target. 
If AARCH64 is a more preferred name, it will be changed in PATCH v2.

https://learn.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions?view=msvc-170

Regards,
Evgeny

-----Original Message-----
Thursday, February 22, 2024 2:11 PM
Richard Earnshaw (lists) wrote:

On 21/02/2024 18:30, Evgeny Karpov wrote:
>
+       tm_defines="${tm_defines} TARGET_ARM64_MS_ABI=1"

I missed this on first reading...

The GCC port name uses AARCH64, please use that internally rather than other names.  The only time when we should be using ARM64 is when it's needed for compatibility with other compilers and that doesn't apply here AFAICT.

R.
Andrew Pinski Feb. 22, 2024, 5:45 p.m. UTC | #4 
On Thu, Feb 22, 2024 at 3:56 AM Richard Earnshaw (lists)
<Richard.Earnshaw@arm.com> wrote:
>
> On 21/02/2024 18:30, Evgeny Karpov wrote:
> >
> +/* X18 reserved for the TEB on Windows.  */
> +#ifdef TARGET_ARM64_MS_ABI
> +# define FIXED_X18 1
> +# define CALL_USED_X18 0
> +#else
> +# define FIXED_X18 0
> +# define CALL_USED_X18 1
> +#endif
>
> I'm not overly keen on ifdefs like this (and the one below), it can get quite confusing if we have to support more than a couple of ABIs.  Perhaps we could create a couple of new headers, one for the EABI (which all existing targets would then need to include) and one for the MS ABI.  Then the mingw port would use that instead of the EABI header.
>
> An alternative is to make all this dynamic, based on the setting of the aarch64_calling_abi enum and to make the adjustments in aarch64_conditional_register_usage.

Dynamically might be needed also if we want to support ms_abi
attribute and/or -mabi=ms to support the wine folks.

Thanks,
Andrew Pinski

>
> +# define CALL_USED_X18 0
>
> Is that really correct?  If the register is really reserved, but some code modifies it anyway, this will cause the compiler to restore the old value at the end of a function; generally, for a reserved register, code that knows what it's doing would want to make permanent changes to this value.
>
> +#ifdef TARGET_ARM64_MS_ABI
> +# define STATIC_CHAIN_REGNUM           R17_REGNUM
> +#else
> +# define STATIC_CHAIN_REGNUM           R18_REGNUM
> +#endif
>
> If we went the enum way, we'd want something like
>
> #define STATIC_CHAIN_REGNUM (calling_abi == AARCH64_CALLING_ABI_MS ? R17_REGNUM : R18_REGNUM)
>
> R.
Iain Sandoe Feb. 22, 2024, 6:15 p.m. UTC | #5 
> On 22 Feb 2024, at 17:45, Andrew Pinski <pinskia@gmail.com> wrote:
> 
> On Thu, Feb 22, 2024 at 3:56 AM Richard Earnshaw (lists)
> <Richard.Earnshaw@arm.com> wrote:
>> 
>> On 21/02/2024 18:30, Evgeny Karpov wrote:
>>> 
>> +/* X18 reserved for the TEB on Windows.  */
>> +#ifdef TARGET_ARM64_MS_ABI
>> +# define FIXED_X18 1
>> +# define CALL_USED_X18 0
>> +#else
>> +# define FIXED_X18 0
>> +# define CALL_USED_X18 1
>> +#endif
>> 
>> I'm not overly keen on ifdefs like this (and the one below), it can get quite confusing if we have to support more than a couple of ABIs.  Perhaps we could create a couple of new headers, one for the EABI (which all existing targets would then need to include) and one for the MS ABI.  Then the mingw port would use that instead of the EABI header.
>> 
>> An alternative is to make all this dynamic, based on the setting of the aarch64_calling_abi enum and to make the adjustments in aarch64_conditional_register_usage.
> 
> Dynamically might be needed also if we want to support ms_abi
> attribute and/or -mabi=ms to support the wine folks.

X18 is also reserved on Darwin - in my current branch I have it non-dynamic too.
Iain

> 
> Thanks,
> Andrew Pinski
> 
>> 
>> +# define CALL_USED_X18 0
>> 
>> Is that really correct?  If the register is really reserved, but some code modifies it anyway, this will cause the compiler to restore the old value at the end of a function; generally, for a reserved register, code that knows what it's doing would want to make permanent changes to this value.
>> 
>> +#ifdef TARGET_ARM64_MS_ABI
>> +# define STATIC_CHAIN_REGNUM           R17_REGNUM
>> +#else
>> +# define STATIC_CHAIN_REGNUM           R18_REGNUM
>> +#endif
>> 
>> If we went the enum way, we'd want something like
>> 
>> #define STATIC_CHAIN_REGNUM (calling_abi == AARCH64_CALLING_ABI_MS ? R17_REGNUM : R18_REGNUM)
>> 
>> R.
Jacek Caban Feb. 23, 2024, 4:10 p.m. UTC | #6 
On 22.02.2024 18:45, Andrew Pinski wrote:
> On Thu, Feb 22, 2024 at 3:56 AM Richard Earnshaw (lists)
> <Richard.Earnshaw@arm.com> wrote:
>> On 21/02/2024 18:30, Evgeny Karpov wrote:
>> +/* X18 reserved for the TEB on Windows.  */
>> +#ifdef TARGET_ARM64_MS_ABI
>> +# define FIXED_X18 1
>> +# define CALL_USED_X18 0
>> +#else
>> +# define FIXED_X18 0
>> +# define CALL_USED_X18 1
>> +#endif
>>
>> I'm not overly keen on ifdefs like this (and the one below), it can get quite confusing if we have to support more than a couple of ABIs.  Perhaps we could create a couple of new headers, one for the EABI (which all existing targets would then need to include) and one for the MS ABI.  Then the mingw port would use that instead of the EABI header.
>>
>> An alternative is to make all this dynamic, based on the setting of the aarch64_calling_abi enum and to make the adjustments in aarch64_conditional_register_usage.
> Dynamically might be needed also if we want to support ms_abi
> attribute and/or -mabi=ms to support the wine folks.


Wine no longer needs ms_abi, it was needed for PE-in-ELF modules in the 
past. We use use proper PE files now, so we need a cross compiler, but 
no special attributes. aarch64-w64-mingw32 is already well supported by 
Wine when using llvm-mingw, so as soon as GCC properly supports the ABI, 
Wine should just work with it, in theory. I didn't try it, but I don't 
see things like vararg support in this patchset nor in the repo, so I 
assume it won't work yet.


Thanks for the work!

Jacek
Richard Sandiford Feb. 23, 2024, 4:56 p.m. UTC | #7 
"Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com> writes:
> On 21/02/2024 18:30, Evgeny Karpov wrote:
>> 
> +/* X18 reserved for the TEB on Windows.  */
> +#ifdef TARGET_ARM64_MS_ABI
> +# define FIXED_X18 1
> +# define CALL_USED_X18 0
> +#else
> +# define FIXED_X18 0
> +# define CALL_USED_X18 1
> +#endif
>
> I'm not overly keen on ifdefs like this (and the one below), it can get quite confusing if we have to support more than a couple of ABIs.  Perhaps we could create a couple of new headers, one for the EABI (which all existing targets would then need to include) and one for the MS ABI.  Then the mingw port would use that instead of the EABI header.
>
> An alternative is to make all this dynamic, based on the setting of the aarch64_calling_abi enum and to make the adjustments in aarch64_conditional_register_usage.

Agreed FWIW.

> +# define CALL_USED_X18 0
>
> Is that really correct?  If the register is really reserved, but some code modifies it anyway, this will cause the compiler to restore the old value at the end of a function; generally, for a reserved register, code that knows what it's doing would want to make permanent changes to this value.

I don't think it would do that for fixed registers.  For those this
is more whether calls are allowed to change the value of x18 or whether
x18 is supposed to remain fixed (e.g. set at the start of the thread and
not changed thereafter).

How does the MS ABI use this register?  Same question for Darwin I suppose.

Thanks,
Richard

>
> +#ifdef TARGET_ARM64_MS_ABI
> +# define STATIC_CHAIN_REGNUM		R17_REGNUM
> +#else
> +# define STATIC_CHAIN_REGNUM		R18_REGNUM
> +#endif
>
> If we went the enum way, we'd want something like
>
> #define STATIC_CHAIN_REGNUM (calling_abi == AARCH64_CALLING_ABI_MS ? R17_REGNUM : R18_REGNUM)
>
> R.
Evgeny Karpov Feb. 26, 2024, 4:07 p.m. UTC | #8 
This change has been refactored based on the review and will be included in v2.

Original aarch64.h will remain unchanged, and the required definition for MS ABI will be implemented in aarch64-abi-ms.h. The reference to this file will be added to config.gcc.
This change has been verified, and it appears to work correctly.

The X18 register should not be used in any case.
The call used flag should be set to 0 for the X18 register to prevent its use in function calls.
The same applies to the static chain. The X17 register will be used instead of X18.

Regards,
Evgeny


gcc/config.gcc
@@ -1263,7 +1263,8 @@

aarch64-*-mingw*)
+       tm_file="${tm_file} aarch64/aarch64-abi-ms.h"



gcc/config/aarch64/aarch64-abi-ms.h
/* Copyright */

/* X18 reserved for the TEB on Windows.  */

#undef FIXED_REGISTERS
#define FIXED_REGISTERS	                            \
  {					                                \
    0, 0, 0, 0,   0, 0, 0, 0,	/* R0 - R7 */		\
    0, 0, 0, 0,   0, 0, 0, 0,	/* R8 - R15 */		\
    0, 0, 1, 0,   0, 0, 0, 0,	/* R16 - R23.  */	\
    0, 0, 0, 0,   0, 1, 0, 1,	/* R24 - R30, SP */	\
    0, 0, 0, 0,   0, 0, 0, 0,   /* V0 - V7 */       \
    0, 0, 0, 0,   0, 0, 0, 0,   /* V8 - V15 */		\
    0, 0, 0, 0,   0, 0, 0, 0,   /* V16 - V23 */     \
    0, 0, 0, 0,   0, 0, 0, 0,   /* V24 - V31 */     \
    1, 1, 1, 1,			/* SFP, AP, CC, VG */       \
    0, 0, 0, 0,   0, 0, 0, 0,   /* P0 - P7 */       \
    0, 0, 0, 0,   0, 0, 0, 0,   /* P8 - P15 */      \
    1, 1,			/* FFR and FFRT */	            \
    1, 1, 1, 1, 1, 1, 1, 1	/* Fake registers */	\
  }

#undef CALL_REALLY_USED_REGISTERS
#define CALL_REALLY_USED_REGISTERS                  \
  {						                            \
    1, 1, 1, 1,   1, 1, 1, 1,	/* R0 - R7 */		\
    1, 1, 1, 1,   1, 1, 1, 1,	/* R8 - R15 */		\
    1, 1, 0, 0,   0, 0, 0, 0,   /* R16 - R23.  */   \
    0, 0, 0, 0,   0, 1, 1, 1,	/* R24 - R30, SP */	\
    1, 1, 1, 1,   1, 1, 1, 1,	/* V0 - V7 */		\
    0, 0, 0, 0,   0, 0, 0, 0,	/* V8 - V15 */		\
    1, 1, 1, 1,   1, 1, 1, 1,   /* V16 - V23 */     \
    1, 1, 1, 1,   1, 1, 1, 1,   /* V24 - V31 */     \
    1, 1, 1, 0,			/* SFP, AP, CC, VG */       \
    1, 1, 1, 1,   1, 1, 1, 1,	/* P0 - P7 */       \
    1, 1, 1, 1,   1, 1, 1, 1,	/* P8 - P15 */      \
    1, 1,			/* FFR and FFRT */              \
    0, 0, 0, 0, 0, 0, 0, 0	/* Fake registers */	\
  }

#undef  STATIC_CHAIN_REGNUM
#define STATIC_CHAIN_REGNUM		R17_REGNUM

 

-----Original Message-----
Thursday, February 22, 2024 12:55 PM 
Richard Earnshaw (lists) wrote:

+/* X18 reserved for the TEB on Windows.  */ #ifdef TARGET_ARM64_MS_ABI 
+# define FIXED_X18 1 # define CALL_USED_X18 0 #else # define FIXED_X18 
+0 # define CALL_USED_X18 1 #endif

I'm not overly keen on ifdefs like this (and the one below), it can get quite confusing if we have to support more than a couple of ABIs.  Perhaps we could create a couple of new headers, one for the EABI (which all existing targets would then need to include) and one for the MS ABI.  Then the mingw port would use that instead of the EABI header.

An alternative is to make all this dynamic, based on the setting of the aarch64_calling_abi enum and to make the adjustments in aarch64_conditional_register_usage.

+# define CALL_USED_X18 0

Is that really correct?  If the register is really reserved, but some code modifies it anyway, this will cause the compiler to restore the old value at the end of a function; generally, for a reserved register, code that knows what it's doing would want to make permanent changes to this value.

+#ifdef TARGET_ARM64_MS_ABI
+# define STATIC_CHAIN_REGNUM           R17_REGNUM
+#else
+# define STATIC_CHAIN_REGNUM           R18_REGNUM
+#endif

If we went the enum way, we'd want something like

#define STATIC_CHAIN_REGNUM (calling_abi == AARCH64_CALLING_ABI_MS ? R17_REGNUM : R18_REGNUM)

R.
Evgeny Karpov Feb. 26, 2024, 4:34 p.m. UTC | #9 
Thank you Jacek for clarifying Wine's needs for ms_abi!
Work on vararg support is planned.

Regards,
Evgeny

-----Original Message-----
Friday, February 23, 2024 5:10 PM 
Jacek Caban wrote:

> Dynamically might be needed also if we want to support ms_abi 
> attribute and/or -mabi=ms to support the wine folks.

Wine no longer needs ms_abi, it was needed for PE-in-ELF modules in the past. We use use proper PE files now, so we need a cross compiler, but no special attributes. aarch64-w64-mingw32 is already well supported by Wine when using llvm-mingw, so as soon as GCC properly supports the ABI, Wine should just work with it, in theory. I didn't try it, but I don't see things like vararg support in this patchset nor in the repo, so I assume it won't work yet.


Thanks for the work!

Jacek
Evgeny Karpov Feb. 26, 2024, 5:03 p.m. UTC | #10 
In user mode on Windows, it points to TEB (Thread Environment Block).
more information here
https://learn.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions?view=msvc-170#integer-registers
https://learn.microsoft.com/en-us/windows/win32/api/winternl/ns-winternl-teb

Regards,
Evgeny

-----Original Message-----
Friday, February 23, 2024 5:56 PM
Richard Sandiford wrote:

How does the MS ABI use this register?  Same question for Darwin I suppose.

Thanks,
Richard
diff mbox series

Patch

diff --git a/gcc/config.gcc b/gcc/config.gcc
index 092a091595d..2a9e4c44f50 100644
--- a/gcc/config.gcc
+++ b/gcc/config.gcc
@@ -1276,6 +1276,7 @@  aarch64*-*-mingw*)
 	default_use_cxa_atexit=yes
 	need_64bit_isa=yes
 	user_headers_inc_next_post="${user_headers_inc_next_post} float.h"
+	tm_defines="${tm_defines} TARGET_ARM64_MS_ABI=1"
 	;;
 aarch64*-wrs-vxworks*)
         tm_file="${tm_file} elfos.h aarch64/aarch64-elf.h"
diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index 45e901cda64..36916e7a97d 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -536,11 +536,20 @@  constexpr auto AARCH64_FL_DEFAULT_ISA_MODE = AARCH64_FL_SM_OFF;
    register.  GCC internally uses the poly_int variable aarch64_sve_vg
    instead.  */
 
+/* X18 reserved for the TEB on Windows.  */
+#ifdef TARGET_ARM64_MS_ABI
+# define FIXED_X18 1
+# define CALL_USED_X18 0
+#else
+# define FIXED_X18 0
+# define CALL_USED_X18 1
+#endif
+
 #define FIXED_REGISTERS					\
   {							\
     0, 0, 0, 0,   0, 0, 0, 0,	/* R0 - R7 */		\
     0, 0, 0, 0,   0, 0, 0, 0,	/* R8 - R15 */		\
-    0, 0, 0, 0,   0, 0, 0, 0,	/* R16 - R23 */		\
+    0, 0, FIXED_X18, 0,   0, 0, 0, 0,	/* R16 - R23.  */	\
     0, 0, 0, 0,   0, 1, 0, 1,	/* R24 - R30, SP */	\
     0, 0, 0, 0,   0, 0, 0, 0,   /* V0 - V7 */           \
     0, 0, 0, 0,   0, 0, 0, 0,   /* V8 - V15 */		\
@@ -564,7 +573,7 @@  constexpr auto AARCH64_FL_DEFAULT_ISA_MODE = AARCH64_FL_SM_OFF;
   {							\
     1, 1, 1, 1,   1, 1, 1, 1,	/* R0 - R7 */		\
     1, 1, 1, 1,   1, 1, 1, 1,	/* R8 - R15 */		\
-    1, 1, 1, 0,   0, 0, 0, 0,	/* R16 - R23 */		\
+    1, 1, CALL_USED_X18, 0, 0,   0, 0, 0, /* R16 - R23.  */   \
     0, 0, 0, 0,   0, 1, 1, 1,	/* R24 - R30, SP */	\
     1, 1, 1, 1,   1, 1, 1, 1,	/* V0 - V7 */		\
     0, 0, 0, 0,   0, 0, 0, 0,	/* V8 - V15 */		\
@@ -642,7 +651,11 @@  constexpr auto AARCH64_FL_DEFAULT_ISA_MODE = AARCH64_FL_SM_OFF;
    uses alloca.  */
 #define EXIT_IGNORE_STACK	(cfun->calls_alloca)
 
-#define STATIC_CHAIN_REGNUM		R18_REGNUM
+#ifdef TARGET_ARM64_MS_ABI
+# define STATIC_CHAIN_REGNUM		R17_REGNUM
+#else
+# define STATIC_CHAIN_REGNUM		R18_REGNUM
+#endif
 #define HARD_FRAME_POINTER_REGNUM	R29_REGNUM
 #define FRAME_POINTER_REGNUM		SFP_REGNUM
 #define STACK_POINTER_REGNUM		SP_REGNUM