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

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

[Andrew Pinski]

[-- Attachment #1: Type: text/plain, Size: 1528 bytes --]

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.

[-- Attachment #2: fixPR64893.diff.txt --]
[-- Type: text/plain, Size: 3566 bytes --]

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.

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);
+}
+

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

[Jakub Jelinek]

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

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

[Alan Lawrence]

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.

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

[pinskia]

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

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

[Andrew Pinski]

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

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

[Andrew Pinski]

[-- Attachment #1: Type: text/plain, Size: 3308 bytes --]

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 #2: fixPR64893.diff.txt --]
[-- Type: text/plain, Size: 4194 bytes --]

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.

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

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

[Marcus Shawcroft]

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

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

[Christophe Lyon]

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

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

[James Greenhalgh]

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

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

[pinskia]

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

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

[James Greenhalgh]

[-- Attachment #1: Type: text/plain, Size: 2224 bytes --]

On Fri, Mar 06, 2015 at 10:03:46AM +0000, pinskia@gmail.com wrote:
> > 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 for the confirmation. In the end I settled on XFAILing (It would be
nice to catch the error early enough that one day this test PASSes) the
test rather than skipping it.

I committed the attached patch as obvious as r221233.

Thanks,
James

---

2015-03-06  James Greenhalgh  <james.greenhalgh@arm.com>

	* c-c++-common/torture/aarch64-vect-lane-2.c: XFAIL for LTO
	compiles using the linker plugin.



[-- Attachment #2: xfail-aarch64-vect-lane.patch --]
[-- Type: text/x-diff, Size: 937 bytes --]

Index: gcc/testsuite/ChangeLog
===================================================================
--- gcc/testsuite/ChangeLog	(revision 221232)
+++ gcc/testsuite/ChangeLog	(working copy)
@@ -1,3 +1,8 @@
+2015-03-06  James Greenhalgh  <james.greenhalgh@arm.com>
+
+	* c-c++-common/torture/aarch64-vect-lane-2.c: XFAIL for LTO
+	compiles using the linker plugin.
+
 2015-03-06  Eric Botcazou  <ebotcazou@adacore.com>
 
 	* g++.dg/other/dump-ada-spec-3.C: Remove include and adjust.
Index: gcc/testsuite/c-c++-common/torture/aarch64-vect-lane-2.c
===================================================================
--- gcc/testsuite/c-c++-common/torture/aarch64-vect-lane-2.c	(revision 221232)
+++ gcc/testsuite/c-c++-common/torture/aarch64-vect-lane-2.c	(working copy)
@@ -1,4 +1,5 @@
 // { dg-do compile { target "aarch64*-*-*" } }
+// { dg-xfail-if "" { *-*-* } { "-flto -fuse-linker-plugin" } { "" } }
 int
 search_line_fast (void)
 {