On 8 February 2015 at 03:24, Andrew Pinski <pins...@gmail.com> wrote:
> On Fri, Feb 6, 2015 at 5:02 PM, Andrew Pinski <pins...@gmail.com> wrote:
>> On Mon, Feb 2, 2015 at 11:37 PM, Jakub Jelinek <ja...@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
