[Bug c++/95726] ICE with aarch64 __Float32x4_t as template argument
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95726 Jakub Jelinek changed: What|Removed |Added Resolution|--- |FIXED Status|ASSIGNED|RESOLVED --- Comment #13 from Jakub Jelinek --- Should be fixed now. Note, PR96377 has been a regression introduced by this fix.
[Bug c++/95726] ICE with aarch64 __Float32x4_t as template argument
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95726 --- Comment #12 from CVS Commits --- The releases/gcc-10 branch has been updated by Richard Sandiford : https://gcc.gnu.org/g:932e9140d3268cf2033c1c3e93219541c53fcd29 commit r10-8501-g932e9140d3268cf2033c1c3e93219541c53fcd29 Author: Richard Sandiford Date: Wed Jul 15 11:58:04 2020 +0100 c++: Treat GNU and Advanced SIMD vectors as distinct [PR95726] This is a release branch version of r11-1741-g:31427b974ed7b7dd54e28fec595e731bf6eea8ba and r11-2022-g:efe99cca78215e339ba79f0a900a896b4c0a3d36. The trunk versions of the patch made GNU and Advanced SIMD vectors distinct (but inter-convertible) in all cases. However, the traditional behaviour is that the types are distinct in template arguments but not otherwise. Following a suggestion from Jason, this patch puts the check for different vector types under comparing_specializations. In order to keep the backport as simple as possible, the patch hard-codes the name of the attribute in the frontend rather than adding a new branch-only target hook. I didn't find a test that tripped the assert on the branch, even with the --param in the PR, so instead I tested this by forcing the hash function to only hash the tree code. That made the static assertion in the test fail without the patch but pass with it. This means that the tests pass for unmodified sources even without the patch (unless you're very unlucky). gcc/ PR target/95726 * config/aarch64/aarch64.c (aarch64_attribute_table): Add "Advanced SIMD type". * config/aarch64/aarch64-builtins.c: Include stringpool.h and attribs.h. (aarch64_init_simd_builtin_types): Add an "Advanced SIMD type" attribute to each Advanced SIMD type. * config/arm/arm.c (arm_attribute_table): Add "Advanced SIMD type". * config/arm/arm-builtins.c: Include stringpool.h and attribs.h. (arm_init_simd_builtin_types): Add an "Advanced SIMD type" attribute to each Advanced SIMD type. gcc/cp/ PR target/95726 * typeck.c (structural_comptypes): When comparing template specializations, differentiate between vectors that have and do not have an "Advanced SIMD type" attribute. gcc/testsuite/ PR target/95726 * g++.target/aarch64/pr95726.C: New test. * g++.target/arm/pr95726.C: Likewise.
[Bug c++/95726] ICE with aarch64 __Float32x4_t as template argument
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95726 --- Comment #11 from CVS Commits --- The master branch has been updated by Richard Sandiford : https://gcc.gnu.org/g:efe99cca78215e339ba79f0a900a896b4c0a3d36 commit r11-2022-gefe99cca78215e339ba79f0a900a896b4c0a3d36 Author: Richard Sandiford Date: Fri Jul 10 19:06:45 2020 +0100 arm: Treat GNU and Advanced SIMD vectors as distinct [PR92789, PR95726] This is an arm version of aarch64 patch r11-1741. The approach is essentially identical, not much more than s/aarch64/arm/. To recap, PR95726 is about template look-up for things like: foo foo The immediate cause of the problem is that the hash function usually returns different hashes for these types, yet the equality function thinks they are equal. This then raises the question of how the types are supposed to be treated. The answer we chose for AArch64 was that the GNU vector type should be treated as distinct from float32x4_t, but that each type should implicitly convert to the other. This would mean that, as far as the PR is concerned, the hashing function is right to (sometimes) treat the types differently and the equality function is wrong to treat them as the same. The most obvious way to enforce the type difference is to use a target-specific type attribute. That on its own is enough to fix the PR. The difficulty is deciding whether the knock-on effects are acceptable. One obvious effect is that GCC then rejects: typedef float vecf __attribute__((vector_size(16))); vecf x; float32x4_t = x; on the basis that the types are no longer reference-compatible. For AArch64 we took the approach that this was the correct behaviour. It is also consistent with current Clang. A trickier question is whether: vecf x; float32x4_t y; ⦠c ? x : y ⦠should be valid, and if so, what its type should be [PR92789]. As explained in the comment in the testcase, GCC and Clang both accepted this, but GCC chose the âthenâ type while Clang chose the âelseâ type. This can lead to different mangling for (probably artificial) corner cases, as seen for âsel1â and âsel2â in the testcase. Adding the attribute makes GCC reject the conditional expression as ambiguous. For AArch64 we took the approach that this too is the correct behaviour, for the reasons described in the testcase. However, it does seem to have the potential to break existing code. gcc/ PR target/92789 PR target/95726 * config/arm/arm.c (arm_attribute_table): Add "Advanced SIMD type". (arm_comp_type_attributes): Check that the "Advanced SIMD type" attributes are equal. * config/arm/arm-builtins.c: Include stringpool.h and attribs.h. (arm_mangle_builtin_vector_type): Use the mangling recorded in the "Advanced SIMD type" attribute. (arm_init_simd_builtin_types): Add an "Advanced SIMD type" attribute to each Advanced SIMD type, using the mangled type as the attribute's single argument. gcc/testsuite/ PR target/92789 PR target/95726 * g++.target/arm/pr95726.C: New test.
[Bug c++/95726] ICE with aarch64 __Float32x4_t as template argument
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95726 rsandifo at gcc dot gnu.org changed: What|Removed |Added Target|aarch64-linux |aarch64*-*-* arm*-*-* --- Comment #10 from rsandifo at gcc dot gnu.org --- Fixed on trunk for aarch64*-*-* only. Still needs fixing for arm*-*-* on trunk, and for both targets on branches.
[Bug c++/95726] ICE with aarch64 __Float32x4_t as template argument
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95726 --- Comment #9 from CVS Commits --- The master branch has been updated by Richard Sandiford : https://gcc.gnu.org/g:31427b974ed7b7dd54e28fec595e731bf6eea8ba commit r11-1741-g31427b974ed7b7dd54e28fec595e731bf6eea8ba Author: Richard Sandiford Date: Tue Jun 30 21:40:30 2020 +0100 aarch64: Treat GNU and Advanced SIMD vectors as distinct [PR92789, PR95726] PR95726 is about template look-up for things like: foo foo The immediate cause of the problem is that the hash function usually returns different hashes for these types, yet the equality function thinks they are equal. This then raises the question of how the types are supposed to be treated. I think the answer is that the GNU vector type should be treated as distinct from float32x4_t, not least because the two types mangle differently. However, each type should implicitly convert to the other. This would mean that, as far as the PR is concerned, the hashing function is right to (sometimes) treat the types differently and the equality function is wrong to treat them as the same. The most obvious way to enforce the type difference is to use a target-specific type attribute. That on its own is enough to fix the PR. The difficulty is deciding whether the knock-on effects are acceptable. One obvious effect is that GCC then rejects: typedef float vecf __attribute__((vector_size(16))); vecf x; float32x4_t = x; on the basis that the types are no longer reference-compatible. I think that's again the correct behaviour, and consistent with current Clang. A trickier question is whether: vecf x; float32x4_t y; ⦠c ? x : y ⦠should be valid, and if so, what its type should be [PR92789]. As explained in the comment in the testcase, GCC and Clang both accepted this, but GCC chose the âthenâ type while Clang chose the âelseâ type. This can lead to different mangling for (probably artificial) corner cases, as seen for âsel1â and âsel2â in the testcase. Adding the attribute makes GCC reject the conditional expression as ambiguous. I think that too is the correct behaviour, for the reasons described in the testcase. However, it does seem to have the potential to break existing code. It looks like aarch64_comp_type_attributes is missing cases for the SVE attributes, but I'll handle that in a separate patch. 2020-06-30 Richard Sandiford gcc/ PR target/92789 PR target/95726 * config/aarch64/aarch64.c (aarch64_attribute_table): Add "Advanced SIMD type". (aarch64_comp_type_attributes): Check that the "Advanced SIMD type" attributes are equal. * config/aarch64/aarch64-builtins.c: Include stringpool.h and attribs.h. (aarch64_mangle_builtin_vector_type): Use the mangling recorded in the "Advanced SIMD type" attribute. (aarch64_init_simd_builtin_types): Add an "Advanced SIMD type" attribute to each Advanced SIMD type, using the mangled type as the attribute's single argument. gcc/testsuite/ PR target/92789 PR target/95726 * g++.target/aarch64/pr95726.C: New test.
[Bug c++/95726] ICE with aarch64 __Float32x4_t as template argument
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95726 --- Comment #8 from rsandifo at gcc dot gnu.org --- Thanks for the pointers. Putting the mangled name in a target-specific attribute (like we do for SVE) seems to fix it. It actually also keeps the testcase in comment 4 “working”, which is unexpected (to me) and seems a little dubious. I guess it's good news for backporting though.
[Bug c++/95726] ICE with aarch64 __Float32x4_t as template argument
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95726 --- Comment #7 from Jason Merrill --- (In reply to Jakub Jelinek from comment #5) > Dunno, perhaps for backporting it could be done in template_args_equal > instead? For backporting we could treat them as different only if comparing_specializations is set.
[Bug c++/95726] ICE with aarch64 __Float32x4_t as template argument
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95726 --- Comment #6 from Jason Merrill --- (In reply to rsand...@gcc.gnu.org from comment #4) > (In reply to Jakub Jelinek from comment #3) > > But if they mangle differently, then structural_comptypes shouldn't treat > > them as same types. Definitely. > That certainly avoids the ICE, and makes GCC's behaviour consistent > with Clang for things like: > > typedef float vecf __attribute__((vector_size(16))); > vecf x; > float32x4_t = x; > > Previously we accepted this, with the struct_comptypes change > we reject it (like Clang does). But that might break existing > code, so I'm not sure it would be backportable. If necessary we could add a conversion between the pointer-to-vector types. > I guess the question then is: what does TYPE_STRUCTURAL_EQUALITY_P > mean for VECTOR_TYPEs in the context of structural_comptypes? The same thing it means for any other type: setting TYPE_CANONICAL properly is too hard, so use structural_comptypes. > And (if this is a different question) what case is that function's > VECTOR_TYPE handling for? I.e. when do we want to return true for > a pair of VECTOR_TYPEs whose TYPE_MAIN_VARIANTs are different? We want to return true if they should be considered the same type. Generally, TYPE_MAIN_VARIANT isn't sufficient for checking type identity, as it only looks through variants of the outermost type: If I have typedef int myint; typedef myint* myintptr; taking TYPE_MAIN_VARIANT of myintptr gives myint*, not int*. That's why TYPE_CANONICAL was introduced, so we didn't need to do structural comparison whenever we wanted to compare types.
[Bug c++/95726] ICE with aarch64 __Float32x4_t as template argument
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95726 --- Comment #5 from Jakub Jelinek --- Dunno, perhaps for backporting it could be done in template_args_equal instead? Like I said, it would probably need to handle also POINTER/REFERENCE/ARRAY_TYPE whose ultimate element type is VECTOR_TYPE possibly affected by this. Dunno about aggregates, I'd hope we set TYPE_CANONICAL for most of them and therefore shouldn't care about this. If comptypes returns false for these, can one still implicitly convert them to the other vector types? Does 32-bit ARM have similar types too? Anyway, your questions are more for Jason...
[Bug c++/95726] ICE with aarch64 __Float32x4_t as template argument
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95726 --- Comment #4 from rsandifo at gcc dot gnu.org --- (In reply to Jakub Jelinek from comment #3) > But if they mangle differently, then structural_comptypes shouldn't treat > them as same types. That certainly avoids the ICE, and makes GCC's behaviour consistent with Clang for things like: typedef float vecf __attribute__((vector_size(16))); vecf x; float32x4_t = x; Previously we accepted this, with the struct_comptypes change we reject it (like Clang does). But that might break existing code, so I'm not sure it would be backportable. I guess the question then is: what does TYPE_STRUCTURAL_EQUALITY_P mean for VECTOR_TYPEs in the context of structural_comptypes? And (if this is a different question) what case is that function's VECTOR_TYPE handling for? I.e. when do we want to return true for a pair of VECTOR_TYPEs whose TYPE_MAIN_VARIANTs are different?
[Bug c++/95726] ICE with aarch64 __Float32x4_t as template argument
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95726 rsandifo at gcc dot gnu.org changed: What|Removed |Added Status|NEW |ASSIGNED Assignee|unassigned at gcc dot gnu.org |rsandifo at gcc dot gnu.org
[Bug c++/95726] ICE with aarch64 __Float32x4_t as template argument
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95726 --- Comment #3 from Jakub Jelinek --- (In reply to rsand...@gcc.gnu.org from comment #2) > They mangle differently, and e.g.: > > void f(float32x4_t); > void f(V); > > aren't ODR equivalent. But a lot of code relies on the GNU vector > extensions being available for float32x4_t as well as V, and on V > and float32x4_t being mutually and implicitly interconvertible. But if they mangle differently, then structural_comptypes shouldn't treat them as same types.
[Bug c++/95726] ICE with aarch64 __Float32x4_t as template argument
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95726 --- Comment #2 from rsandifo at gcc dot gnu.org --- (In reply to Jason Merrill from comment #1) > Does the aarch64 port expect __Float32x4_t type to be considered equivalent > to the GNU vector type or not? If so, why use build_distinct_type_copy over > build_variant_type_copy? If not, they might want to set TYPE_INDIVISIBLE_P > so that structural_comptypes treats them as different. They mangle differently, and e.g.: void f(float32x4_t); void f(V); aren't ODR equivalent. But a lot of code relies on the GNU vector extensions being available for float32x4_t as well as V, and on V and float32x4_t being mutually and implicitly interconvertible.
[Bug c++/95726] ICE with aarch64 __Float32x4_t as template argument
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95726 Jason Merrill changed: What|Removed |Added Ever confirmed|0 |1 Status|UNCONFIRMED |NEW Last reconfirmed||2020-06-17
[Bug c++/95726] ICE with aarch64 __Float32x4_t as template argument
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95726 --- Comment #1 from Jason Merrill --- Does the aarch64 port expect __Float32x4_t type to be considered equivalent to the GNU vector type or not? If so, why use build_distinct_type_copy over build_variant_type_copy? If not, they might want to set TYPE_INDIVISIBLE_P so that structural_comptypes treats them as different.
