"Andre Vieira (lists)" <andre.simoesdiasvie...@arm.com> writes:
> Hey,
>
> Dropped the first patch and dealt with the comments above, hopefully I
> didn't miss any this time.
>
> ----------------------------------
>
> This patch adds support for C23's _BitInt for the AArch64 port when
> compiling
> for little endianness. Big Endianness requires further target-agnostic
> support and we therefor disable it for now.
>
> gcc/ChangeLog:
>
> * config/aarch64/aarch64.cc (TARGET_C_BITINT_TYPE_INFO): Declare MACRO.
> (aarch64_bitint_type_info): New function.
> (aarch64_return_in_memory_1): Return large _BitInt's in memory.
> (aarch64_function_arg_alignment): Adapt to correctly return the ABI
> mandated alignment of _BitInt(N) where N > 128 as the alignment of
> TImode.
> (aarch64_composite_type_p): Return true for _BitInt(N), where N > 128.
>
> libgcc/ChangeLog:
>
> * config/aarch64/t-softfp (softfp_extras): Add floatbitinthf,
> floatbitintbf, floatbitinttf and fixtfbitint.
> * config/aarch64/libgcc-softfp.ver (GCC_14.0.0): Add __floatbitinthf,
> __floatbitintbf, __floatbitinttf and __fixtfbitint.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/aarch64/bitint-alignments.c: New test.
> * gcc.target/aarch64/bitint-args.c: New test.
> * gcc.target/aarch64/bitint-sizes.c: New test.
>
>
> On 02/02/2024 14:46, Jakub Jelinek wrote:
>> On Thu, Jan 25, 2024 at 05:45:01PM +0000, Andre Vieira wrote:
>>> This patch adds support for C23's _BitInt for the AArch64 port when
>>> compiling
>>> for little endianness. Big Endianness requires further target-agnostic
>>> support and we therefor disable it for now.
>>>
>>> gcc/ChangeLog:
>>>
>>> * config/aarch64/aarch64.cc (TARGET_C_BITINT_TYPE_INFO): Declare MACRO.
>>> (aarch64_bitint_type_info): New function.
>>> (aarch64_return_in_memory_1): Return large _BitInt's in memory.
>>> (aarch64_function_arg_alignment): Adapt to correctly return the ABI
>>> mandated alignment of _BitInt(N) where N > 128 as the alignment of
>>> TImode.
>>> (aarch64_composite_type_p): Return true for _BitInt(N), where N > 128.
>>>
>>> libgcc/ChangeLog:
>>>
>>> * config/aarch64/t-softfp: Add fixtfbitint, floatbitinttf and
>>> floatbitinthf to the softfp_extras variable to ensure the
>>> runtime support is available for _BitInt.
>>
>> I think this lacks some config/aarch64/t-whatever.ver
>> additions.
>> See PR113700 for some more details.
>> We want the support routines for binary floating point <-> _BitInt
>> conversions in both libgcc.a and libgcc_s.so.1 and exported from the latter
>> too at GCC_14.0.0 symver, while decimal floating point <-> _BitInt solely in
>> libgcc.a (as with all the huge dfp/bid stuff).
>>
>> Jakub
>>
>
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index
> 16318bf925883ecedf9345e53fc0824a553b2747..9bd8d22f6edd9f6c77907ec383f9e8bf055cfb8b
> 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -6583,6 +6583,7 @@ aarch64_return_in_memory_1 (const_tree type)
> int count;
>
> if (!AGGREGATE_TYPE_P (type)
> + && TREE_CODE (type) != BITINT_TYPE
> && TREE_CODE (type) != COMPLEX_TYPE
> && TREE_CODE (type) != VECTOR_TYPE)
> /* Simple scalar types always returned in registers. */
> @@ -21895,6 +21896,11 @@ aarch64_composite_type_p (const_tree type,
> if (type && (AGGREGATE_TYPE_P (type) || TREE_CODE (type) == COMPLEX_TYPE))
> return true;
>
> + if (type
> + && TREE_CODE (type) == BITINT_TYPE
> + && int_size_in_bytes (type) > 16)
> + return true;
> +
Think I probably said this before, but for the record: I don't think
the above code has any practical effect, but I agree it's probably better
to include it for completeness.
> if (mode == BLKmode
> || GET_MODE_CLASS (mode) == MODE_COMPLEX_FLOAT
> || GET_MODE_CLASS (mode) == MODE_COMPLEX_INT)
> @@ -28400,6 +28406,42 @@ aarch64_excess_precision (enum excess_precision_type
> type)
> return FLT_EVAL_METHOD_UNPREDICTABLE;
> }
>
> +/* Implement TARGET_C_BITINT_TYPE_INFO.
> + Return true if _BitInt(N) is supported and fill its details into *INFO.
> */
> +bool
> +aarch64_bitint_type_info (int n, struct bitint_info *info)
> +{
> + if (TARGET_BIG_END)
> + return false;
> +
> + if (n <= 8)
> + info->limb_mode = QImode;
> + else if (n <= 16)
> + info->limb_mode = HImode;
> + else if (n <= 32)
> + info->limb_mode = SImode;
> + else if (n <= 64)
> + info->limb_mode = DImode;
> + else if (n <= 128)
> + info->limb_mode = TImode;
> + else
> + /* The AAPCS for AArch64 defines _BitInt(N > 128) as an array with
> + type {signed,unsigned} __int128[M] where M*128 >= N. However, to be
> + able to use libgcc's implementation to support large _BitInt's we need
> + to use a LIMB_MODE that is no larger than 'long long'. This is why we
> + use DImode for our internal LIMB_MODE and we define the ABI_LIMB_MODE
> to
> + be TImode to ensure we are ABI compliant. */
> + info->limb_mode = DImode;
> +
> + if (n > 128)
> + info->abi_limb_mode = TImode;
> + else
> + info->abi_limb_mode = info->limb_mode;
> + info->big_endian = TARGET_BIG_END;
> + info->extended = false;
> + return true;
> +}
> +
> /* Implement TARGET_SCHED_CAN_SPECULATE_INSN. Return true if INSN can be
> scheduled for speculative execution. Reject the long-running division
> and square-root instructions. */
> @@ -30524,6 +30566,9 @@ aarch64_run_selftests (void)
> #undef TARGET_C_EXCESS_PRECISION
> #define TARGET_C_EXCESS_PRECISION aarch64_excess_precision
>
> +#undef TARGET_C_BITINT_TYPE_INFO
> +#define TARGET_C_BITINT_TYPE_INFO aarch64_bitint_type_info
> +
> #undef TARGET_EXPAND_BUILTIN
> #define TARGET_EXPAND_BUILTIN aarch64_expand_builtin
>
OK for code bits. I've got some comments about the tests though:
> diff --git a/gcc/testsuite/gcc.target/aarch64/bitint-alignments.c
> b/gcc/testsuite/gcc.target/aarch64/bitint-alignments.c
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..4de31fe7ebd933247911c48ace01ab520fe194a3
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/bitint-alignments.c
> @@ -0,0 +1,58 @@
> +/* { dg-do run } */
> +/* { dg-options "-std=c23" } */
> +
> +static long unsigned int
> +calc_size (int n)
> +{
> + if (n > 64)
> + return alignof(__int128_t);
> + if (n > 32)
> + return alignof(long long);
> + if (n > 16)
> + return alignof(int);
> + if (n > 8)
> + return alignof(short);
> + else
> + return alignof(char);
> +}
> +
> +#define CHECK_ALIGNMENT(N) \
> + if (alignof(_BitInt(N)) != calc_size(N)) \
> + __builtin_abort ();
> +
> +int main (void)
> +{
> + CHECK_ALIGNMENT(2);
> + CHECK_ALIGNMENT(3);
> + CHECK_ALIGNMENT(7);
> + CHECK_ALIGNMENT(8);
> + CHECK_ALIGNMENT(9);
> + CHECK_ALIGNMENT(13);
> + CHECK_ALIGNMENT(15);
> + CHECK_ALIGNMENT(16);
> + CHECK_ALIGNMENT(17);
> + CHECK_ALIGNMENT(24);
> + CHECK_ALIGNMENT(31);
> + CHECK_ALIGNMENT(32);
> + CHECK_ALIGNMENT(33);
> + CHECK_ALIGNMENT(42);
> + CHECK_ALIGNMENT(53);
> + CHECK_ALIGNMENT(63);
> + CHECK_ALIGNMENT(64);
> + CHECK_ALIGNMENT(65);
> + CHECK_ALIGNMENT(79);
> + CHECK_ALIGNMENT(96);
> + CHECK_ALIGNMENT(113);
> + CHECK_ALIGNMENT(127);
> + CHECK_ALIGNMENT(128);
> + CHECK_ALIGNMENT(129);
> + CHECK_ALIGNMENT(153);
> + CHECK_ALIGNMENT(255);
> + CHECK_ALIGNMENT(256);
> + CHECK_ALIGNMENT(257);
> + CHECK_ALIGNMENT(353);
> + CHECK_ALIGNMENT(512);
> + CHECK_ALIGNMENT(620);
> + CHECK_ALIGNMENT(1024);
> + CHECK_ALIGNMENT(30000);
> +}
> diff --git a/gcc/testsuite/gcc.target/aarch64/bitint-args.c
> b/gcc/testsuite/gcc.target/aarch64/bitint-args.c
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..a6806ce609b3262c942e722918081ad466853910
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/bitint-args.c
> @@ -0,0 +1,84 @@
> +/* { dg-do compile } */
> +/* { dg-options "-std=c23 -O -fno-stack-clash-protection -g" } */
> +/* { dg-final { check-function-bodies "**" "" } } */
> +
> +#define CHECK_ARG(N) \
> +_BitInt(N) g##N; \
> +void f##N(int x, _BitInt(N) y) \
> +{ \
> + g##N = y; \
> +}
> +
> +
> +CHECK_ARG(2)
> +/*
> +** f2:
> +** sbfiz w1, w1, 6, 2
> +** asr w1, w1, 6
> +** adrp x0, .*
> +** strb w1, \[x0, [^\]]*\]
> +** ret
There's no requirement for w1 or x0 to be used as the temporaries,
so everything except the incoming w1 should be escaped and captured. E.g.:
> +** sbfiz (w[0-9]+), w1, 6, 2
> +** asr (w[0-9]+), \1, 6
> +** adrp (x[0-9]+), .*
> +** strb \2, \[\3, [^\]]*\]
> +** ret
FWIW, passing a pointer a _BitInt(N) instead of x would avoid the need
for the adrp, and so make the tests more robust against code order. E.g.:
void f##N(_BitInt(N) *ptr, _BitInt(N) y) \
{ \
*ptr = y; \
}
could be matched by:
> +** sbfiz (w[0-9]+), w1, 6, 2
> +** asr (w[0-9]+), \1, 6
> +** strb \2, \[x0\]
> +** ret
Do you know why we don't use a single SBFX? Probably worth filing a PR
if we don't have one already.
> +*/
> +CHECK_ARG(8)
> +/*
> +** f8:
> +** adrp x0, .*
> +** strb w1, \[x0, [^\]]*\]
> +** ret
> +*/
> +CHECK_ARG(9)
> +/*
> +** f9:
> +** sbfiz w1, w1, 7, 9
> +** asr w1, w1, 7
> +** adrp x0, .*
> +** strh w1, \[x0, [^\]]*\]
> +** ret
> +*/
> +CHECK_ARG(16)
> +/*
> +** f16:
> +** adrp x0, .*
> +** strh w1, \[x0, [^\]]*\]
> +** ret
> +*/
> +CHECK_ARG(19)
> +/*
> +** f19:
> +** sbfx x1, x1, 0, 19
> +** adrp x0, .*
> +** str w1, \[x0, [^\]]*\]
> +** ret
> +*/
> +CHECK_ARG(32)
> +/*
> +** f32:
> +** adrp x0, .*
> +** str w1, \[x0, [^\]]*\]
> +** ret
> +*/
> +CHECK_ARG(42)
> +/*
> +** f42:
> +** sbfx x1, x1, 0, 42
> +** adrp x0, .*
> +** str x1, \[x0, [^\]]*\]
> +** ret
> +*/
> +CHECK_ARG(64)
> +/*
> +** f64:
> +** adrp x0, .*
> +** str x1, \[x0, [^\]]*\]
> +** ret
> +*/
> +CHECK_ARG(65)
> +/*
> +** f65:
> +** extr x3, x3, x2, 1
> +** asr x3, x3, 63
> +** adrp x0, .*
> +** add x0, x0, .*
> +** stp x2, x3, \[x0, [^\]]*\]
> +** ret
> +*/
Can you add tests for 127, 128 and 129 too?
I think we should also have ABI tests for more exotic combinations, such as:
struct S1 {
_BitInt(120) x1 : 120;
_BitInt(8) x2 : 8;
};
struct S2 {
_BitInt(120) x1 : 120;
_BitInt(8) x2 : 8;
};
struct S3 {
_BitInt(125) x1 : 63;
unsigned _BitInt(125) x2 : 62;
};
struct S4 {
_BitInt(5) x1 : 5;
__attribute__((packed, aligned(2))) _BitInt(300) x2;
};
etc.
It'd also be good to have a version of
gcc.target/aarch64/bitfield-abi-warning.* that tests _BitInts rather
than plain integers --- not for the warning as such (which I guess we
shouldn't emit), but for the code generation. It took Christophe and I
a lot of effort to untangle the brokenness captured in those tests, so
it would be good to preempt something similar happening here :)
It's also be good to make sure that the alignment on things like:
typedef _BitInt(100) bi1 __attribute__((aligned(1)));
typedef _BitInt(8) bi2 __attribute__((aligned(16)));
do not change how bi1 and bi2 are passed (which might be partly
covered by the bitfield warning tests, can't remember).
Thanks,
Richard
> diff --git a/gcc/testsuite/gcc.target/aarch64/bitint-sizes.c
> b/gcc/testsuite/gcc.target/aarch64/bitint-sizes.c
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..bee9abfe91b0dcb1ec335ef9ed02f212f7aa34b7
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/bitint-sizes.c
> @@ -0,0 +1,60 @@
> +/* { dg-do run } */
> +/* { dg-options "-std=c23" } */
> +
> +static long unsigned int
> +calc_size (int n)
> +{
> + if (n > 128)
> + return ((n - 1)/128 + 1) * sizeof(__int128_t);
> + if (n > 64)
> + return sizeof(__int128_t);
> + if (n > 32)
> + return sizeof(long long);
> + if (n > 16)
> + return sizeof(int);
> + if (n > 8)
> + return sizeof(short);
> + else
> + return sizeof(char);
> +}
> +
> +#define CHECK_SIZE(N) \
> + if (sizeof(_BitInt(N)) != calc_size(N)) \
> + __builtin_abort ();
> +
> +int main (void)
> +{
> + CHECK_SIZE(2);
> + CHECK_SIZE(3);
> + CHECK_SIZE(7);
> + CHECK_SIZE(8);
> + CHECK_SIZE(9);
> + CHECK_SIZE(13);
> + CHECK_SIZE(15);
> + CHECK_SIZE(16);
> + CHECK_SIZE(17);
> + CHECK_SIZE(24);
> + CHECK_SIZE(31);
> + CHECK_SIZE(32);
> + CHECK_SIZE(33);
> + CHECK_SIZE(42);
> + CHECK_SIZE(53);
> + CHECK_SIZE(63);
> + CHECK_SIZE(64);
> + CHECK_SIZE(65);
> + CHECK_SIZE(79);
> + CHECK_SIZE(96);
> + CHECK_SIZE(113);
> + CHECK_SIZE(127);
> + CHECK_SIZE(128);
> + CHECK_SIZE(129);
> + CHECK_SIZE(153);
> + CHECK_SIZE(255);
> + CHECK_SIZE(256);
> + CHECK_SIZE(257);
> + CHECK_SIZE(353);
> + CHECK_SIZE(512);
> + CHECK_SIZE(620);
> + CHECK_SIZE(1024);
> + CHECK_SIZE(30000);
> +}
> diff --git a/libgcc/config/aarch64/libgcc-softfp.ver
> b/libgcc/config/aarch64/libgcc-softfp.ver
> index
> e73f5f9129776d39eb5020ed7398dc59aba2d197..9ba857036abef99913eebe56971eaaabf5e1952e
> 100644
> --- a/libgcc/config/aarch64/libgcc-softfp.ver
> +++ b/libgcc/config/aarch64/libgcc-softfp.ver
> @@ -39,3 +39,11 @@ GCC_13.0.0 {
> __trunctfbf2
> __trunchfbf2
> }
> +
> +%inherit GCC_14.0.0 GCC_13.0.0
> +GCC_14.0.0 {
> + __fixtfbitint
> + __floatbitintbf
> + __floatbitinthf
> + __floatbitinttf
> +}
> diff --git a/libgcc/config/aarch64/t-softfp b/libgcc/config/aarch64/t-softfp
> index
> 2e32366f891361e2056c680b2e36edb1871c7670..80e7e77a545cc10eeccd84eea092871751c3e139
> 100644
> --- a/libgcc/config/aarch64/t-softfp
> +++ b/libgcc/config/aarch64/t-softfp
> @@ -4,7 +4,8 @@ softfp_extensions := sftf dftf hftf bfsf
> softfp_truncations := tfsf tfdf tfhf tfbf dfbf sfbf hfbf
> softfp_exclude_libgcc2 := n
> softfp_extras += fixhfti fixunshfti floattihf floatuntihf \
> - floatdibf floatundibf floattibf floatuntibf
> + floatdibf floatundibf floattibf floatuntibf \
> + floatbitinthf floatbitintbf floatbitinttf fixtfbitint
>
> TARGET_LIBGCC2_CFLAGS += -Wno-missing-prototypes
>
