Re: [PATCH/AARCH64] Fix 64893: ICE with vget_lane_u32 with C++ front-end at -O0

[pinskia at gmail dot com]

> 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.
> 
>