| Message ID | CA+=Sn1nmq5gaB7G5B5x7QTEt4v0wVCKki15nC6oXJodzuDR-TA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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.
> 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. > >
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
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); +} +
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. 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.