<[email protected]> writes:
> This patch introduces support for LUTI2/LUTI4 ACLE for SVE2.
>
> LUTI instructions are used for efficient table lookups with 2-bit
> or 4-bit indices. LUTI2 reads indexed 8-bit or 16-bit elements from
> the low 128 bits of the table vector using packed 2-bit indices,
> while LUTI4 can read from the low 128 or 256 bits of the table
> vector or from two table vectors using packed 4-bit indices.
> These instructions fill the destination vector by copying elements
> indexed by segments of the source vector, selected by the vector
> segment index.
>
> The changes include the addition of a new AArch64 option
> extension "lut", __ARM_FEATURE_LUT preprocessor macro, definitions
> for the new LUTI instruction shapes, and implementations of the
> svluti2 and svluti4 builtins.
>
> New tests are added as well.
>
> ---
>
> Hey,
>
> This is a respin of
> https://gcc.gnu.org/pipermail/gcc-patches/2024-July/658015.html. Rebased
> with master. Regression tested on aarch64-unknown-linux-gnu and found no
> regressions.
Thanks for picking this up! The issues described below are of course not
your fault :)
> Ok for master?
>
> Thanks,
> Saurabh
> ---
> [...]
> diff --git a/gcc/config/aarch64/aarch64-sve-builtins-shapes.cc
> b/gcc/config/aarch64/aarch64-sve-builtins-shapes.cc
> index ca721dd2c09..0f6d366b2d6 100644
> --- a/gcc/config/aarch64/aarch64-sve-builtins-shapes.cc
> +++ b/gcc/config/aarch64/aarch64-sve-builtins-shapes.cc
> @@ -903,6 +903,47 @@ struct load_ext_gather_base : public overloaded_base<1>
> }
> };
>
> +
> +/* sv<v0>_t svlut_<t0>(sv<t0>_t, svuint8_t, uint64_t)
Because of the potential tuple argument, I suppose this should be:
sv<v0>_t svlut[_<t0>_g](sv<t0>x<g>_t, svuint8_t, uint64_t)
Unlike for the ZT version of svluti, the type suffix is optional.
To quote from the ACLE spec:
svint8_t svluti2_lane[_s8](svint8_t table, svuint8_t indices, uint64_t
imm_idx);
svint16_t svluti2_lane[_s16]( svint16_t table, svuint8_t indices, uint64_t
imm_idx);
svint8_t svluti4_lane[_s8](svint8_t table, svuint8_t indices, uint64_t
imm_idx);
svint16_t svluti4_lane[_s16](svint16_t table, svuint8_t indices, uint64_t
imm_idx);
svint16_t svluti4_lane[_s16_x2](svint16x2_t table, svuint8_t indices,
uint64_t imm_idx);
Because of that:
> + where the final argument is a constant index, the instruction divides
> + the vector argument in BITS-bit quantities. */
> +template<unsigned int BITS>
> +struct luti_base : public nonoverloaded_base
...this should be an overloaded function and have a resolver.
> +{
> + void
> + build (function_builder &b, const function_group_info &group) const
> override
> + {
> + /* Format: return type, table vector, indices vector, immediate value.
> */
> + build_all (b, "v0,t0,vu8,su64", group, MODE_none);
> + }
> +
> + bool
> + check (function_checker &c) const override
> + {
> + int max_range;
> + bool byte_mode = c.type_suffix (0).element_bits == 8;
> +
> + if (BITS == 2)
> + max_range = byte_mode ? 3 : 7;
> + else if (BITS == 4)
> + max_range = byte_mode ? 1 : 7;
It looks like this should be ? 1 : 3, see:
https://developer.arm.com/documentation/ddi0602/2024-12/SVE-Instructions/LUTI4--Lookup-table-read-with-4-bit-indices-?lang=en
Or, programmatically, I think this is:
auto max_range = c.type.suffix (0).element_bits / BITS - 1;
for all cases.
> + else
> + /* Unsupported number of indices bits for LUTI. */
> + gcc_unreachable ();
> +
> + return c.require_immediate_range (2, 0, max_range);
> + }
> +
> +};
> +
> +/* Specializations for 2-bit and 4-bit indices. */
> +using luti2_def = luti_base<2>;
> +SHAPE (luti2)
> +
> +using luti4_def = luti_base<4>;
> +SHAPE (luti4)
> +
> +
> /* sv<t0>x<g>_t svfoo_t0_g(uint64_t, svuint8_t, uint64_t)
>
> where the first argument is the ZT register number (currently always 0)
> [...]
> diff --git a/gcc/config/aarch64/aarch64-sve-builtins-sve2.def
> b/gcc/config/aarch64/aarch64-sve-builtins-sve2.def
> index e726fa1fb68..0c4f8251ac0 100644
> --- a/gcc/config/aarch64/aarch64-sve-builtins-sve2.def
> +++ b/gcc/config/aarch64/aarch64-sve-builtins-sve2.def
> @@ -164,6 +164,10 @@ DEF_SVE_FUNCTION (svwhilegt, compare_scalar, while, none)
> DEF_SVE_FUNCTION (svwhilerw, compare_ptr, all_data, none)
> DEF_SVE_FUNCTION (svwhilewr, compare_ptr, all_data, none)
> DEF_SVE_FUNCTION (svxar, ternary_shift_right_imm, all_integer, none)
> +DEF_SVE_FUNCTION (svluti2_lane, luti2, bhs_data, none)
> +DEF_SVE_FUNCTION (svluti4_lane, luti4, bhs_data, none)
> +DEF_SVE_FUNCTION_GS (svluti4_lane, luti4, bhs_data, x2, none)
bhs_data looks wrong: there should be no .s versions. Similarly...
> +
> #undef REQUIRED_EXTENSIONS
>
> #define REQUIRED_EXTENSIONS nonstreaming_sve (AARCH64_FL_SVE2)
> [...]
> diff --git a/gcc/config/aarch64/aarch64-sve2.md
> b/gcc/config/aarch64/aarch64-sve2.md
> index f8cfe08f4c0..7dcbc0700da 100644
> --- a/gcc/config/aarch64/aarch64-sve2.md
> +++ b/gcc/config/aarch64/aarch64-sve2.md
> @@ -133,6 +133,7 @@
> ;; ---- Optional AES extensions
> ;; ---- Optional SHA-3 extensions
> ;; ---- Optional SM4 extensions
> +;; ---- Table lookup
>
> ;; =========================================================================
> ;; == Moves
> @@ -4211,3 +4212,47 @@
> "sm4ekey\t%0.s, %1.s, %2.s"
> [(set_attr "type" "crypto_sm4")]
> )
> +
> +;; -------------------------------------------------------------------------
> +;; ---- Table lookup
> +;; -------------------------------------------------------------------------
> +;; Includes:
> +;; - LUTI2
> +;; - LUTI4
> +;; -------------------------------------------------------------------------
> +
> +(define_insn "@aarch64_sve_luti<LUTI_BITS><mode>"
> + [(set (match_operand:SVE_FULL_BS 0 "register_operand" "=w")
> + (unspec:SVE_FULL_BS
> + [(match_operand:SVE_FULL_BS 1 "register_operand" "w")
> + (match_operand:VNx16QI 2 "register_operand" "w")
> + (match_operand:DI 3 "const_int_operand")
> + (const_int LUTI_BITS)]
> + UNSPEC_SVE_LUTI))]
> + "TARGET_SVE2"
> + "luti<LUTI_BITS>\t%0.<Vetype>, { %1.<Vetype> }, %2[%3]"
> +)
> +
> +(define_insn "@aarch64_sve_luti<LUTI_BITS><mode>"
> + [(set (match_operand:<VSINGLE> 0 "register_operand" "=w")
> + (unspec:<VSINGLE>
> + [(match_operand:SVE_FULL_H 1 "aligned_register_operand" "w")
> + (match_operand:VNx16QI 2 "register_operand" "w")
> + (match_operand:DI 3 "const_int_operand")
> + (const_int LUTI_BITS)]
> + UNSPEC_SVE_LUTI))]
> + "TARGET_SVE2"
> + "luti<LUTI_BITS>\t%0.<Vetype>, { %1.<Vetype> }, %2[%3]"
> +)
...there should be .S (VNx4) variants here. Also, the .H variants
don't require an aligned register operand. (FWIW, using "w" with
"aligned_register_operand" is wrong in any case, since "w" accepts
unaligned registers.)
It looks like we could merge the patterns above into a single SVE_FULL_BH
pattern.
> +
> +(define_insn "@aarch64_sve_luti<LUTI_BITS><mode>"
> + [(set (match_operand:<VSINGLE> 0 "register_operand" "=w")
> + (unspec:<VSINGLE>
> + [(match_operand:SVE_FULL_Hx2 1 "aligned_register_operand" "Uw2")
This operand also isn't required to be aligned: Zn has a 5-bit encoding.
> + (match_operand:VNx16QI 2 "register_operand" "w")
> + (match_operand:DI 3 "const_int_operand")
> + (const_int LUTI_BITS)]
> + UNSPEC_SVE_LUTI))]
> + "TARGET_SVE2"
> + "luti<LUTI_BITS>\t%0.<Vetype>, %1, %2[%3]"
> +)
Also, formatting nit, but: it's more usual to indent the "[" in an
unspec by 2 or 1 extra columns, rather than a full tab.
> [...]
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve2/acle/asm/luti2_bf16.c
> b/gcc/testsuite/gcc.target/aarch64/sve2/acle/asm/luti2_bf16.c
> new file mode 100644
> index 00000000000..f423bfae2c6
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/sve2/acle/asm/luti2_bf16.c
> @@ -0,0 +1,40 @@
> +/* { dg-do compile } */
This would be better as:
/* { dg-do assemble { target aarch64_asm_lut_ok } } */
/* { dg-do compile { target { ! aarch64_asm_lut_ok } } } */
with lut added to:
foreach { aarch64_ext } { "fp" "simd" "crypto" "crc" "lse" "dotprod" "sve"
"i8mm" "f32mm" "f64mm" "bf16" "sb" "sve2" "ls64"
"sme" "sme-i16i64" "sme2" "sve-b16b16"
"sme-b16b16" "sme-f16f16" "sme2p1" "fp8" "fp8fma"
"ssve-fp8fma" "fp8dot2" "ssve-fp8dot2" "fp8dot4"
"ssve-fp8dot4"} {
eval [string map [list FUNC $aarch64_ext] {
proc check_effective_target_aarch64_asm_FUNC_ok { } {
if { [istarget aarch64*-*-*] } {
return [check_no_compiler_messages aarch64_FUNC_assembler
object {
__asm__ (".arch_extension FUNC");
} "-march=armv8-a+FUNC"]
} else {
return 0
}
}
}]
}
in target-supports.exp.
> +/* { dg-final { check-function-bodies "**" "" "-DCHECK_ASM" } } */
> +
> +#include "test_sve_acle.h"
> +
> +#pragma GCC target "+sve2+lut"
> +#if STREAMING_COMPATIBLE
> +#pragma GCC target "+sme2"
> +#endif
> +
> +/*
> +** luti2_test_imm0:
> +** luti2 z1\.h, \{ z28\.h \}, z0\[0\]
> +** ret
> +*/
> +
> +TEST_XN_SINGLE (luti2_test_imm0, svbfloat16_t, svuint8_t, z1,
> + svluti2_lane_bf16 (z28, z0, 0),
> + svluti2_lane_bf16 (z28, z0, 0))
Following on from the comment above about these intrinsics being
overloaded: the second call above should not have a type suffix.
Similarly for the other tests (sorry!).
> +
> +/*
> +** luti2_test_imm1:
> +** luti2 z1\.h, \{ z28\.h \}, z0\[1\]
> +** ret
> +*/
> +
> +TEST_XN_SINGLE (luti2_test_imm1, svbfloat16_t, svuint8_t, z1,
> + svluti2_lane_bf16 (z28, z0, 1),
> + svluti2_lane_bf16 (z28, z0, 1))
> +
> +/*
> +** luti2_test_tied:
> +** luti2 z28\.h, \{ z28\.h \}, z0\[2\]
> +** ret
> +*/
> +
> +TEST_XN_SINGLE (luti2_test_tied, svbfloat16_t, svuint8_t, z28,
> + svluti2_lane_bf16 (z28, z0, 2),
> + svluti2_lane_bf16 (z28, z0, 2))
I think we should test the upper bound of each range. A combination of
testing the upper bound + the dg-do change would have caught the ? 1 : 7
thing above.
> [...]
> diff --git a/gcc/testsuite/lib/target-supports.exp
> b/gcc/testsuite/lib/target-supports.exp
> index 45ba2f47a9d..e0d9867801c 100644
> --- a/gcc/testsuite/lib/target-supports.exp
> +++ b/gcc/testsuite/lib/target-supports.exp
> @@ -4800,6 +4800,18 @@ proc check_effective_target_aarch64_sve2 { } {
> }]
> }
>
> +# Return 1 if this is an AArch64 target supporting LUT (Lookup table)
> +proc check_effective_target_aarch64_lut { } {
> + if { ![istarget aarch64*-*-*] || ![check_effective_target_aarch64_sve2]
> } {
> + return 0
> + }
> + return [check_no_compiler_messages aarch64_lut assembly {
> + #if !defined (__ARM_FEATURE_LUT)
> + #error FOO
> + #endif
> + }]
> +}
> +
> # Return 1 if this is an AArch64 target only supporting SVE (not SVE2).
> proc check_effective_target_aarch64_sve1_only { } {
> return [expr { [check_effective_target_aarch64_sve]
I'm not sure this is needed.
Thanks,
Richard
