* [PATCH][simplify-rtx] PR 65235: Calculate element size correctly when simplifying (vec_select (vec_concat (const_int) (...)) [...])
@ 2015-03-04 9:37 Kyrill Tkachov
2015-03-04 10:41 ` Marc Glisse
2015-03-06 9:42 ` Eric Botcazou
0 siblings, 2 replies; 6+ messages in thread
From: Kyrill Tkachov @ 2015-03-04 9:37 UTC (permalink / raw)
To: GCC Patches; +Cc: Eric Botcazou
[-- Attachment #1: Type: text/plain, Size: 2088 bytes --]
Hi all,
This patch fixes PR rtl-optimization 65235.
As mentioned in bugzilla:
Combine tries to combine:
(insn 72 71 73 2 (set (reg:V2DI 117 [ D.18177 ])
(vec_concat:V2DI (reg:DI 176 [ D.18179 ])
(reg:DI 114 [ D.18168 ])))
(expr_list:REG_DEAD (reg:DI 176 [ D.18179 ])
(expr_list:REG_DEAD (reg:DI 114 [ D.18168 ])
and
(insn 104 102 105 2 (set (reg:DI 193 [ D.18168 ])
(vec_select:DI (reg:V2DI 117 [ D.18177 ])
(parallel [
(const_int 0 [0])
])))
(expr_list:REG_DEAD (reg:V2DI 117 [ D.18177 ])
(nil)))
but ends up generating:
(set (reg:DI 193 [ D.18168 ])
(reg:DI 114 [ D.18168 ]))
that is, it gets the wrong element of the vec_concat.
The problem is that in simplify-rtx it tries to get the size of the
first element,
compare the requested offset against it and pick the second element if
the offset is
greater than that size. If the first element is a const_int, the size is
0, so it will
always pick the second part.
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.
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.
Should this go in now? Or wait for stage 1?
Thanks,
Kyrill
2015-03-04 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-04 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
PR rtl-optimization 65235
* gcc.target/aarch64/pr65235_1.c: New test.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vec-select-concat.patch --]
[-- Type: text/x-patch; name=vec-select-concat.patch, Size: 2285 bytes --]
commit 816f7592617681c48e5303aaa9e3b0ee8a858457
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..3a6159f 100644
--- a/gcc/simplify-rtx.c
+++ b/gcc/simplify-rtx.c
@@ -3555,7 +3555,19 @@ 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. */
+ gcc_assert (!CONST_INT_P (XEXP (vec, 1)));
+ 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 ();
+}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][simplify-rtx] PR 65235: Calculate element size correctly when simplifying (vec_select (vec_concat (const_int) (...)) [...])
2015-03-04 9:37 [PATCH][simplify-rtx] PR 65235: Calculate element size correctly when simplifying (vec_select (vec_concat (const_int) (...)) [...]) Kyrill Tkachov
@ 2015-03-04 10:41 ` Marc Glisse
2015-03-04 11:05 ` Kyrill Tkachov
2015-03-06 9:42 ` Eric Botcazou
1 sibling, 1 reply; 6+ messages in thread
From: Marc Glisse @ 2015-03-04 10:41 UTC (permalink / raw)
To: Kyrill Tkachov; +Cc: GCC Patches, Eric Botcazou
On Wed, 4 Mar 2015, Kyrill Tkachov wrote:
> Hi all,
>
> This patch fixes PR rtl-optimization 65235.
> As mentioned in bugzilla:
> Combine tries to combine:
> (insn 72 71 73 2 (set (reg:V2DI 117 [ D.18177 ])
> (vec_concat:V2DI (reg:DI 176 [ D.18179 ])
> (reg:DI 114 [ D.18168 ])))
> (expr_list:REG_DEAD (reg:DI 176 [ D.18179 ])
> (expr_list:REG_DEAD (reg:DI 114 [ D.18168 ])
>
> and
>
> (insn 104 102 105 2 (set (reg:DI 193 [ D.18168 ])
> (vec_select:DI (reg:V2DI 117 [ D.18177 ])
> (parallel [
> (const_int 0 [0])
> ])))
> (expr_list:REG_DEAD (reg:V2DI 117 [ D.18177 ])
> (nil)))
>
> but ends up generating:
> (set (reg:DI 193 [ D.18168 ])
> (reg:DI 114 [ D.18168 ]))
>
>
> that is, it gets the wrong element of the vec_concat.
>
> The problem is that in simplify-rtx it tries to get the size of the
> first element, compare the requested offset against it and pick the
> second element if the offset is greater than that size. If the first
> element is a const_int, the size is 0, so it will always pick the second
> part.
I am a bit surprised that the first element is a const_int and not reg
176, maybe that's why it doesn't reproduce on other platforms? Not that it
really matters.
> 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 wonder if we should admit that vector sizes are always a power of 2 and
use half the size of the outer mode?
> 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.
--
Marc Glisse
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][simplify-rtx] PR 65235: Calculate element size correctly when simplifying (vec_select (vec_concat (const_int) (...)) [...])
2015-03-04 10:41 ` Marc Glisse
@ 2015-03-04 11:05 ` Kyrill Tkachov
0 siblings, 0 replies; 6+ messages in thread
From: Kyrill Tkachov @ 2015-03-04 11:05 UTC (permalink / raw)
To: gcc-patches; +Cc: Eric Botcazou
On 04/03/15 10:41, Marc Glisse wrote:
> On Wed, 4 Mar 2015, Kyrill Tkachov wrote:
>
>> Hi all,
>>
>> This patch fixes PR rtl-optimization 65235.
>> As mentioned in bugzilla:
>> Combine tries to combine:
>> (insn 72 71 73 2 (set (reg:V2DI 117 [ D.18177 ])
>> (vec_concat:V2DI (reg:DI 176 [ D.18179 ])
>> (reg:DI 114 [ D.18168 ])))
>> (expr_list:REG_DEAD (reg:DI 176 [ D.18179 ])
>> (expr_list:REG_DEAD (reg:DI 114 [ D.18168 ])
>>
>> and
>>
>> (insn 104 102 105 2 (set (reg:DI 193 [ D.18168 ])
>> (vec_select:DI (reg:V2DI 117 [ D.18177 ])
>> (parallel [
>> (const_int 0 [0])
>> ])))
>> (expr_list:REG_DEAD (reg:V2DI 117 [ D.18177 ])
>> (nil)))
>>
>> but ends up generating:
>> (set (reg:DI 193 [ D.18168 ])
>> (reg:DI 114 [ D.18168 ]))
>>
>>
>> that is, it gets the wrong element of the vec_concat.
>>
>> The problem is that in simplify-rtx it tries to get the size of the
>> first element, compare the requested offset against it and pick the
>> second element if the offset is greater than that size. If the first
>> element is a const_int, the size is 0, so it will always pick the second
>> part.
> I am a bit surprised that the first element is a const_int and not reg
> 176, maybe that's why it doesn't reproduce on other platforms? Not that it
> really matters.
Yeah, I guess combine tries various whacky combinations when it's trying to
munge things together.
>
>> 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 wonder if we should admit that vector sizes are always a power of 2 and
> use half the size of the outer mode?
Perhaps. I tried to be as defensive as possible since I'm not sure what
the restrictions
are on vec_select+vec_concat combinations.
Kyrill
>
>> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][simplify-rtx] PR 65235: Calculate element size correctly when simplifying (vec_select (vec_concat (const_int) (...)) [...])
2015-03-04 9:37 [PATCH][simplify-rtx] PR 65235: Calculate element size correctly when simplifying (vec_select (vec_concat (const_int) (...)) [...]) Kyrill Tkachov
2015-03-04 10:41 ` Marc Glisse
@ 2015-03-06 9:42 ` Eric Botcazou
2015-03-12 13:28 ` Kyrill Tkachov
1 sibling, 1 reply; 6+ messages in thread
From: Eric Botcazou @ 2015-03-06 9:42 UTC (permalink / raw)
To: Kyrill Tkachov; +Cc: gcc-patches
> 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?
> 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.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][simplify-rtx] PR 65235: Calculate element size correctly when simplifying (vec_select (vec_concat (const_int) (...)) [...])
2015-03-06 9:42 ` Eric Botcazou
@ 2015-03-12 13:28 ` Kyrill Tkachov
2015-03-12 13:33 ` Richard Biener
0 siblings, 1 reply; 6+ messages in thread
From: Kyrill Tkachov @ 2015-03-12 13:28 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches, 'Richard Biener'
[-- Attachment #1: Type: text/plain, Size: 1462 bytes --]
>> 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.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vec-select-concat.patch --]
[-- Type: text/x-patch; name=vec-select-concat.patch, Size: 2303 bytes --]
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 ();
+}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][simplify-rtx] PR 65235: Calculate element size correctly when simplifying (vec_select (vec_concat (const_int) (...)) [...])
2015-03-12 13:28 ` Kyrill Tkachov
@ 2015-03-12 13:33 ` Richard Biener
0 siblings, 0 replies; 6+ messages in thread
From: Richard Biener @ 2015-03-12 13:33 UTC (permalink / raw)
To: Kyrill Tkachov; +Cc: Eric Botcazou, gcc-patches
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-03-12 13:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-04 9:37 [PATCH][simplify-rtx] PR 65235: Calculate element size correctly when simplifying (vec_select (vec_concat (const_int) (...)) [...]) Kyrill Tkachov
2015-03-04 10:41 ` Marc Glisse
2015-03-04 11:05 ` Kyrill Tkachov
2015-03-06 9:42 ` Eric Botcazou
2015-03-12 13:28 ` Kyrill Tkachov
2015-03-12 13:33 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).