The following testcase reduced from Linux kernel's crypto/aegis128-neon-inner.c started to be rejected with r11-1741-g31427b974ed7b7dd54e28fec595e731bf6eea8ba and r10-8501-g932e9140d3268cf2033c1c3e93219541c53fcd29 #include <arm_neon.h> struct aegis128_state { uint8x16_t v[5]; }; void foo(const void *key, const void *iv, const void *const0, const void *const1) { uint8x16_t k = vld1q_u8(key); uint8x16_t kiv = k ^ vld1q_u8(iv); struct aegis128_state st = {{ kiv, vld1q_u8(const1), vld1q_u8(const0), k ^ vld1q_u8(const0), k ^ vld1q_u8(const1), }}; } The error is: error: incompatible types when initializing type ‘unsigned char’ using type ‘uint8x16_t’ {aka ‘__Uint8x16_t’} (twice)
I think the problem is that c_common_type does: 742 if (TYPE_ATTRIBUTES (t1) != NULL_TREE) 743 t1 = build_type_attribute_variant (t1, NULL_TREE); 744 745 if (TYPE_ATTRIBUTES (t2) != NULL_TREE) 746 t2 = build_type_attribute_variant (t2, NULL_TREE); which strips off the "Advanced SIMD type" attribute from both of the types. C++ FE doesn't do anything like that.
This dates back to https://gcc.gnu.org/legacy-ml/gcc-patches/2004-06/msg00288.html Dunno, do we want to never strip attributes from VECTOR_TYPEs and only strip them that way from non-VECTOR_TYPEs? Or only strip some attributes and keep others? Or just add a if (t1 == t2) return t1; before the attribute stripping (perhaps for VECTOR_TYPEs only)? Note, due to the typedef __Uint8x16_t uint8x16_t; on this testcase t1 and t2 are equal and are the typedef file, and because they are unqualified, we don't use TYPE_MAIN_VARIANT (which is the __Uint8x16_t type). What is the desirable behavior for the common type if one is the aarch64 specific vector and the other generic vector with the same mode? The C++ behavior for vector types in cp_common_type is that it will merge attributes from both types, and if one is TYPE_UNSIGNED, pick that one, otherwise pick the other one. So in that case the ARM specific vector type wins. I must say I'm surprised that the 10.2 version of the patch also matters, perhaps the FE isn't using the target hook to compare attributes, but rather just compares TYPE_MAIN_VARIANT or something similar to determine if the initializer is initializing a whole vector or an element of the vector.
For those that need a quick workaround for the kernel, I think (uint8x16_t) (k ^ vld1q_u8(const0)), (uint8x16_t) (k ^ vld1q_u8(const1)), instead of k ^ vld1q_u8(const0), k ^ vld1q_u8(const1), will do the job. And it is a question if portable code can use binary operators on the ARM specific vector types rather than intrinsics or the normal generic vectors instead.
Gah. Thanks for the report and analysis. Like you say, I was hoping that adding the attributes in 10.2 but not making them matter for type compatibility would have been pretty conservative, but obviously it wasn't conservative enough. I'll take a look tomorrow.
I think this is bound up with the question whether: typedef int v4si __attribute__((vector_size(16))); typedef short v8hi __attribute__((vector_size(16))); struct s { v8hi x; v4si y; }; void foo (v4si i, v8hi h) { struct s x1 = { i, i }; struct s x2 = { h, h }; struct s x3 = { i, h }; struct s x4 = { h, i }; } should be valid with -flax-vector-conversions. g++ and clang think it should be, but gcc doesn't accept it. IMO process_init_element shouldn't recurse into vector types if the initialisation value is also a vector type. We should treat the vector value as initialising the whole type and report an error if they're not compatible.
Joseph, Marek, any thoughts on comment #5?
I guess that is reasonable thing to do, if the two vector types aren't really compatible one will get an error. But then, for trunk, won't the stripping of the attributes from vector types still mean that comp_type_attributes will return false when comparing the destination vector type (the uint8x16_t one) with the one from the binary expression (the same with stripped attribute, i.e. essentially a generic vector) and thus the initialization will be considered erroneous?
(In reply to Jakub Jelinek from comment #7) > I guess that is reasonable thing to do, if the two vector types aren't > really compatible one will get an error. > But then, for trunk, won't the stripping of the attributes from vector types > still mean that comp_type_attributes will return false when comparing the > destination vector type (the uint8x16_t one) with the one from the binary > expression (the same with stripped attribute, i.e. essentially a generic > vector) and thus the initialization will be considered erroneous? The initialization itself seems to work, since for vectors compatibility depends on vector_types_compatible_p. But the stripping of the attributes does still affect: #include <arm_neon.h> uint8x16_t foo (int c, uint8x16_t x, uint8x16_t y) { return c ? x + 1 : y; } which is wrongly rejected for C, but not C++. So I guess for trunk we need both fixes.
Created attachment 48964 [details] Patch for the initialisation problem I'm testing this patch to fix the initialisation side of the problem.
On Thu, 30 Jul 2020, rsandifo at gcc dot gnu.org wrote: > IMO process_init_element shouldn't recurse into vector types > if the initialisation value is also a vector type. We should > treat the vector value as initialising the whole type and report > an error if they're not compatible. Yes, that seems correct.
The master branch has been updated by Richard Sandiford <rsandifo@gcc.gnu.org>: https://gcc.gnu.org/g:7d599ad27b9bcf5165f87710f1abc64bbabd06ae commit r11-2481-g7d599ad27b9bcf5165f87710f1abc64bbabd06ae Author: Richard Sandiford <richard.sandiford@arm.com> Date: Sat Aug 1 12:41:28 2020 +0100 c: Fix bogus vector initialisation error [PR96377] One of the problems in this PR was that if we had: vector_type1 array[] = { vector_value1 }; process_init_element would only treat vector_value1 as initialising a vector_type1 if they had the same TYPE_MAIN_VARIANT. This has several problems: (1) It gives confusing error messages if the vector types are incompatible. (Tested by gcc.dg/pr96377-1.c.) (2) It means that we reject code that should be valid with -flax-vector-conversions. (Tested by gcc.dg/pr96377-2.c.) (3) On arm and aarch64 targets, it means that we reject some initializers that mix Advanced SIMD and standard GNU vectors. These vectors have traditionally had different TYPE_MAIN_VARIANTs because they have different mangling schemes. (Tested by gcc.dg/pr96377-[3-6].c.) (4) It means that we reject SVE initializers that should be valid. (Tested by gcc.target/aarch64/sve/gnu_vectors_[34].c.) (5) After r11-1741-g:31427b974ed7b7dd54e2 we reject: arm_neon_type1 array[] = { k ^ arm_neon_value1 }; because applying the binary operator to arm_neon_value1 strips the "Advanced SIMD type" attributes that were added in that patch. Stripping the attributes is problematic for other reasons though, so that still needs to be fixed separately. g++.target/aarch64/sve/gnu_vectors_[34].C already pass. gcc/c/ PR c/96377 * c-typeck.c (process_init_element): Split test for whether to recurse into a record, union or array into... (initialize_elementwise_p): ...this new function. Don't recurse into a vector type if the initialization value is also a vector. gcc/testsuite/ PR c/96377 * gcc.dg/pr96377-1.c: New test. * gcc.dg/pr96377-2.c: Likewise. * gcc.dg/pr96377-3.c: Likewise. * gcc.dg/pr96377-4.c: Likewise. * gcc.dg/pr96377-5.c: Likewise. * gcc.dg/pr96377-6.c: Likewise. * gcc.target/aarch64/pr96377-1.c: Likewise. * gcc.target/aarch64/sve/acle/general-c/gnu_vectors_3.c: Likewise. * gcc.target/aarch64/sve/acle/general-c/gnu_vectors_4.c: Likewise. * g++.target/aarch64/sve/acle/general-c++/gnu_vectors_3.C: Likewise. * g++.target/aarch64/sve/acle/general-c++/gnu_vectors_4.C: Likewise.
The releases/gcc-10 branch has been updated by Richard Sandiford <rsandifo@gcc.gnu.org>: https://gcc.gnu.org/g:a216daaa30bc8949086a16e7656f2025b692d03c commit r10-8562-ga216daaa30bc8949086a16e7656f2025b692d03c Author: Richard Sandiford <richard.sandiford@arm.com> Date: Mon Aug 3 09:48:36 2020 +0100 c: Fix bogus vector initialisation error [PR96377] One of the problems in this PR was that if we had: vector_type1 array[] = { vector_value1 }; process_init_element would only treat vector_value1 as initialising a vector_type1 if they had the same TYPE_MAIN_VARIANT. This has several problems: (1) It gives confusing error messages if the vector types are incompatible. (Tested by gcc.dg/pr96377-1.c.) (2) It means that we reject code that should be valid with -flax-vector-conversions. (Tested by gcc.dg/pr96377-2.c.) (3) On arm and aarch64 targets, it means that we reject some initializers that mix Advanced SIMD and standard GNU vectors. These vectors have traditionally had different TYPE_MAIN_VARIANTs because they have different mangling schemes. (Tested by gcc.dg/pr96377-[3-6].c.) (4) It means that we reject SVE initializers that should be valid. (Tested by gcc.target/aarch64/sve/gnu_vectors_[34].c.) (5) After r11-1741-g:31427b974ed7b7dd54e2 we reject: arm_neon_type1 array[] = { k ^ arm_neon_value1 }; because applying the binary operator to arm_neon_value1 strips the "Advanced SIMD type" attributes that were added in that patch. Stripping the attributes is problematic for other reasons though, so that still needs to be fixed separately. g++.target/aarch64/sve/gnu_vectors_[34].C already pass. gcc/c/ PR c/96377 * c-typeck.c (process_init_element): Split test for whether to recurse into a record, union or array into... (initialize_elementwise_p): ...this new function. Don't recurse into a vector type if the initialization value is also a vector. gcc/testsuite/ PR c/96377 * gcc.dg/pr96377-1.c: New test. * gcc.dg/pr96377-2.c: Likewise. * gcc.dg/pr96377-3.c: Likewise. * gcc.dg/pr96377-4.c: Likewise. * gcc.dg/pr96377-5.c: Likewise. * gcc.dg/pr96377-6.c: Likewise. * gcc.target/aarch64/pr96377-1.c: Likewise. * gcc.target/aarch64/sve/acle/general-c/gnu_vectors_3.c: Likewise. * gcc.target/aarch64/sve/acle/general-c/gnu_vectors_4.c: Likewise. * g++.target/aarch64/sve/acle/general-c++/gnu_vectors_3.C: Likewise. * g++.target/aarch64/sve/acle/general-c++/gnu_vectors_4.C: Likewise. (cherry picked from commit 7d599ad27b9bcf5165f87710f1abc64bbabd06ae)
*** Bug 96990 has been marked as a duplicate of this bug. ***
Fixed.
The original problem is fixed for GCC 10 and the kernel manifestation is fixed for trunk. We still need to deal with the testcase in comment 8, which is another manifestation that affects trunk only.
Please open a new bug for the remaining issue since this one was P1 blocking a 10.3 release.