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

[Andrew Pinski <pinskia at gmail dot com>]

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: fixPR64893.diff.txt
Description: Text document