diff mbox

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

Message ID CA+=Sn1k0P3t1r65R_8E1Z3_pr3oosgZWpkEEFC=nmF9fni6PrA@mail.gmail.com
State New
Headers show

Commit Message

Andrew Pinski Feb. 8, 2015, 2:24 a.m. UTC 
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
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.

Comments

Marcus Shawcroft Feb. 11, 2015, 10:10 a.m. UTC | #1 
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
Christophe Lyon Feb. 12, 2015, 3:37 p.m. UTC | #2 
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
James Greenhalgh March 6, 2015, 9:45 a.m. UTC | #3 
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
Andrew Pinski March 6, 2015, 10:03 a.m. UTC | #4 
> 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
>
diff mbox

Patch

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 "" } */
+}
+