diff mbox

[ARM] Fix unrecognizable vector comparisons

Message ID CACgzC7Ah7+rBUXCej2dHD3y-oVtpgrRKMA9y+e=h7jEk=ZmLOQ@mail.gmail.com
State New
Headers show

Commit Message

Zhenqiang Chen June 26, 2013, 8:01 a.m. UTC 
On 18 June 2013 17:41, Ramana Radhakrishnan <ramrad01@arm.com> wrote:
> On 06/18/13 09:50, Zhenqiang Chen wrote:
>>
>> Hi,
>>
>> During expand, function vcond<mode><mode> inverses some CMP, e.g.
>>
>>     a LE b -> b GE a
>>
>> But if "b" is "CONST0_RTX", "b GE a" will be an illegal insn.
>>
>> (insn 933 932 934 113 (set (reg:V4SI 1027)
>>          (unspec:V4SI [
>>                  (const_vector:V4SI [
>>                          (const_int 0 [0])
>>                          (const_int 0 [0])
>>                          (const_int 0 [0])
>>                          (const_int 0 [0])
>>                      ])
>>                  (reg:V4SI 1023 [ vect_var_.49 ])
>>                  (const_int 1 [0x1])
>>              ] UNSPEC_VCGE)) PUGHSlab/Mapping.c:567 -1
>>       (nil))
>>
>> Refer https://bugs.launchpad.net/linaro-toolchain-binaries/+bug/1189445
>> for more. And the bug also happens for FSF trunk.
>>
>> The similar issue
>> (https://bugs.launchpad.net/linaro-toolchain-binaries/+bug/1163942)
>> had fixed on AARCH64:
>> http://gcc.gnu.org/ml/gcc-patches/2013-04/msg00581.html
>>
>> The patch is similar to the fix for aarch64.
>>
>> Bootstrap and no make check regression on Panda Board.
>>
>> Is it OK for trunk and 4.8?
>
>
> No, not without an appropriate set of testcases that exercise these cases.

Thanks for the comments. Patch is updated with a test case.

Comments

Ramana Radhakrishnan July 8, 2013, 12:57 p.m. UTC | #1 
Not yet. Why is this not a problem in the LT / UNLT case ?

The testcase is not correctly written.


@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-options "-O3 -mfpu=neon -mcpu=cortex-a9 -mthumb
-mfloat-abi=hard -S" } */

dg-add-options arm_neon ?
dg-require-effective-target arm_neon ?




regards
Ramana

On Wed, Jun 26, 2013 at 9:01 AM, Zhenqiang Chen
<zhenqiang.chen@linaro.org> wrote:
> On 18 June 2013 17:41, Ramana Radhakrishnan <ramrad01@arm.com> wrote:
>> On 06/18/13 09:50, Zhenqiang Chen wrote:
>>>
>>> Hi,
>>>
>>> During expand, function vcond<mode><mode> inverses some CMP, e.g.
>>>
>>>     a LE b -> b GE a
>>>
>>> But if "b" is "CONST0_RTX", "b GE a" will be an illegal insn.
>>>
>>> (insn 933 932 934 113 (set (reg:V4SI 1027)
>>>          (unspec:V4SI [
>>>                  (const_vector:V4SI [
>>>                          (const_int 0 [0])
>>>                          (const_int 0 [0])
>>>                          (const_int 0 [0])
>>>                          (const_int 0 [0])
>>>                      ])
>>>                  (reg:V4SI 1023 [ vect_var_.49 ])
>>>                  (const_int 1 [0x1])
>>>              ] UNSPEC_VCGE)) PUGHSlab/Mapping.c:567 -1
>>>       (nil))
>>>
>>> Refer https://bugs.launchpad.net/linaro-toolchain-binaries/+bug/1189445
>>> for more. And the bug also happens for FSF trunk.
>>>
>>> The similar issue
>>> (https://bugs.launchpad.net/linaro-toolchain-binaries/+bug/1163942)
>>> had fixed on AARCH64:
>>> http://gcc.gnu.org/ml/gcc-patches/2013-04/msg00581.html
>>>
>>> The patch is similar to the fix for aarch64.
>>>
>>> Bootstrap and no make check regression on Panda Board.
>>>
>>> Is it OK for trunk and 4.8?
>>
>>
>> No, not without an appropriate set of testcases that exercise these cases.
>
> Thanks for the comments. Patch is updated with a test case.
>
> diff --git a/gcc/config/arm/neon.md b/gcc/config/arm/neon.md
> index 2761adb..6d9f604 100644
> --- a/gcc/config/arm/neon.md
> +++ b/gcc/config/arm/neon.md
> @@ -1710,6 +1710,9 @@
>      case LE:
>      case UNLE:
>        inverse = 1;
> +      /* Can not inverse "a LE 0" to "0 GE a".  */
> +      if (operands[5] == CONST0_RTX (<MODE>mode))
> +       inverse = 0;
>        /* Fall through.  */
>      case GT:
>      case UNGT:
> diff --git a/gcc/testsuite/gcc.target/arm/lp1189445.c
> b/gcc/testsuite/gcc.target/arm/lp1189445.c
> new file mode 100644
> index 0000000..8ce4b97
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/lp1189445.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O3 -mfpu=neon -mcpu=cortex-a9 -mthumb
> -mfloat-abi=hard -S" } */
> +
> +int id;
> +int
> +test (const long int *data)
> +{
> +  int i, retval;
> +  retval = id;
> +  for (i = 0; i < id; i++)
> +    {
> +      retval &= (data[i] <= 0);
> +    }
> +
> +  return (retval);
> +}
Zhenqiang Chen July 8, 2013, 3:32 p.m. UTC | #2 
On 8 July 2013 20:57, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote:
> Not yet. Why is this not a problem in the LT / UNLT case ?

From the context, after the first switch, only GE/LE/EQ might have
operands[5] which is not REG (CONST0_RTX). For others including
LT/UNLT, operands[5] should be REG.

if (!REG_P (operands[5]))
  operands[5] = force_reg (<MODE>mode, operands[5]);

For GE/LE/EQ, we only reverse LE. So only LE has issue.

> The testcase is not correctly written.
>
>
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O3 -mfpu=neon -mcpu=cortex-a9 -mthumb
> -mfloat-abi=hard -S" } */
>
> dg-add-options arm_neon ?
> dg-require-effective-target arm_neon ?

I will update it.

Thanks!
-Zhenqiang

> On Wed, Jun 26, 2013 at 9:01 AM, Zhenqiang Chen
> <zhenqiang.chen@linaro.org> wrote:
>> On 18 June 2013 17:41, Ramana Radhakrishnan <ramrad01@arm.com> wrote:
>>> On 06/18/13 09:50, Zhenqiang Chen wrote:
>>>>
>>>> Hi,
>>>>
>>>> During expand, function vcond<mode><mode> inverses some CMP, e.g.
>>>>
>>>>     a LE b -> b GE a
>>>>
>>>> But if "b" is "CONST0_RTX", "b GE a" will be an illegal insn.
>>>>
>>>> (insn 933 932 934 113 (set (reg:V4SI 1027)
>>>>          (unspec:V4SI [
>>>>                  (const_vector:V4SI [
>>>>                          (const_int 0 [0])
>>>>                          (const_int 0 [0])
>>>>                          (const_int 0 [0])
>>>>                          (const_int 0 [0])
>>>>                      ])
>>>>                  (reg:V4SI 1023 [ vect_var_.49 ])
>>>>                  (const_int 1 [0x1])
>>>>              ] UNSPEC_VCGE)) PUGHSlab/Mapping.c:567 -1
>>>>       (nil))
>>>>
>>>> Refer https://bugs.launchpad.net/linaro-toolchain-binaries/+bug/1189445
>>>> for more. And the bug also happens for FSF trunk.
>>>>
>>>> The similar issue
>>>> (https://bugs.launchpad.net/linaro-toolchain-binaries/+bug/1163942)
>>>> had fixed on AARCH64:
>>>> http://gcc.gnu.org/ml/gcc-patches/2013-04/msg00581.html
>>>>
>>>> The patch is similar to the fix for aarch64.
>>>>
>>>> Bootstrap and no make check regression on Panda Board.
>>>>
>>>> Is it OK for trunk and 4.8?
>>>
>>>
>>> No, not without an appropriate set of testcases that exercise these cases.
>>
>> Thanks for the comments. Patch is updated with a test case.
>>
>> diff --git a/gcc/config/arm/neon.md b/gcc/config/arm/neon.md
>> index 2761adb..6d9f604 100644
>> --- a/gcc/config/arm/neon.md
>> +++ b/gcc/config/arm/neon.md
>> @@ -1710,6 +1710,9 @@
>>      case LE:
>>      case UNLE:
>>        inverse = 1;
>> +      /* Can not inverse "a LE 0" to "0 GE a".  */
>> +      if (operands[5] == CONST0_RTX (<MODE>mode))
>> +       inverse = 0;
>>        /* Fall through.  */
>>      case GT:
>>      case UNGT:
>> diff --git a/gcc/testsuite/gcc.target/arm/lp1189445.c
>> b/gcc/testsuite/gcc.target/arm/lp1189445.c
>> new file mode 100644
>> index 0000000..8ce4b97
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/arm/lp1189445.c
>> @@ -0,0 +1,16 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O3 -mfpu=neon -mcpu=cortex-a9 -mthumb
>> -mfloat-abi=hard -S" } */
>> +
>> +int id;
>> +int
>> +test (const long int *data)
>> +{
>> +  int i, retval;
>> +  retval = id;
>> +  for (i = 0; i < id; i++)
>> +    {
>> +      retval &= (data[i] <= 0);
>> +    }
>> +
>> +  return (retval);
>> +}
James Greenhalgh July 8, 2013, 4:37 p.m. UTC | #3 
On Mon, Jul 08, 2013 at 04:32:13PM +0100, Zhenqiang Chen wrote:
> On 8 July 2013 20:57, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote:
> > Not yet. Why is this not a problem in the LT / UNLT case ?
> 
> From the context, after the first switch, only GE/LE/EQ might have
> operands[5] which is not REG (CONST0_RTX). For others including
> LT/UNLT, operands[5] should be REG.

This is true, but looks like an omission. My copy of the ARMARM
has immediate #0 instruction forms for CMLT, CMLE, CMGE, CMGT
and CMEQ. Perhaps it is beyond the scope of your bugfix
(though it was in your original patch?), but this should be
fixed in future so as not to force 0 to registers.

> 
> if (!REG_P (operands[5]))
>   operands[5] = force_reg (<MODE>mode, operands[5]);
> 
> For GE/LE/EQ, we only reverse LE. So only LE has issue.
> 

For now, but as above - as soon as this code is fixed to generate
immediate #0 forms, it will be fragile again.

> >> diff --git a/gcc/config/arm/neon.md b/gcc/config/arm/neon.md
> >> index 2761adb..6d9f604 100644
> >> --- a/gcc/config/arm/neon.md
> >> +++ b/gcc/config/arm/neon.md
> >> @@ -1710,6 +1710,9 @@
> >>      case LE:
> >>      case UNLE:
> >>        inverse = 1;
> >> +      /* Can not inverse "a LE 0" to "0 GE a".  */
> >> +      if (operands[5] == CONST0_RTX (<MODE>mode))
> >> +       inverse = 0;
> >>        /* Fall through.  */
> >>      case GT:
> >>      case UNGT:

Is this really what you mean? Surely now you will have:

  inverse = 0
  base_comparison = gen_neon_vcgt

Thus in the next switch you will call:
  emit_insn (gen_neon_vcgt (mask, operands[4], operands[5], magic_rtx));

Which looks wrong. Would you not also have to set swap_bsl_operands
to get back to the correct semantics?

> >> diff --git a/gcc/testsuite/gcc.target/arm/lp1189445.c
> >> b/gcc/testsuite/gcc.target/arm/lp1189445.c
> >> new file mode 100644
> >> index 0000000..8ce4b97
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.target/arm/lp1189445.c
> >> @@ -0,0 +1,16 @@
> >> +/* { dg-do compile } */
> >> +/* { dg-options "-O3 -mfpu=neon -mcpu=cortex-a9 -mthumb
> >> -mfloat-abi=hard -S" } */
> >> +
> >> +int id;
> >> +int
> >> +test (const long int *data)
> >> +{
> >> +  int i, retval;
> >> +  retval = id;
> >> +  for (i = 0; i < id; i++)
> >> +    {
> >> +      retval &= (data[i] <= 0);
> >> +    }
> >> +
> >> +  return (retval);
> >> +}
> 

This testcase is not much use. It may well compile, but won't catch
the wrong instruction generation issue I pointed out above.

I much prefer your original patch, with a more rigorous testcase.

Thanks,
James
Janis Johnson July 8, 2013, 6:44 p.m. UTC | #4 
On 07/08/2013 08:32 AM, Zhenqiang Chen wrote:
> On 8 July 2013 20:57, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote:
>> Not yet. Why is this not a problem in the LT / UNLT case ?
> 
>>From the context, after the first switch, only GE/LE/EQ might have
> operands[5] which is not REG (CONST0_RTX). For others including
> LT/UNLT, operands[5] should be REG.
> 
> if (!REG_P (operands[5]))
>   operands[5] = force_reg (<MODE>mode, operands[5]);
> 
> For GE/LE/EQ, we only reverse LE. So only LE has issue.
> 
>> The testcase is not correctly written.
>>
>>
>> @@ -0,0 +1,16 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O3 -mfpu=neon -mcpu=cortex-a9 -mthumb
>> -mfloat-abi=hard -S" } */
>>
>> dg-add-options arm_neon ?
>> dg-require-effective-target arm_neon ?
> 
> I will update it.

Please skip the test for multilibs whose flags include -mfpu or -mcpu options,
which would conflict with or override the options in the test.

Janis

> Thanks!
> -Zhenqiang
> 
>> On Wed, Jun 26, 2013 at 9:01 AM, Zhenqiang Chen
>> <zhenqiang.chen@linaro.org> wrote:
>>> On 18 June 2013 17:41, Ramana Radhakrishnan <ramrad01@arm.com> wrote:
>>>> On 06/18/13 09:50, Zhenqiang Chen wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> During expand, function vcond<mode><mode> inverses some CMP, e.g.
>>>>>
>>>>>     a LE b -> b GE a
>>>>>
>>>>> But if "b" is "CONST0_RTX", "b GE a" will be an illegal insn.
>>>>>
>>>>> (insn 933 932 934 113 (set (reg:V4SI 1027)
>>>>>          (unspec:V4SI [
>>>>>                  (const_vector:V4SI [
>>>>>                          (const_int 0 [0])
>>>>>                          (const_int 0 [0])
>>>>>                          (const_int 0 [0])
>>>>>                          (const_int 0 [0])
>>>>>                      ])
>>>>>                  (reg:V4SI 1023 [ vect_var_.49 ])
>>>>>                  (const_int 1 [0x1])
>>>>>              ] UNSPEC_VCGE)) PUGHSlab/Mapping.c:567 -1
>>>>>       (nil))
>>>>>
>>>>> Refer https://bugs.launchpad.net/linaro-toolchain-binaries/+bug/1189445
>>>>> for more. And the bug also happens for FSF trunk.
>>>>>
>>>>> The similar issue
>>>>> (https://bugs.launchpad.net/linaro-toolchain-binaries/+bug/1163942)
>>>>> had fixed on AARCH64:
>>>>> http://gcc.gnu.org/ml/gcc-patches/2013-04/msg00581.html
>>>>>
>>>>> The patch is similar to the fix for aarch64.
>>>>>
>>>>> Bootstrap and no make check regression on Panda Board.
>>>>>
>>>>> Is it OK for trunk and 4.8?
>>>>
>>>>
>>>> No, not without an appropriate set of testcases that exercise these cases.
>>>
>>> Thanks for the comments. Patch is updated with a test case.
>>>
>>> diff --git a/gcc/config/arm/neon.md b/gcc/config/arm/neon.md
>>> index 2761adb..6d9f604 100644
>>> --- a/gcc/config/arm/neon.md
>>> +++ b/gcc/config/arm/neon.md
>>> @@ -1710,6 +1710,9 @@
>>>      case LE:
>>>      case UNLE:
>>>        inverse = 1;
>>> +      /* Can not inverse "a LE 0" to "0 GE a".  */
>>> +      if (operands[5] == CONST0_RTX (<MODE>mode))
>>> +       inverse = 0;
>>>        /* Fall through.  */
>>>      case GT:
>>>      case UNGT:
>>> diff --git a/gcc/testsuite/gcc.target/arm/lp1189445.c
>>> b/gcc/testsuite/gcc.target/arm/lp1189445.c
>>> new file mode 100644
>>> index 0000000..8ce4b97
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.target/arm/lp1189445.c
>>> @@ -0,0 +1,16 @@
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-O3 -mfpu=neon -mcpu=cortex-a9 -mthumb
>>> -mfloat-abi=hard -S" } */
>>> +
>>> +int id;
>>> +int
>>> +test (const long int *data)
>>> +{
>>> +  int i, retval;
>>> +  retval = id;
>>> +  for (i = 0; i < id; i++)
>>> +    {
>>> +      retval &= (data[i] <= 0);
>>> +    }
>>> +
>>> +  return (retval);
>>> +}
> 
>
Jakub Jelinek July 8, 2013, 6:49 p.m. UTC | #5 
On Mon, Jul 08, 2013 at 11:44:04AM -0700, Janis Johnson wrote:
> >> @@ -0,0 +1,16 @@
> >> +/* { dg-do compile } */
> >> +/* { dg-options "-O3 -mfpu=neon -mcpu=cortex-a9 -mthumb
> >> -mfloat-abi=hard -S" } */
> >>
> >> dg-add-options arm_neon ?
> >> dg-require-effective-target arm_neon ?
> > 
> > I will update it.
> 
> Please skip the test for multilibs whose flags include -mfpu or -mcpu options,
> which would conflict with or override the options in the test.

Also the -S in dg-options looks wrong.  That should be derived from dg-do.

	Jakub
Zhenqiang Chen Aug. 1, 2013, 2:04 a.m. UTC | #6 
Thank you all for the comments. The patch is updated as:
1) Revert it to the original one.
2) For the testcase, replace the dg-options with
/* { dg-do compile } */
/* { dg-require-effective-target arm_neon } */
/* { dg-add-options arm_neon } */
/* { dg-options "-O3" } */

Bootstrap on Chromebook and Pandaboard.
No make check regression on Pandaboard.

Thanks!
-Zhenqiang

On 9 July 2013 02:49, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Jul 08, 2013 at 11:44:04AM -0700, Janis Johnson wrote:
>> >> @@ -0,0 +1,16 @@
>> >> +/* { dg-do compile } */
>> >> +/* { dg-options "-O3 -mfpu=neon -mcpu=cortex-a9 -mthumb
>> >> -mfloat-abi=hard -S" } */
>> >>
>> >> dg-add-options arm_neon ?
>> >> dg-require-effective-target arm_neon ?
>> >
>> > I will update it.
>>
>> Please skip the test for multilibs whose flags include -mfpu or -mcpu options,
>> which would conflict with or override the options in the test.
>
> Also the -S in dg-options looks wrong.  That should be derived from dg-do.
>
>         Jakub
Zhenqiang Chen Aug. 8, 2013, 9 a.m. UTC | #7 
Ping? Is it OK for 4.8 and trunk?

Thanks!
-Zhenqiang

On 1 August 2013 10:04, Zhenqiang Chen <zhenqiang.chen@linaro.org> wrote:
> Thank you all for the comments. The patch is updated as:
> 1) Revert it to the original one.
> 2) For the testcase, replace the dg-options with
> /* { dg-do compile } */
> /* { dg-require-effective-target arm_neon } */
> /* { dg-add-options arm_neon } */
> /* { dg-options "-O3" } */
>
> Bootstrap on Chromebook and Pandaboard.
> No make check regression on Pandaboard.
>
> Thanks!
> -Zhenqiang
>
> On 9 July 2013 02:49, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Mon, Jul 08, 2013 at 11:44:04AM -0700, Janis Johnson wrote:
>>> >> @@ -0,0 +1,16 @@
>>> >> +/* { dg-do compile } */
>>> >> +/* { dg-options "-O3 -mfpu=neon -mcpu=cortex-a9 -mthumb
>>> >> -mfloat-abi=hard -S" } */
>>> >>
>>> >> dg-add-options arm_neon ?
>>> >> dg-require-effective-target arm_neon ?
>>> >
>>> > I will update it.
>>>
>>> Please skip the test for multilibs whose flags include -mfpu or -mcpu options,
>>> which would conflict with or override the options in the test.
>>
>> Also the -S in dg-options looks wrong.  That should be derived from dg-do.
>>
>>         Jakub
Ramana Radhakrishnan Aug. 8, 2013, 9:02 a.m. UTC | #8 
On 08/01/13 03:04, Zhenqiang Chen wrote:
> Thank you all for the comments. The patch is updated as:
> 1) Revert it to the original one.
> 2) For the testcase, replace the dg-options with
> /* { dg-do compile } */
> /* { dg-require-effective-target arm_neon } */
> /* { dg-add-options arm_neon } */
> /* { dg-options "-O3" } */
>
> Bootstrap on Chromebook and Pandaboard.
> No make check regression on Pandaboard.
>
> Thanks!
> -Zhenqiang

