diff mbox

[simplify-rtx] PR 65235: Calculate element size correctly when simplifying (vec_select (vec_concat (const_int) (...)) [...])

Message ID 5501947D.5020208@arm.com
State New
Headers show

Commit Message

Kyrylo Tkachov March 12, 2015, 1:28 p.m. UTC
>> The patch fixes that by calculating the size of the first element by
 >> taking the size of the outer mode and subtracting the size of the second
 >> element.
 >>
 >> I've added an assert to make sure that the second element is not also a
 >> const_int, as a vec_concat of const_ints doesn't make sense as far 
as I can
 >> see.
 >
 > I'm not sure about the assert, can't we just punt in this case?

Ok, here's a patch returning 0 in that case.
The assert had never triggered in my testing anyway, but I agree we
want to just cancel the simplification rather than ICE.

 >
 >> Bootstrapped and tested on aarch64-none-linux-gnu,
 >> arm-none-linux-gnueabihf, x86_64-linux-gnu.
 >> This bug appears on trunk, 4.9 and 4.8, so it's not a regression on the
 >> release branches but it is a wrong-code bug.
 >
 > I think that the fix would be acceptable for GCC 5 without the assert.
 >

Thanks for reviewing.
Richard, do you think this can go in for GCC 5 now?
What about 4.9 and 4.8? The bug appears there as well.

Thanks,
Kyrill


2015-03-12  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization 65235
     * simplify-rtx.c (simplify_binary_operation_1, VEC_SELECT case):
     When first element of vec_concat is const_int, calculate its size
     using second element.

2015-03-12  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization 65235
     * gcc.target/aarch64/pr65235_1.c: New test.

Comments

Richard Biener March 12, 2015, 1:33 p.m. UTC | #1
On Thu, Mar 12, 2015 at 2:28 PM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
>>> The patch fixes that by calculating the size of the first element by
>>> taking the size of the outer mode and subtracting the size of the second
>>> element.
>>>
>>> I've added an assert to make sure that the second element is not also a
>>> const_int, as a vec_concat of const_ints doesn't make sense as far as I
>>> can
>>> see.
>>
>> I'm not sure about the assert, can't we just punt in this case?
>
> Ok, here's a patch returning 0 in that case.
> The assert had never triggered in my testing anyway, but I agree we
> want to just cancel the simplification rather than ICE.
>
>>
>>> Bootstrapped and tested on aarch64-none-linux-gnu,
>>> arm-none-linux-gnueabihf, x86_64-linux-gnu.
>>> This bug appears on trunk, 4.9 and 4.8, so it's not a regression on the
>>> release branches but it is a wrong-code bug.
>>
>> I think that the fix would be acceptable for GCC 5 without the assert.
>>
>
> Thanks for reviewing.
> Richard, do you think this can go in for GCC 5 now?
> What about 4.9 and 4.8? The bug appears there as well.

Sure - it's a wrong-code fix.  Ok for trunk and branches (after a while).

Thanks,
Richard.

> Thanks,
> Kyrill
>
>
> 2015-03-12  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>     PR rtl-optimization 65235
>     * simplify-rtx.c (simplify_binary_operation_1, VEC_SELECT case):
>     When first element of vec_concat is const_int, calculate its size
>     using second element.
>
> 2015-03-12  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>
>     PR rtl-optimization 65235
>     * gcc.target/aarch64/pr65235_1.c: New test.
diff mbox

Patch

commit 9946603f73e89f50d6610a943f770627ed533dbc
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Thu Feb 26 16:40:52 2015 +0000

    [simplify-rtx] Calculate vector size correctly when simplifying (vec_select (vec_concat (const_int) (...)) [...])

diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
index a003b41..5d17498 100644
--- a/gcc/simplify-rtx.c
+++ b/gcc/simplify-rtx.c
@@ -3555,7 +3555,21 @@  simplify_binary_operation_1 (enum rtx_code code, machine_mode mode,
 	  while (GET_MODE (vec) != mode
 		 && GET_CODE (vec) == VEC_CONCAT)
 	    {
-	      HOST_WIDE_INT vec_size = GET_MODE_SIZE (GET_MODE (XEXP (vec, 0)));
+	      HOST_WIDE_INT vec_size;
+
+	      if (CONST_INT_P (XEXP (vec, 0)))
+	        {
+	          /* vec_concat of two const_ints doesn't make sense with
+	             respect to modes.  */
+	          if (CONST_INT_P (XEXP (vec, 1)))
+	            return 0;
+
+	          vec_size = GET_MODE_SIZE (GET_MODE (trueop0))
+	                     - GET_MODE_SIZE (GET_MODE (XEXP (vec, 1)));
+	        }
+	      else
+	        vec_size = GET_MODE_SIZE (GET_MODE (XEXP (vec, 0)));
+
 	      if (offset < vec_size)
 		vec = XEXP (vec, 0);
 	      else
diff --git a/gcc/testsuite/gcc.target/aarch64/pr65235_1.c b/gcc/testsuite/gcc.target/aarch64/pr65235_1.c
new file mode 100644
index 0000000..ca12cd5
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr65235_1.c
@@ -0,0 +1,30 @@ 
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+#include "arm_neon.h"
+
+int
+main (int argc, char** argv)
+{
+  int64x1_t val1;
+  int64x1_t val2;
+  int64x1_t val3;
+  uint64x1_t val13;
+  uint64x2_t val14;
+  uint64_t got;
+  uint64_t exp;
+  val1 = vcreate_s64(UINT64_C(0xffffffff80008000));
+  val2 = vcreate_s64(UINT64_C(0x0000f38d00000000));
+  val3 = vcreate_s64(UINT64_C(0xffff7fff0000809b));
+  /* Expect: "val13" = 8000000000001553.  */
+  val13 = vcreate_u64 (UINT64_C(0x8000000000001553));
+  /* Expect: "val14" = 0010 0000 0000 0002 0000 0000 0000 0000.  */
+  val14 = vcombine_u64(vcgt_s64(vqrshl_s64(val1, val2),
+				vshr_n_s64(val3, 18)),
+		       vshr_n_u64(val13, 11));
+  /* Should be 0000000000000000.  */
+  got = vgetq_lane_u64(val14, 0);
+  exp = 0;
+  if(exp != got)
+    __builtin_abort ();
+}