On 12/19/19 10:08 AM, Richard Sandiford wrote: > Stam Markianos-Wright <stam.markianos-wri...@arm.com> writes: >> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c >> index f57469b6e23..f40f6432fd4 100644 >> --- a/gcc/config/aarch64/aarch64.c >> +++ b/gcc/config/aarch64/aarch64.c >> @@ -21661,6 +21661,68 @@ aarch64_stack_protect_guard (void) >> return NULL_TREE; >> } >> >> +/* Return the diagnostic message string if conversion from FROMTYPE to >> + TOTYPE is not allowed, NULL otherwise. */ >> + >> +static const char * >> +aarch64_invalid_conversion (const_tree fromtype, const_tree totype) >> +{ >> + static char templ[100]; >> + if ((GET_MODE_INNER (TYPE_MODE (fromtype)) == BFmode >> + || GET_MODE_INNER (TYPE_MODE (totype)) == BFmode) >> + && TYPE_MODE (fromtype) != TYPE_MODE (totype)) >> + { >> + snprintf (templ, sizeof (templ), \ >> + "incompatible types when assigning to type '%s' from type '%s'", >> + IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (totype))), >> + IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (fromtype)))); >> + return N_(templ); >> + } >> + /* Conversion allowed. */ >> + return NULL; >> +} >> + > > This won't handle translation properly. We also have no guarantee that > the formatted string will fit in 100 characters since at least one of > the type names is unconstrained. (Also, not all types have names.) >
Hi Richard. I'm sending an email here to show you what I have done here, too :) Currently I have the following: static const char * aarch64_invalid_conversion (const_tree fromtype, const_tree totype) { static char templ[100]; if (TYPE_MODE (fromtype) != TYPE_MODE (totype) && ((TYPE_MODE (fromtype) == BFmode && !VECTOR_TYPE_P (fromtype)) || (TYPE_MODE (totype) == BFmode && !VECTOR_TYPE_P (totype)))) { if (TYPE_NAME (fromtype) != NULL && TYPE_NAME (totype) != NULL) { snprintf (templ, sizeof (templ), "incompatible types when assigning to type '%s' from type '%s'", IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (totype))), IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (fromtype)))); return N_(templ); } else { snprintf (templ, sizeof (templ), "incompatible types for assignment"); return N_(templ); } } /* Conversion allowed. */ return NULL; } This blocks the conversion only if the two types are of different modes and one of them is a BFmode scalar. Doing it like this seems to block all scalar-sized assignments: C: typedef bfloat16_t vbf __attribute__((vector_size(2))); vbf foo3 (void) { return (vbf) 0x1234; } bfloat16_t foo1 (void) { return (bfloat16_t) 0x1234; } bfloat16_t scalar1_3 = 0; bfloat16_t scalar1_4 = 0.1; bfloat16_t scalar1_5 = is_a_float; bfloat16x4_t vector2_8 = { 0.0, 0, n2, is_a_float }; // (blocked on each element assignment) C++: bfloat16_t c1 (void) { return bfloat16_t (0x1234); } bfloat16_t c2 (void) { return bfloat16_t (0.1); } But then it allows vector initialisation from binary: C: bfloat16x4_t foo1 (void) { return (bfloat16x4_t) 0x1234567812345678; } C++: bfloat16x4_t foo1 (void) { return bfloat16x4_t (0x1234567812345678); } typedef bfloat16_t v2bf __attribute__((vector_size(4))); v2bf foo3 (void) { return v2bf (0x12345678); } I also need to check with a colleague who is on holiday if any of this impacts the vector-reinterpret intrinsics that he was working on... Let me know of your thoughts! Cheers, Stam > Unfortunately the interface of the current hook doesn't allow for good > diagnostics. We'll just have to return a fixed string. > > Formatting nit: braced block should be indented two spaces more > than the "if (...)". > > Same comment for the other hooks. Done. Will be in next revision > >> +/* Return the diagnostic message string if the unary operation OP is >> + not permitted on TYPE, NULL otherwise. */ >> + >> +static const char * >> +aarch64_invalid_unary_op (int op, const_tree type) >> +{ >> + static char templ[100]; >> + /* Reject all single-operand operations on BFmode except for &. */ >> + if (GET_MODE_INNER (TYPE_MODE (type)) == BFmode && op != ADDR_EXPR) >> + { >> + snprintf (templ, sizeof (templ), >> + "operation not permitted on type '%s'", >> + IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (type)))); >> + return N_(templ); >> + } >> + /* Operation allowed. */ >> + return NULL; >> +} > > The problem with testing TYPE_MODE is that we'll then miss things > that don't have a dedicated mode. E.g. it'd be interesting to > test what happens for arithmetic on: > > typedef bfloat16_t v16bf __attribute__((vector_size(32))); > > Probably better to use element_mode instead. Done. Will be in next revision > >> diff --git a/gcc/testsuite/gcc.target/aarch64/bfloat16_scalar_typecheck.c >> b/gcc/testsuite/gcc.target/aarch64/bfloat16_scalar_typecheck.c >> new file mode 100644 >> index 00000000000..6f6a6af9587 >> --- /dev/null >> +++ b/gcc/testsuite/gcc.target/aarch64/bfloat16_scalar_typecheck.c >> @@ -0,0 +1,83 @@ >> +/* { dg-do compile { target { aarch64*-*-* } } } */ >> +/* { dg-skip-if "" { *-*-* } { "-fno-fat-lto-objects" } } */ >> +/* { dg-options "-march=armv8.2-a+i8mm" } */ > > +bf16 rather than +i8mm. But using: > > /* { dg-require-effective-target arm_v8_2a_bf16_neon_ok } */ > /* { dg-add-options arm_v8_2a_bf16_neon } */ > > would be better. Done. Will be in next revision > >> + >> +#include <arm_neon.h> >> + >> +bfloat16_t glob; >> +float is_a_float; >> +int n; >> + >> +bfloat16_t footest (bfloat16_t scalar0) >> +{ >> + >> + /* Initialisation */ >> + >> + bfloat16_t scalar1 = 0.1; /* { dg-error "incompatible types when >> assigning to type 'bfloat16_t' from type 'double'" "" {target *-*-*} } */ >> + bfloat16_t scalar2 = 0; /* { dg-error "incompatible types when >> assigning to type 'bfloat16_t' from type 'int'" "" {target *-*-*} } */ >> + bfloat16_t scalar3 = {}; /* { dg-error "empty scalar initializer" "" >> {target *-*-*} } */ > > Would also be worth testing { scalar0 }, { is_a_float } and { 0.1 }. Done. Will be in next revision > > (For SVE the tests are divided between sizeless_1.c and gnu_vectors_1.c. > Most of the cases mentioned here are handled in gnu_vectors_1.c instead.) > >> + >> + float16_t initi_a = scalar1; /* { dg-error "incompatible types when >> assigning to type 'float16_t' from type 'bfloat16_t'" "" {target *-*-*} } */ >> + float16_t initi_b = { scalar1 }; /* { dg-error "incompatible types when >> assigning to type 'float16_t' from type 'bfloat16_t'" "" {target *-*-*} } */ >> + >> + /* Compound literals. */ >> + >> + (bfloat16_t) {}; /* { dg-error "empty scalar initializer" "" {target >> *-*-*} } */ >> + (bfloat16_t) { scalar1 }; > > Same here. Done. Will be in next revision > >> + >> + (int) { scalar1 }; /* { dg-error "incompatible types when assigning to >> type 'int' from type 'bfloat16_t'" "" {target *-*-*} } */ >> + >> + /* Casting. */ >> + >> + (void) scalar1; >> + (bfloat16_t) scalar1; > > Would be good to have some tests for invalid cases too. Done. Will be in next revision > >> + >> + /* Arrays and Structs. */ >> + >> + typedef bfloat16_t array_type[2]; >> + extern bfloat16_t extern_array[]; >> + >> + bfloat16_t array[2]; >> + bfloat16_t zero_length_array[0]; >> + bfloat16_t empty_init_array[] = {}; >> + typedef bfloat16_t vla_type[n]; >> + >> + struct struct1 { >> + bfloat16_t a; >> + }; >> + >> + union union1 { >> + bfloat16_t a; >> + }; >> + >> + /* Assignments. */ >> + >> + n = scalar1; /* { dg-error "incompatible types when assigning to type >> 'int' from type 'bfloat16_t'" "" {target *-*-*} } */ >> + is_a_float = scalar1; /* { dg-error "incompatible types when assigning to >> type 'float' from type 'bfloat16_t'" "" {target *-*-*} } */ >> + scalar1 = 0; /* { dg-error "incompatible types when assigning to type >> 'bfloat16_t' from type 'int'" "" {target *-*-*} } */ >> + scalar1 = 0.1; /* { dg-error "incompatible types when assigning to type >> 'bfloat16_t' from type 'double'" "" {target *-*-*} } */ >> + scalar1 = scalar2; > > Would be good to test the other way too: "scalar1 = is_a_float", > "scalar1 = n". Done. Will be in next revision > >> + >> + /* Addressing and dereferencing. */ >> + >> + bfloat16_t *bfloat_ptr = &scalar1; >> + scalar1 = *bfloat_ptr; >> + >> + /* Pointer assignment. */ >> + >> + bfloat16_t *bfloat_ptr2 = bfloat_ptr; >> + >> + /* Single-operand operation. */ >> + >> + scalar1 = !glob; /* { dg-error "operation not permitted on type >> 'bfloat16_t'" "" {target *-*-*} } */ > > Would be good to test "+" and "-" as well -- "!" isn't really typical > for floats. > Done. Will be in next revision > Other things worth testing for are: > > - comparisons > - bfloats used as a condition (e.g. bfloat16 ? a : b) > - bfloats selected via ?:, including cases where the types don't match > Done. Will be in next revision >> [...] >> diff --git a/gcc/testsuite/gcc.target/aarch64/bfloat16_vector_typecheck1.c >> b/gcc/testsuite/gcc.target/aarch64/bfloat16_vector_typecheck1.c > > Very minor, but local aarch64 style seems to be to use foo_1, foo_2, > etc. rather than foo, foo1, etc., although things aren't very consistent. Done. Will be in next revision > > Similar comments for these tests as for the scalar ones. > > It would be good to have C++ tests too. An extra thing to test there > is elementwise vector ? vector : vector. Done. Will be in next revision > > Thanks, > Richard >