* [PATCH/AARCH64] Fix 64893: ICE with vget_lane_u32 with C++ front-end at -O0 @ 2015-02-02 22:51 Andrew Pinski 2015-02-03 7:37 ` Jakub Jelinek 2015-02-03 11:57 ` Alan Lawrence 0 siblings, 2 replies; 11+ messages in thread From: Andrew Pinski @ 2015-02-02 22:51 UTC (permalink / raw) To: GCC Patches, Alan Lawrence [-- Attachment #1: Type: text/plain, Size: 1528 bytes --] While trying to build the GCC 5 with GCC 5, I ran into an ICE when building libcpp at -O0. The problem is the C++ front-end was not folding sizeof(a)/sizeof(a[0]) when passed to a function at -O0. The C++ front-end keeps around sizeof until the gimplifier and there is no way to fold the expressions that involve them. So to work around the issue we need to change __builtin_aarch64_im_lane_boundsi to accept an extra argument and change the first two arguments to size_t type so we don't get an extra cast there and do the division inside the compiler itself. Also we don't want to cause an ICE on any source code so I changed the assert to be a sorry if either of the two arguments are not integer constants. OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions and I was able to bootstrap without a modified libcpp. Thanks, Andrew Pinski ChangeLog: * config/aarch64/aarch64-builtins.c (aarch64_init_simd_builtins): Change the first argument type to size_type_node and add another size_type_node. (aarch64_simd_expand_builtin): Handle the new argument to AARCH64_SIMD_BUILTIN_LANE_CHECK and don't ICE but rather print sorry out when the first two arguments are not integer constants. * config/aarch64/arm_neon.h (__AARCH64_LANE_CHECK): Pass the sizeof's directly to __builtin_aarch64_im_lane_boundsi. testsuite/ChangeLog: * c-c++-common/torture/aarch64-vect-lane-1.c: New testcase. [-- Attachment #2: fixPR64893.diff.txt --] [-- Type: text/plain, Size: 3566 bytes --] commit 455a54f36a205af281b3fe8dbc97916ede704ca8 Author: Andrew Pinski <apinski@cavium.com> Date: Mon Feb 2 18:40:08 2015 +0000 Fix bug 64893: ICE with vget_lane_u32 with C++ front-end PR target/64893 * config/aarch64/aarch64-builtins.c (aarch64_init_simd_builtins): Change the first argument type to size_type_node and add another size_type_node. (aarch64_simd_expand_builtin): Handle the new argument to AARCH64_SIMD_BUILTIN_LANE_CHECK and don't ICE but rather print sorry out when the first two arguments are not integer constants. * config/aarch64/arm_neon.h (__AARCH64_LANE_CHECK): Pass the sizeof directly to __builtin_aarch64_im_lane_boundsi. * testsuite/c-c++-common/torture/aarch64-vect-lane-1.c: New testcase. diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c index 87f1ac2..5bd15d1 100644 --- a/gcc/config/aarch64/aarch64-builtins.c +++ b/gcc/config/aarch64/aarch64-builtins.c @@ -712,7 +712,8 @@ aarch64_init_simd_builtins (void) aarch64_init_simd_builtin_scalar_types (); tree lane_check_fpr = build_function_type_list (void_type_node, - intSI_type_node, + size_type_node, + size_type_node, intSI_type_node, NULL); aarch64_builtin_decls[AARCH64_SIMD_BUILTIN_LANE_CHECK] = @@ -1001,13 +1002,18 @@ aarch64_simd_expand_builtin (int fcode, tree exp, rtx target) { if (fcode == AARCH64_SIMD_BUILTIN_LANE_CHECK) { - tree nlanes = CALL_EXPR_ARG (exp, 0); - gcc_assert (TREE_CODE (nlanes) == INTEGER_CST); - rtx lane_idx = expand_normal (CALL_EXPR_ARG (exp, 1)); - if (CONST_INT_P (lane_idx)) - aarch64_simd_lane_bounds (lane_idx, 0, TREE_INT_CST_LOW (nlanes), exp); + rtx totalsize = expand_normal (CALL_EXPR_ARG (exp, 0)); + rtx elementsize = expand_normal (CALL_EXPR_ARG (exp, 1)); + if (CONST_INT_P (totalsize) && CONST_INT_P (elementsize)) + { + rtx lane_idx = expand_normal (CALL_EXPR_ARG (exp, 2)); + if (CONST_INT_P (lane_idx)) + aarch64_simd_lane_bounds (lane_idx, 0, UINTVAL (totalsize)/UINTVAL (elementsize), exp); + else + error ("%Klane index must be a constant immediate", exp); + } else - error ("%Klane index must be a constant immediate", exp); + sorry ("%Ktotal size and element size must be a constant immediate", exp); /* Don't generate any RTL. */ return const0_rtx; } diff --git a/gcc/config/aarch64/arm_neon.h b/gcc/config/aarch64/arm_neon.h index d4ce0b8..938a3cc 100644 --- a/gcc/config/aarch64/arm_neon.h +++ b/gcc/config/aarch64/arm_neon.h @@ -541,7 +541,7 @@ typedef struct poly16x8x4_t #define __AARCH64_NUM_LANES(__v) (sizeof (__v) / sizeof (__v[0])) #define __AARCH64_LANE_CHECK(__vec, __idx) \ - __builtin_aarch64_im_lane_boundsi (__AARCH64_NUM_LANES (__vec), __idx) + __builtin_aarch64_im_lane_boundsi (sizeof(__vec), sizeof(__vec[0]), __idx) /* For big-endian, GCC's vector indices are the opposite way around to the architectural lane indices used by Neon intrinsics. */ diff --git a/gcc/testsuite/c-c++-common/torture/aarch64-vect-lane-1.c b/gcc/testsuite/c-c++-common/torture/aarch64-vect-lane-1.c new file mode 100644 index 0000000..1790c34 --- /dev/null +++ b/gcc/testsuite/c-c++-common/torture/aarch64-vect-lane-1.c @@ -0,0 +1,8 @@ +// { dg-do compile { target "aarch64*-*-*" } } +#include <arm_neon.h> +int +search_line_fast (uint32x2_t t) +{ + return vget_lane_u32 (t, 0); +} + ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH/AARCH64] Fix 64893: ICE with vget_lane_u32 with C++ front-end at -O0 2015-02-02 22:51 [PATCH/AARCH64] Fix 64893: ICE with vget_lane_u32 with C++ front-end at -O0 Andrew Pinski @ 2015-02-03 7:37 ` Jakub Jelinek 2015-02-07 1:02 ` Andrew Pinski 2015-02-03 11:57 ` Alan Lawrence 1 sibling, 1 reply; 11+ messages in thread From: Jakub Jelinek @ 2015-02-03 7:37 UTC (permalink / raw) To: Andrew Pinski; +Cc: GCC Patches, Alan Lawrence On Mon, Feb 02, 2015 at 02:51:43PM -0800, Andrew Pinski wrote: > While trying to build the GCC 5 with GCC 5, I ran into an ICE when > building libcpp at -O0. The problem is the C++ front-end was not > folding sizeof(a)/sizeof(a[0]) when passed to a function at -O0. The > C++ front-end keeps around sizeof until the gimplifier and there is no > way to fold the expressions that involve them. So to work around the > issue we need to change __builtin_aarch64_im_lane_boundsi to accept an > extra argument and change the first two arguments to size_t type so we > don't get an extra cast there and do the division inside the compiler > itself. Relying on anything being folded at -O0 when the language does not guarantee it is going to be more and more of a problem. So I think your patch is reasonable (of course, I'll defer this to target maintainers). > + rtx totalsize = expand_normal (CALL_EXPR_ARG (exp, 0)); > + rtx elementsize = expand_normal (CALL_EXPR_ARG (exp, 1)); > + if (CONST_INT_P (totalsize) && CONST_INT_P (elementsize)) > + { > + rtx lane_idx = expand_normal (CALL_EXPR_ARG (exp, 2)); > + if (CONST_INT_P (lane_idx)) > + aarch64_simd_lane_bounds (lane_idx, 0, UINTVAL (totalsize)/UINTVAL (elementsize), exp); Too long line? Also, missing spaces around / . And, ICE if somebody uses __builtin_aarch64_im_lane_boundsi (4, 0, 0); So you need to check and complain for zero elementsize too. > + else > + error ("%Klane index must be a constant immediate", exp); > + } > else > - error ("%Klane index must be a constant immediate", exp); > + sorry ("%Ktotal size and element size must be a constant immediate", exp); But why sorry? If you say the builtin requires constant arguments, then it is not sorry, but error, it is not an unimplemented feature. Jakub ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH/AARCH64] Fix 64893: ICE with vget_lane_u32 with C++ front-end at -O0 2015-02-03 7:37 ` Jakub Jelinek @ 2015-02-07 1:02 ` Andrew Pinski 2015-02-08 2:24 ` Andrew Pinski 0 siblings, 1 reply; 11+ messages in thread From: Andrew Pinski @ 2015-02-07 1:02 UTC (permalink / raw) To: Jakub Jelinek; +Cc: GCC Patches, Alan Lawrence On Mon, Feb 2, 2015 at 11:37 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Mon, Feb 02, 2015 at 02:51:43PM -0800, Andrew Pinski wrote: >> While trying to build the GCC 5 with GCC 5, I ran into an ICE when >> building libcpp at -O0. The problem is the C++ front-end was not >> folding sizeof(a)/sizeof(a[0]) when passed to a function at -O0. The >> C++ front-end keeps around sizeof until the gimplifier and there is no >> way to fold the expressions that involve them. So to work around the >> issue we need to change __builtin_aarch64_im_lane_boundsi to accept an >> extra argument and change the first two arguments to size_t type so we >> don't get an extra cast there and do the division inside the compiler >> itself. > > Relying on anything being folded at -O0 when the language does not guarantee > it is going to be more and more of a problem. So I think your patch is > reasonable (of course, I'll defer this to target maintainers). > >> + rtx totalsize = expand_normal (CALL_EXPR_ARG (exp, 0)); >> + rtx elementsize = expand_normal (CALL_EXPR_ARG (exp, 1)); >> + if (CONST_INT_P (totalsize) && CONST_INT_P (elementsize)) >> + { >> + rtx lane_idx = expand_normal (CALL_EXPR_ARG (exp, 2)); >> + if (CONST_INT_P (lane_idx)) >> + aarch64_simd_lane_bounds (lane_idx, 0, UINTVAL (totalsize)/UINTVAL (elementsize), exp); > > Too long line? Also, missing spaces around / . And, ICE if > somebody uses __builtin_aarch64_im_lane_boundsi (4, 0, 0); > So you need to check and complain for zero elementsize too. Good points, I don't know why I missed this. > >> + else >> + error ("%Klane index must be a constant immediate", exp); >> + } >> else >> - error ("%Klane index must be a constant immediate", exp); >> + sorry ("%Ktotal size and element size must be a constant immediate", exp); > > But why sorry? If you say the builtin requires constant arguments, then it > is not sorry, but error, it is not an unimplemented feature. Because I originally thought that would be better than error but now thinking this over, I was incorrect, it should be an error. I will add a testcase for the __builtin_aarch64_im_lane_boundsi (4, 0, 0) case and retest the patch. Thanks, Andrew > > Jakub ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH/AARCH64] Fix 64893: ICE with vget_lane_u32 with C++ front-end at -O0 2015-02-07 1:02 ` Andrew Pinski @ 2015-02-08 2:24 ` Andrew Pinski 2015-02-11 10:11 ` Marcus Shawcroft 2015-02-12 15:37 ` Christophe Lyon 0 siblings, 2 replies; 11+ messages in thread From: Andrew Pinski @ 2015-02-08 2:24 UTC (permalink / raw) To: Jakub Jelinek; +Cc: GCC Patches, Alan Lawrence [-- Attachment #1: Type: text/plain, Size: 3308 bytes --] On Fri, Feb 6, 2015 at 5:02 PM, Andrew Pinski <pinskia@gmail.com> wrote: > On Mon, Feb 2, 2015 at 11:37 PM, Jakub Jelinek <jakub@redhat.com> wrote: >> On Mon, Feb 02, 2015 at 02:51:43PM -0800, Andrew Pinski wrote: >>> While trying to build the GCC 5 with GCC 5, I ran into an ICE when >>> building libcpp at -O0. The problem is the C++ front-end was not >>> folding sizeof(a)/sizeof(a[0]) when passed to a function at -O0. The >>> C++ front-end keeps around sizeof until the gimplifier and there is no >>> way to fold the expressions that involve them. So to work around the >>> issue we need to change __builtin_aarch64_im_lane_boundsi to accept an >>> extra argument and change the first two arguments to size_t type so we >>> don't get an extra cast there and do the division inside the compiler >>> itself. >> >> Relying on anything being folded at -O0 when the language does not guarantee >> it is going to be more and more of a problem. So I think your patch is >> reasonable (of course, I'll defer this to target maintainers). >> >>> + rtx totalsize = expand_normal (CALL_EXPR_ARG (exp, 0)); >>> + rtx elementsize = expand_normal (CALL_EXPR_ARG (exp, 1)); >>> + if (CONST_INT_P (totalsize) && CONST_INT_P (elementsize)) >>> + { >>> + rtx lane_idx = expand_normal (CALL_EXPR_ARG (exp, 2)); >>> + if (CONST_INT_P (lane_idx)) >>> + aarch64_simd_lane_bounds (lane_idx, 0, UINTVAL (totalsize)/UINTVAL (elementsize), exp); >> >> Too long line? Also, missing spaces around / . And, ICE if >> somebody uses __builtin_aarch64_im_lane_boundsi (4, 0, 0); >> So you need to check and complain for zero elementsize too. > > Good points, I don't know why I missed this. > >> >>> + else >>> + error ("%Klane index must be a constant immediate", exp); >>> + } >>> else >>> - error ("%Klane index must be a constant immediate", exp); >>> + sorry ("%Ktotal size and element size must be a constant immediate", exp); >> >> But why sorry? If you say the builtin requires constant arguments, then it >> is not sorry, but error, it is not an unimplemented feature. > > Because I originally thought that would be better than error but now > thinking this over, I was incorrect, it should be an error. > I will add a testcase for the __builtin_aarch64_im_lane_boundsi (4, 0, > 0) case and retest the patch. Here is the updated patch with Jakub's comments included and added a testcase for the 0, 0 case. Thanks, Andrew Pinski ChangeLog: PR target/64893 * config/aarch64/aarch64-builtins.c (aarch64_init_simd_builtins): Change the first argument type to size_type_node and add another size_type_node. (aarch64_simd_expand_builtin): Handle the new argument to AARCH64_SIMD_BUILTIN_LANE_CHECK and don't ICE but rather print an out when the first two arguments are not nonzero integer constants. * config/aarch64/arm_neon.h (__AARCH64_LANE_CHECK): Pass the sizeof directly to __builtin_aarch64_im_lane_boundsi. testsuite/ChangeLog: * c-c++-common/torture/aarch64-vect-lane-1.c: New testcase. * c-c++-common/torture/aarch64-vect-lane-2.c: New testcase. > > Thanks, > Andrew > > >> >> Jakub [-- Attachment #2: fixPR64893.diff.txt --] [-- Type: text/plain, Size: 4194 bytes --] commit b5c809bddf8a34f490ee0cd540f12be1b8b2b897 Author: Andrew Pinski <apinski@cavium.com> Date: Mon Feb 2 18:40:08 2015 +0000 Fix bug 64893: ICE with vget_lane_u32 with C++ front-end PR target/64893 * config/aarch64/aarch64-builtins.c (aarch64_init_simd_builtins): Change the first argument type to size_type_node and add another size_type_node. (aarch64_simd_expand_builtin): Handle the new argument to AARCH64_SIMD_BUILTIN_LANE_CHECK and don't ICE but rather print an out when the first two arguments are not nonzero integer constants. * config/aarch64/arm_neon.h (__AARCH64_LANE_CHECK): Pass the sizeof directly to __builtin_aarch64_im_lane_boundsi. * testsuite/c-c++-common/torture/aarch64-vect-lane-1.c: New testcase. * testsuite/c-c++-common/torture/aarch64-vect-lane-2.c: New testcase. diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c index 87f1ac2..eabf873 100644 --- a/gcc/config/aarch64/aarch64-builtins.c +++ b/gcc/config/aarch64/aarch64-builtins.c @@ -712,7 +712,8 @@ aarch64_init_simd_builtins (void) aarch64_init_simd_builtin_scalar_types (); tree lane_check_fpr = build_function_type_list (void_type_node, - intSI_type_node, + size_type_node, + size_type_node, intSI_type_node, NULL); aarch64_builtin_decls[AARCH64_SIMD_BUILTIN_LANE_CHECK] = @@ -1000,14 +1001,24 @@ rtx aarch64_simd_expand_builtin (int fcode, tree exp, rtx target) { if (fcode == AARCH64_SIMD_BUILTIN_LANE_CHECK) - { - tree nlanes = CALL_EXPR_ARG (exp, 0); - gcc_assert (TREE_CODE (nlanes) == INTEGER_CST); - rtx lane_idx = expand_normal (CALL_EXPR_ARG (exp, 1)); - if (CONST_INT_P (lane_idx)) - aarch64_simd_lane_bounds (lane_idx, 0, TREE_INT_CST_LOW (nlanes), exp); +{ + rtx totalsize = expand_normal (CALL_EXPR_ARG (exp, 0)); + rtx elementsize = expand_normal (CALL_EXPR_ARG (exp, 1)); + if (CONST_INT_P (totalsize) && CONST_INT_P (elementsize) + && UINTVAL (elementsize) != 0 + && UINTVAL (totalsize) != 0) + { + rtx lane_idx = expand_normal (CALL_EXPR_ARG (exp, 2)); + if (CONST_INT_P (lane_idx)) + aarch64_simd_lane_bounds (lane_idx, 0, + UINTVAL (totalsize) + / UINTVAL (elementsize), + exp); + else + error ("%Klane index must be a constant immediate", exp); + } else - error ("%Klane index must be a constant immediate", exp); + error ("%Ktotal size and element size must be a non-zero constant immediate", exp); /* Don't generate any RTL. */ return const0_rtx; } diff --git a/gcc/config/aarch64/arm_neon.h b/gcc/config/aarch64/arm_neon.h index 2525a27..4c15312 100644 --- a/gcc/config/aarch64/arm_neon.h +++ b/gcc/config/aarch64/arm_neon.h @@ -541,7 +541,7 @@ typedef struct poly16x8x4_t #define __AARCH64_NUM_LANES(__v) (sizeof (__v) / sizeof (__v[0])) #define __AARCH64_LANE_CHECK(__vec, __idx) \ - __builtin_aarch64_im_lane_boundsi (__AARCH64_NUM_LANES (__vec), __idx) + __builtin_aarch64_im_lane_boundsi (sizeof(__vec), sizeof(__vec[0]), __idx) /* For big-endian, GCC's vector indices are the opposite way around to the architectural lane indices used by Neon intrinsics. */ diff --git a/gcc/testsuite/c-c++-common/torture/aarch64-vect-lane-1.c b/gcc/testsuite/c-c++-common/torture/aarch64-vect-lane-1.c new file mode 100644 index 0000000..1790c34 --- /dev/null +++ b/gcc/testsuite/c-c++-common/torture/aarch64-vect-lane-1.c @@ -0,0 +1,8 @@ +// { dg-do compile { target "aarch64*-*-*" } } +#include <arm_neon.h> +int +search_line_fast (uint32x2_t t) +{ + return vget_lane_u32 (t, 0); +} + diff --git a/gcc/testsuite/c-c++-common/torture/aarch64-vect-lane-2.c b/gcc/testsuite/c-c++-common/torture/aarch64-vect-lane-2.c new file mode 100644 index 0000000..2950480 --- /dev/null +++ b/gcc/testsuite/c-c++-common/torture/aarch64-vect-lane-2.c @@ -0,0 +1,7 @@ +// { dg-do compile { target "aarch64*-*-*" } } +int +search_line_fast (void) +{ + __builtin_aarch64_im_lane_boundsi (4, 0, 0); /* { dg-error "" } */ +} + ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH/AARCH64] Fix 64893: ICE with vget_lane_u32 with C++ front-end at -O0 2015-02-08 2:24 ` Andrew Pinski @ 2015-02-11 10:11 ` Marcus Shawcroft 2015-02-12 15:37 ` Christophe Lyon 1 sibling, 0 replies; 11+ messages in thread From: Marcus Shawcroft @ 2015-02-11 10:11 UTC (permalink / raw) To: Andrew Pinski; +Cc: Jakub Jelinek, GCC Patches, Alan Lawrence On 8 February 2015 at 02:24, Andrew Pinski <pinskia@gmail.com> wrote: > Here is the updated patch with Jakub's comments included and added a > testcase for the 0, 0 case. > > Thanks, > Andrew Pinski > ChangeLog: > > PR target/64893 > * config/aarch64/aarch64-builtins.c (aarch64_init_simd_builtins): > Change the first argument type to size_type_node and add another > size_type_node. > (aarch64_simd_expand_builtin): Handle the new argument to > AARCH64_SIMD_BUILTIN_LANE_CHECK and don't ICE but rather > print an out when the first two arguments are not > nonzero integer constants. > * config/aarch64/arm_neon.h (__AARCH64_LANE_CHECK): > Pass the sizeof directly to __builtin_aarch64_im_lane_boundsi. > > testsuite/ChangeLog: > > * c-c++-common/torture/aarch64-vect-lane-1.c: New testcase. > * c-c++-common/torture/aarch64-vect-lane-2.c: New testcase. OK, thanks /Marcus ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH/AARCH64] Fix 64893: ICE with vget_lane_u32 with C++ front-end at -O0 2015-02-08 2:24 ` Andrew Pinski 2015-02-11 10:11 ` Marcus Shawcroft @ 2015-02-12 15:37 ` Christophe Lyon 2015-03-06 9:45 ` James Greenhalgh 1 sibling, 1 reply; 11+ messages in thread From: Christophe Lyon @ 2015-02-12 15:37 UTC (permalink / raw) To: Andrew Pinski; +Cc: Jakub Jelinek, GCC Patches, Alan Lawrence On 8 February 2015 at 03:24, Andrew Pinski <pinskia@gmail.com> wrote: > On Fri, Feb 6, 2015 at 5:02 PM, Andrew Pinski <pinskia@gmail.com> wrote: >> On Mon, Feb 2, 2015 at 11:37 PM, Jakub Jelinek <jakub@redhat.com> wrote: >>> On Mon, Feb 02, 2015 at 02:51:43PM -0800, Andrew Pinski wrote: >>>> While trying to build the GCC 5 with GCC 5, I ran into an ICE when >>>> building libcpp at -O0. The problem is the C++ front-end was not >>>> folding sizeof(a)/sizeof(a[0]) when passed to a function at -O0. The >>>> C++ front-end keeps around sizeof until the gimplifier and there is no >>>> way to fold the expressions that involve them. So to work around the >>>> issue we need to change __builtin_aarch64_im_lane_boundsi to accept an >>>> extra argument and change the first two arguments to size_t type so we >>>> don't get an extra cast there and do the division inside the compiler >>>> itself. >>> >>> Relying on anything being folded at -O0 when the language does not guarantee >>> it is going to be more and more of a problem. So I think your patch is >>> reasonable (of course, I'll defer this to target maintainers). >>> >>>> + rtx totalsize = expand_normal (CALL_EXPR_ARG (exp, 0)); >>>> + rtx elementsize = expand_normal (CALL_EXPR_ARG (exp, 1)); >>>> + if (CONST_INT_P (totalsize) && CONST_INT_P (elementsize)) >>>> + { >>>> + rtx lane_idx = expand_normal (CALL_EXPR_ARG (exp, 2)); >>>> + if (CONST_INT_P (lane_idx)) >>>> + aarch64_simd_lane_bounds (lane_idx, 0, UINTVAL (totalsize)/UINTVAL (elementsize), exp); >>> >>> Too long line? Also, missing spaces around / . And, ICE if >>> somebody uses __builtin_aarch64_im_lane_boundsi (4, 0, 0); >>> So you need to check and complain for zero elementsize too. >> >> Good points, I don't know why I missed this. >> >>> >>>> + else >>>> + error ("%Klane index must be a constant immediate", exp); >>>> + } >>>> else >>>> - error ("%Klane index must be a constant immediate", exp); >>>> + sorry ("%Ktotal size and element size must be a constant immediate", exp); >>> >>> But why sorry? If you say the builtin requires constant arguments, then it >>> is not sorry, but error, it is not an unimplemented feature. >> >> Because I originally thought that would be better than error but now >> thinking this over, I was incorrect, it should be an error. >> I will add a testcase for the __builtin_aarch64_im_lane_boundsi (4, 0, >> 0) case and retest the patch. > > > Here is the updated patch with Jakub's comments included and added a > testcase for the 0, 0 case. > > Thanks, > Andrew Pinski > ChangeLog: > > PR target/64893 > * config/aarch64/aarch64-builtins.c (aarch64_init_simd_builtins): > Change the first argument type to size_type_node and add another > size_type_node. > (aarch64_simd_expand_builtin): Handle the new argument to > AARCH64_SIMD_BUILTIN_LANE_CHECK and don't ICE but rather > print an out when the first two arguments are not > nonzero integer constants. > * config/aarch64/arm_neon.h (__AARCH64_LANE_CHECK): > Pass the sizeof directly to __builtin_aarch64_im_lane_boundsi. > Hi Andrew, > testsuite/ChangeLog: > > * c-c++-common/torture/aarch64-vect-lane-1.c: New testcase. > * c-c++-common/torture/aarch64-vect-lane-2.c: New testcase. In case you haven't noticed, aarch64-vect-lane-2.c FAILs when compiled at -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects but PASSes with the other optimization levels. Christophe. > >> >> Thanks, >> Andrew >> >> >>> >>> Jakub ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH/AARCH64] Fix 64893: ICE with vget_lane_u32 with C++ front-end at -O0 2015-02-12 15:37 ` Christophe Lyon @ 2015-03-06 9:45 ` James Greenhalgh 2015-03-06 10:03 ` pinskia 0 siblings, 1 reply; 11+ messages in thread From: James Greenhalgh @ 2015-03-06 9:45 UTC (permalink / raw) To: Christophe Lyon; +Cc: Andrew Pinski, Jakub Jelinek, GCC Patches, Alan Lawrence On Thu, Feb 12, 2015 at 03:37:33PM +0000, Christophe Lyon wrote: > On 8 February 2015 at 03:24, Andrew Pinski <pinskia@gmail.com> wrote: > > On Fri, Feb 6, 2015 at 5:02 PM, Andrew Pinski <pinskia@gmail.com> wrote: > > PR target/64893 > > * config/aarch64/aarch64-builtins.c (aarch64_init_simd_builtins): > > Change the first argument type to size_type_node and add another > > size_type_node. > > (aarch64_simd_expand_builtin): Handle the new argument to > > AARCH64_SIMD_BUILTIN_LANE_CHECK and don't ICE but rather > > print an out when the first two arguments are not > > nonzero integer constants. > > * config/aarch64/arm_neon.h (__AARCH64_LANE_CHECK): > > Pass the sizeof directly to __builtin_aarch64_im_lane_boundsi. > > > > testsuite/ChangeLog: > > > > * c-c++-common/torture/aarch64-vect-lane-1.c: New testcase. > > * c-c++-common/torture/aarch64-vect-lane-2.c: New testcase. > In case you haven't noticed, aarch64-vect-lane-2.c FAILs when compiled at > -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects > but PASSes with the other optimization levels. Hi Andrew, Did you get a chance to look at this? Given the way that your error checking works, the test probably just needs a dg-skip-if -flto directive. Did you intend to come back to this and make it fail earlier than link time with -flto? If I don't hear otherwise, I'll propose the patch adding dg-skip-if on Monday. Cheers, James ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH/AARCH64] Fix 64893: ICE with vget_lane_u32 with C++ front-end at -O0 2015-03-06 9:45 ` James Greenhalgh @ 2015-03-06 10:03 ` pinskia 2015-03-06 10:51 ` James Greenhalgh 0 siblings, 1 reply; 11+ messages in thread From: pinskia @ 2015-03-06 10:03 UTC (permalink / raw) To: James Greenhalgh Cc: Christophe Lyon, Jakub Jelinek, GCC Patches, Alan Lawrence > On Mar 6, 2015, at 1:45 AM, James Greenhalgh <james.greenhalgh@arm.com> wrote: > >> On Thu, Feb 12, 2015 at 03:37:33PM +0000, Christophe Lyon wrote: >>> On 8 February 2015 at 03:24, Andrew Pinski <pinskia@gmail.com> wrote: >>> On Fri, Feb 6, 2015 at 5:02 PM, Andrew Pinski <pinskia@gmail.com> wrote: >>> PR target/64893 >>> * config/aarch64/aarch64-builtins.c (aarch64_init_simd_builtins): >>> Change the first argument type to size_type_node and add another >>> size_type_node. >>> (aarch64_simd_expand_builtin): Handle the new argument to >>> AARCH64_SIMD_BUILTIN_LANE_CHECK and don't ICE but rather >>> print an out when the first two arguments are not >>> nonzero integer constants. >>> * config/aarch64/arm_neon.h (__AARCH64_LANE_CHECK): >>> Pass the sizeof directly to __builtin_aarch64_im_lane_boundsi. >>> >>> testsuite/ChangeLog: >>> >>> * c-c++-common/torture/aarch64-vect-lane-1.c: New testcase. >>> * c-c++-common/torture/aarch64-vect-lane-2.c: New testcase. >> In case you haven't noticed, aarch64-vect-lane-2.c FAILs when compiled at >> -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects >> but PASSes with the other optimization levels. > > Hi Andrew, > > Did you get a chance to look at this? Given the way that your error > checking works, the test probably just needs a dg-skip-if -flto > directive. Did you intend to come back to this and make it fail earlier > than link time with -flto? > > If I don't hear otherwise, I'll propose the patch adding dg-skip-if on > Monday. Yes that is the correct approach as I see it. Thanks, Andrew > > Cheers, > James > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH/AARCH64] Fix 64893: ICE with vget_lane_u32 with C++ front-end at -O0 2015-03-06 10:03 ` pinskia @ 2015-03-06 10:51 ` James Greenhalgh 0 siblings, 0 replies; 11+ messages in thread From: James Greenhalgh @ 2015-03-06 10:51 UTC (permalink / raw) To: pinskia; +Cc: Christophe Lyon, Jakub Jelinek, GCC Patches, Alan Lawrence [-- Attachment #1: Type: text/plain, Size: 2224 bytes --] On Fri, Mar 06, 2015 at 10:03:46AM +0000, pinskia@gmail.com wrote: > > On Mar 6, 2015, at 1:45 AM, James Greenhalgh <james.greenhalgh@arm.com> wrote: > > > >> On Thu, Feb 12, 2015 at 03:37:33PM +0000, Christophe Lyon wrote: > >>> On 8 February 2015 at 03:24, Andrew Pinski <pinskia@gmail.com> wrote: > >>> On Fri, Feb 6, 2015 at 5:02 PM, Andrew Pinski <pinskia@gmail.com> wrote: > >>> PR target/64893 > >>> * config/aarch64/aarch64-builtins.c (aarch64_init_simd_builtins): > >>> Change the first argument type to size_type_node and add another > >>> size_type_node. > >>> (aarch64_simd_expand_builtin): Handle the new argument to > >>> AARCH64_SIMD_BUILTIN_LANE_CHECK and don't ICE but rather > >>> print an out when the first two arguments are not > >>> nonzero integer constants. > >>> * config/aarch64/arm_neon.h (__AARCH64_LANE_CHECK): > >>> Pass the sizeof directly to __builtin_aarch64_im_lane_boundsi. > >>> > >>> testsuite/ChangeLog: > >>> > >>> * c-c++-common/torture/aarch64-vect-lane-1.c: New testcase. > >>> * c-c++-common/torture/aarch64-vect-lane-2.c: New testcase. > >> In case you haven't noticed, aarch64-vect-lane-2.c FAILs when compiled at > >> -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects > >> but PASSes with the other optimization levels. > > > > Hi Andrew, > > > > Did you get a chance to look at this? Given the way that your error > > checking works, the test probably just needs a dg-skip-if -flto > > directive. Did you intend to come back to this and make it fail earlier > > than link time with -flto? > > > > If I don't hear otherwise, I'll propose the patch adding dg-skip-if on > > Monday. > > Yes that is the correct approach as I see it. Thanks for the confirmation. In the end I settled on XFAILing (It would be nice to catch the error early enough that one day this test PASSes) the test rather than skipping it. I committed the attached patch as obvious as r221233. Thanks, James --- 2015-03-06 James Greenhalgh <james.greenhalgh@arm.com> * c-c++-common/torture/aarch64-vect-lane-2.c: XFAIL for LTO compiles using the linker plugin. [-- Attachment #2: xfail-aarch64-vect-lane.patch --] [-- Type: text/x-diff, Size: 937 bytes --] Index: gcc/testsuite/ChangeLog =================================================================== --- gcc/testsuite/ChangeLog (revision 221232) +++ gcc/testsuite/ChangeLog (working copy) @@ -1,3 +1,8 @@ +2015-03-06 James Greenhalgh <james.greenhalgh@arm.com> + + * c-c++-common/torture/aarch64-vect-lane-2.c: XFAIL for LTO + compiles using the linker plugin. + 2015-03-06 Eric Botcazou <ebotcazou@adacore.com> * g++.dg/other/dump-ada-spec-3.C: Remove include and adjust. Index: gcc/testsuite/c-c++-common/torture/aarch64-vect-lane-2.c =================================================================== --- gcc/testsuite/c-c++-common/torture/aarch64-vect-lane-2.c (revision 221232) +++ gcc/testsuite/c-c++-common/torture/aarch64-vect-lane-2.c (working copy) @@ -1,4 +1,5 @@ // { dg-do compile { target "aarch64*-*-*" } } +// { dg-xfail-if "" { *-*-* } { "-flto -fuse-linker-plugin" } { "" } } int search_line_fast (void) { ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH/AARCH64] Fix 64893: ICE with vget_lane_u32 with C++ front-end at -O0 2015-02-02 22:51 [PATCH/AARCH64] Fix 64893: ICE with vget_lane_u32 with C++ front-end at -O0 Andrew Pinski 2015-02-03 7:37 ` Jakub Jelinek @ 2015-02-03 11:57 ` Alan Lawrence 2015-02-03 13:01 ` pinskia 1 sibling, 1 reply; 11+ messages in thread From: Alan Lawrence @ 2015-02-03 11:57 UTC (permalink / raw) To: Andrew Pinski; +Cc: gcc-patches Andrew Pinski wrote: > While trying to build the GCC 5 with GCC 5, I ran into an ICE when > building libcpp at -O0. The problem is the C++ front-end was not > folding sizeof(a)/sizeof(a[0]) when passed to a function at -O0. The > C++ front-end keeps around sizeof until the gimplifier and there is no > way to fold the expressions that involve them. So to work around the > issue we need to change __builtin_aarch64_im_lane_boundsi to accept an > extra argument and change the first two arguments to size_t type so we > don't get an extra cast there and do the division inside the compiler > itself. > > Also we don't want to cause an ICE on any source code so I changed the > assert to be a sorry if either of the two arguments are not integer > constants. TBH I think it _is_ appropriate to ICE rather than sorry, or even error, if the size of the vector or vector elements are non-constant or zero. All the calls to this __builtin are in gcc-specific headers, and if those are wrong, then the error is in(ternal to) the compiler. (Of course if the lane index is non-constant that is a programmer error.) We don't wish to support the programmer writing direct calls to the builtin him/herself! --Alan > > OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions > and I was able to bootstrap without a modified libcpp. > > Thanks, > Andrew Pinski > > ChangeLog: > * config/aarch64/aarch64-builtins.c (aarch64_init_simd_builtins): > Change the first argument type to size_type_node and add another > size_type_node. > (aarch64_simd_expand_builtin): Handle the new argument to > AARCH64_SIMD_BUILTIN_LANE_CHECK and don't ICE but rather > print sorry out when the first two arguments are not > integer constants. > * config/aarch64/arm_neon.h (__AARCH64_LANE_CHECK): > Pass the sizeof's directly to __builtin_aarch64_im_lane_boundsi. > > testsuite/ChangeLog: > * c-c++-common/torture/aarch64-vect-lane-1.c: New testcase. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH/AARCH64] Fix 64893: ICE with vget_lane_u32 with C++ front-end at -O0 2015-02-03 11:57 ` Alan Lawrence @ 2015-02-03 13:01 ` pinskia 0 siblings, 0 replies; 11+ messages in thread From: pinskia @ 2015-02-03 13:01 UTC (permalink / raw) To: Alan Lawrence; +Cc: gcc-patches > On Feb 3, 2015, at 3:57 AM, Alan Lawrence <alan.lawrence@arm.com> wrote: > > > Andrew Pinski wrote: >> While trying to build the GCC 5 with GCC 5, I ran into an ICE when >> building libcpp at -O0. The problem is the C++ front-end was not >> folding sizeof(a)/sizeof(a[0]) when passed to a function at -O0. The >> C++ front-end keeps around sizeof until the gimplifier and there is no >> way to fold the expressions that involve them. So to work around the >> issue we need to change __builtin_aarch64_im_lane_boundsi to accept an >> extra argument and change the first two arguments to size_t type so we >> don't get an extra cast there and do the division inside the compiler >> itself. >> Also we don't want to cause an ICE on any source code so I changed the >> assert to be a sorry if either of the two arguments are not integer >> constants. > > TBH I think it _is_ appropriate to ICE rather than sorry, or even error, if the size of the vector or vector elements are non-constant or zero. All the calls to this __builtin are in gcc-specific headers, and if those are wrong, then the error is in(ternal to) the compiler. (Of course if the lane index is non-constant that is a programmer error.) We don't wish to support the programmer writing direct calls to the builtin him/herself! Even if we don't support direct calls to it, it still should not ice. This is gcc policy for a long time now. Iceing on any input is bad form and not helpful to users. We can say in the error message that calling it direct is not supported and they should not do it. Thanks, Andrew > > --Alan > > >> OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions >> and I was able to bootstrap without a modified libcpp. >> Thanks, >> Andrew Pinski >> ChangeLog: >> * config/aarch64/aarch64-builtins.c (aarch64_init_simd_builtins): >> Change the first argument type to size_type_node and add another >> size_type_node. >> (aarch64_simd_expand_builtin): Handle the new argument to >> AARCH64_SIMD_BUILTIN_LANE_CHECK and don't ICE but rather >> print sorry out when the first two arguments are not >> integer constants. >> * config/aarch64/arm_neon.h (__AARCH64_LANE_CHECK): >> Pass the sizeof's directly to __builtin_aarch64_im_lane_boundsi. >> testsuite/ChangeLog: >> * c-c++-common/torture/aarch64-vect-lane-1.c: New testcase. > > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-03-06 10:51 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-02-02 22:51 [PATCH/AARCH64] Fix 64893: ICE with vget_lane_u32 with C++ front-end at -O0 Andrew Pinski 2015-02-03 7:37 ` Jakub Jelinek 2015-02-07 1:02 ` Andrew Pinski 2015-02-08 2:24 ` Andrew Pinski 2015-02-11 10:11 ` Marcus Shawcroft 2015-02-12 15:37 ` Christophe Lyon 2015-03-06 9:45 ` James Greenhalgh 2015-03-06 10:03 ` pinskia 2015-03-06 10:51 ` James Greenhalgh 2015-02-03 11:57 ` Alan Lawrence 2015-02-03 13:01 ` pinskia
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).