Ok for trunk and 4.8 branch.

Sorry about the delay.

Thanks,
Ramana
diff mbox

Patch

diff --git a/gcc/config/arm/neon.md b/gcc/config/arm/neon.md
index 2761adb..6d9f604 100644
--- a/gcc/config/arm/neon.md
+++ b/gcc/config/arm/neon.md
@@ -1710,6 +1710,9 @@ 
     case LE:
     case UNLE:
       inverse = 1;
+      /* Can not inverse "a LE 0" to "0 GE a".  */
+      if (operands[5] == CONST0_RTX (<MODE>mode))
+       inverse = 0;
       /* Fall through.  */
     case GT:
     case UNGT:
diff --git a/gcc/testsuite/gcc.target/arm/lp1189445.c
b/gcc/testsuite/gcc.target/arm/lp1189445.c
new file mode 100644
index 0000000..8ce4b97
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/lp1189445.c
@@ -0,0 +1,16 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O3 -mfpu=neon -mcpu=cortex-a9 -mthumb
-mfloat-abi=hard -S" } */
+
+int id;
+int
+test (const long int *data)
+{
+  int i, retval;
+  retval = id;
+  for (i = 0; i < id; i++)
+    {
+      retval &= (data[i] <= 0);
+    }
+
+  return (retval);
+}