Re: [PATCH 3/6][ARM] Implement support for ACLE Coprocessor CDP intrinsics

[Kyrill Tkachov <kyrylo dot tkachov at foss dot arm dot com>]

Hi Andre,

On 09/11/16 10:11, Andre Vieira (lists) wrote:
> Hi,
>
> This patch implements support for the ARM ACLE Coprocessor CDP
> intrinsics. See below a table mapping the intrinsics to their respective
> instructions:
>
> +----------------------------------------------------+--------------------------------------+
> | Intrinsic signature                                | Instruction
> pattern                  |
> +----------------------------------------------------+--------------------------------------+
> |void __arm_cdp(coproc, opc1, CRd, CRn, CRm, opc2)   |CDP coproc, opc1,
> CRd, CRn, CRm, opc2 |
> +----------------------------------------------------+--------------------------------------+
> |void __arm_cdp2(coproc, opc1, CRd, CRn, CRm, opc2)  |CDP2 coproc, opc1,
> CRd, CRn, CRm, opc2|
> +----------------------------------------------------+--------------------------------------+
> Note that any untyped variable in the intrinsic signature is required to
> be a compiler-time constant and has the type 'unsigned int'.  We do some
> boundary checks for coproc:[0-15], opc1:[0-15], CR*:[0-31], opc2:[0-7].
> If either of these requirements are not met a diagnostic is issued.
>
> I renamed neon_const_bounds in this patch, to arm_const_bounds, simply
> because it is also used in the Coprocessor intrinsics.  It also requires
> the expansion of the builtin frame work such that it accepted 'void'
> modes and intrinsics with 6 arguments.
>
> I also changed acle.exp to run tests for multiple options, where all lto
> option sets are appended with -ffat-objects to allow for assembly scans.
>
> Is this OK for trunk?

This is okay if bootstrap and testing is ok (as part of the whole series)
modulo a couple of nits in the documentation below.

Thanks,
Kyrill

> Regards,
> Andre
>
> gcc/ChangeLog:
> 2016-11-09  Andre Vieira  <andre.simoesdiasvieira@arm.com>
>
>    * config/arm/arm.md (<cdp>): New.
>    * config/arm/arm.c (neon_const_bounds): Rename this ...
>    (arm_const_bounds): ... this.
>    (arm_coproc_builtin_available): New.
>    * config/arm/arm-builtins.c (SIMD_MAX_BUILTIN_ARGS): Increase.
>    (arm_type_qualifiers): Add 'qualifier_unsigned_immediate'.
>    (CDP_QUALIFIERS): Define to...
>    (arm_cdp_qualifiers): ... this. New.
>    (void_UP): Define.
>    (arm_expand_builtin_args): Add case for 6 arguments.
>    * config/arm/arm-protos.h (neon_const_bounds): Rename this ...
>    (arm_const_bounds): ... this.
>    (arm_coproc_builtin_available): New.
>    * config/arm/arm_acle.h (__arm_cdp): New.
>    (__arm_cdp2): New.
>    * config/arm/arm_acle_builtins.def (cdp): New.
>    (cdp2): New.
>    * config/arm/iterators.md (CDPI,CDP,cdp): New.
>    * config/arm/neon.md: Rename all 'neon_const_bounds' to
>    'arm_const_bounds'.
>    * config/arm/types.md (coproc): New.
>    * config/arm/unspecs.md (VUNSPEC_CDP, VUNSPEC_CDP2): New.
>    * gcc/doc/extend.texi (ACLE): Add a mention of Coprocessor intrinsics.
>
> gcc/testsuite/ChangeLog:
> 2016-11-09  Andre Vieira  <andre.simoesdiasvieira@arm.com>
>
>    * gcc.target/arm/acle/acle.exp: Run tests for different options
>    and make sure fat-lto-objects is used such that we can still do
>    assemble scans.
>    * gcc.target/arm/acle/cdp.c: New.
>    * gcc.target/arm/acle/cdp2.c: New.
>    * lib/target-supports.exp (check_effective_target_arm_coproc1_ok): New.
>    (check_effective_target_arm_coproc1_ok_nocache): New.
>    (check_effective_target_arm_coproc2_ok): New.
>    (check_effective_target_arm_coproc2_ok_nocache): New.
>    (check_effective_target_arm_coproc3_ok): New.
>    (check_effective_target_arm_coproc3_ok_nocache): New.

--- a/gcc/doc/sourcebuild.texi
+++ b/gcc/doc/sourcebuild.texi
@@ -1675,6 +1675,21 @@ and @code{MOVT} instructions available.
 ARM target generates Thumb-1 code for @code{-mthumb} with
 @code{CBZ} and @code{CBNZ} instructions available.
 
+@item arm_coproc1_ok
+@anchor{arm_coproc1_ok}
+ARM target supports the following coprocessor instruction: @code{CDP},
+@code{LDC}, @code{STC}, @code{MCR} and @code{MRC}.


s/instruction/instructions/


+@item arm_coproc2_ok
+@anchor{arm_coproc2_ok}
+ARM target supports the all the coprocessor instructions also listed as
+supported in @ref{arm_coproc1_ok} and the following: @code{CDP2}, @code{LDC2},
+@code{LDC2l}, @code{STC2}, @code{STC2l}, @code{MCR2} and @code{MRC2}.
+

s/the all the/all the/.
Also, I'd prefer to say "in addition to the following" rather than "and the following"

 +@item arm_coproc3_ok
+ARM target supports the all the coprocessor instructions also listed as
+supported in @ref{arm_coproc2_ok} and the following: @code{MCRR}, @code{MCRR2},
+@code{MRRC}, and @code{MRRC2}.

Likewise.