[PATCH] recog: Fix a constrain_operands corner case [PR97144]

[PATCH] recog: Fix a constrain_operands corner case [PR97144]

[Richard Sandiford]

aarch64's *add<mode>3_poly_1 has a pattern with the constraints:

  "=...,r,&r"
  "...,0,rk"
  "...,Uai,Uat"

i.e. the penultimate alternative requires operands 0 and 1 to match,
but the final alternative does not allow them to match.

The register allocators dealt with this correctly, and so used
different input and output registers for instructions with Uat
operands.  However, constrain_operands carried the penultimate
alternative's matching rule over to the final alternative,
so it would essentially ignore the earlyclobber.  This in turn
allowed postreload to convert a correct Uat pairing into an
incorrect one.

The fix is simple: recompute the matching information for each
alternative.

Tested on aarch64-linux-gnu, aarch64_be-elf and x86_64-linux-gnu.
OK to install?

Richard


gcc/
	PR rtl-optimization/97144
	* recog.c (constrain_operands): Initialize matching_operand
	for each alternative, rather than only doing it once.

gcc/testsuite/
	PR rtl-optimization/97144
	* gcc.target/aarch64/sve/pr97144.c: New test.
---
 gcc/recog.c                                   |  8 +++---
 .../gcc.target/aarch64/sve/pr97144.c          | 26 +++++++++++++++++++
 2 files changed, 30 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/sve/pr97144.c

diff --git a/gcc/recog.c b/gcc/recog.c
index e9aa1ba253d..29d6c491e15 100644
--- a/gcc/recog.c
+++ b/gcc/recog.c
@@ -3022,10 +3022,7 @@ constrain_operands (int strict, alternative_mask alternatives)
     return 1;
 
   for (c = 0; c < recog_data.n_operands; c++)
-    {
-      constraints[c] = recog_data.constraints[c];
-      matching_operands[c] = -1;
-    }
+    constraints[c] = recog_data.constraints[c];
 
   do
     {
@@ -3045,6 +3042,9 @@ constrain_operands (int strict, alternative_mask alternatives)
 	  continue;
 	}
 
+      for (opno = 0; opno < recog_data.n_operands; opno++)
+	matching_operands[opno] = -1;
+
       for (opno = 0; opno < recog_data.n_operands; opno++)
 	{
 	  rtx op = recog_data.operand[opno];
diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pr97144.c b/gcc/testsuite/gcc.target/aarch64/sve/pr97144.c
new file mode 100644
index 00000000000..75245e2350b
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/sve/pr97144.c
@@ -0,0 +1,26 @@
+/* { dg-options "-O2 -ftree-vectorize" } */
+
+int a, b = 5, c = 3;
+char d;
+char e[1];
+int f[] = {0, 0, 1};
+short g;
+char *h = e;
+void i(void) { b = a; }
+static void j(void) {
+  h = e;
+  if (f[2])
+  k:
+    for (;;) {
+      for (c = 0; c <= 4; c++) {
+        for (g = 0; g <= 4; g++)
+          f[g + 4] &= 2;
+      }
+      if (d)
+        goto k;
+    }
+}
+void l(void) {
+  j();
+  c = 0;
+}

Re: [PATCH] recog: Fix a constrain_operands corner case [PR97144]

[H.J. Lu]

On Thu, Dec 31, 2020 at 7:57 AM Richard Sandiford via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> aarch64's *add<mode>3_poly_1 has a pattern with the constraints:
>
>   "=...,r,&r"
>   "...,0,rk"
>   "...,Uai,Uat"
>
> i.e. the penultimate alternative requires operands 0 and 1 to match,
> but the final alternative does not allow them to match.
>
> The register allocators dealt with this correctly, and so used
> different input and output registers for instructions with Uat
> operands.  However, constrain_operands carried the penultimate
> alternative's matching rule over to the final alternative,
> so it would essentially ignore the earlyclobber.  This in turn
> allowed postreload to convert a correct Uat pairing into an
> incorrect one.
>
> The fix is simple: recompute the matching information for each
> alternative.
>
> Tested on aarch64-linux-gnu, aarch64_be-elf and x86_64-linux-gnu.
> OK to install?
>
> Richard
>
>
> gcc/
>         PR rtl-optimization/97144
>         * recog.c (constrain_operands): Initialize matching_operand
>         for each alternative, rather than only doing it once.
>
> gcc/testsuite/
>         PR rtl-optimization/97144
>         * gcc.target/aarch64/sve/pr97144.c: New test.
> ---
>  gcc/recog.c                                   |  8 +++---
>  .../gcc.target/aarch64/sve/pr97144.c          | 26 +++++++++++++++++++
>  2 files changed, 30 insertions(+), 4 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/aarch64/sve/pr97144.c
>
> diff --git a/gcc/recog.c b/gcc/recog.c
> index e9aa1ba253d..29d6c491e15 100644
> --- a/gcc/recog.c
> +++ b/gcc/recog.c
> @@ -3022,10 +3022,7 @@ constrain_operands (int strict, alternative_mask alternatives)
>      return 1;
>
>    for (c = 0; c < recog_data.n_operands; c++)
> -    {
> -      constraints[c] = recog_data.constraints[c];
> -      matching_operands[c] = -1;
> -    }
> +    constraints[c] = recog_data.constraints[c];
>
>    do
>      {
> @@ -3045,6 +3042,9 @@ constrain_operands (int strict, alternative_mask alternatives)
>           continue;
>         }
>
> +      for (opno = 0; opno < recog_data.n_operands; opno++)
> +       matching_operands[opno] = -1;
> +
>        for (opno = 0; opno < recog_data.n_operands; opno++)
>         {
>           rtx op = recog_data.operand[opno];
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pr97144.c b/gcc/testsuite/gcc.target/aarch64/sve/pr97144.c
> new file mode 100644
> index 00000000000..75245e2350b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pr97144.c
> @@ -0,0 +1,26 @@
> +/* { dg-options "-O2 -ftree-vectorize" } */
> +
> +int a, b = 5, c = 3;
> +char d;
> +char e[1];
> +int f[] = {0, 0, 1};
> +short g;
> +char *h = e;
> +void i(void) { b = a; }
> +static void j(void) {
> +  h = e;
> +  if (f[2])
> +  k:
> +    for (;;) {
> +      for (c = 0; c <= 4; c++) {
> +        for (g = 0; g <= 4; g++)
> +          f[g + 4] &= 2;
> +      }
> +      if (d)
> +        goto k;
> +    }
> +}
> +void l(void) {
> +  j();
> +  c = 0;
> +}

Is this test specific to aarch64?

-- 
H.J.

Re: [PATCH] recog: Fix a constrain_operands corner case [PR97144]

[Richard Sandiford]

"H.J. Lu" <hjl.tools@gmail.com> writes:
> On Thu, Dec 31, 2020 at 7:57 AM Richard Sandiford via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> aarch64's *add<mode>3_poly_1 has a pattern with the constraints:
>>
>>   "=...,r,&r"
>>   "...,0,rk"
>>   "...,Uai,Uat"
>>
>> i.e. the penultimate alternative requires operands 0 and 1 to match,
>> but the final alternative does not allow them to match.
>>
>> The register allocators dealt with this correctly, and so used
>> different input and output registers for instructions with Uat
>> operands.  However, constrain_operands carried the penultimate
>> alternative's matching rule over to the final alternative,
>> so it would essentially ignore the earlyclobber.  This in turn
>> allowed postreload to convert a correct Uat pairing into an
>> incorrect one.
>>
>> The fix is simple: recompute the matching information for each
>> alternative.
>>
>> Tested on aarch64-linux-gnu, aarch64_be-elf and x86_64-linux-gnu.
>> OK to install?
>>
>> Richard
>>
>>
>> gcc/
>>         PR rtl-optimization/97144
>>         * recog.c (constrain_operands): Initialize matching_operand
>>         for each alternative, rather than only doing it once.
>>
>> gcc/testsuite/
>>         PR rtl-optimization/97144
>>         * gcc.target/aarch64/sve/pr97144.c: New test.
>> ---
>>  gcc/recog.c                                   |  8 +++---
>>  .../gcc.target/aarch64/sve/pr97144.c          | 26 +++++++++++++++++++
>>  2 files changed, 30 insertions(+), 4 deletions(-)
>>  create mode 100644 gcc/testsuite/gcc.target/aarch64/sve/pr97144.c
>>
>> diff --git a/gcc/recog.c b/gcc/recog.c
>> index e9aa1ba253d..29d6c491e15 100644
>> --- a/gcc/recog.c
>> +++ b/gcc/recog.c
>> @@ -3022,10 +3022,7 @@ constrain_operands (int strict, alternative_mask alternatives)
>>      return 1;
>>
>>    for (c = 0; c < recog_data.n_operands; c++)
>> -    {
>> -      constraints[c] = recog_data.constraints[c];
>> -      matching_operands[c] = -1;
>> -    }
>> +    constraints[c] = recog_data.constraints[c];
>>
>>    do
>>      {
>> @@ -3045,6 +3042,9 @@ constrain_operands (int strict, alternative_mask alternatives)
>>           continue;
>>         }
>>
>> +      for (opno = 0; opno < recog_data.n_operands; opno++)
>> +       matching_operands[opno] = -1;
>> +
>>        for (opno = 0; opno < recog_data.n_operands; opno++)
>>         {
>>           rtx op = recog_data.operand[opno];
>> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pr97144.c b/gcc/testsuite/gcc.target/aarch64/sve/pr97144.c
>> new file mode 100644
>> index 00000000000..75245e2350b
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pr97144.c
>> @@ -0,0 +1,26 @@
>> +/* { dg-options "-O2 -ftree-vectorize" } */
>> +
>> +int a, b = 5, c = 3;
>> +char d;
>> +char e[1];
>> +int f[] = {0, 0, 1};
>> +short g;
>> +char *h = e;
>> +void i(void) { b = a; }
>> +static void j(void) {
>> +  h = e;
>> +  if (f[2])
>> +  k:
>> +    for (;;) {
>> +      for (c = 0; c <= 4; c++) {
>> +        for (g = 0; g <= 4; g++)
>> +          f[g + 4] &= 2;
>> +      }
>> +      if (d)
>> +        goto k;
>> +    }
>> +}
>> +void l(void) {
>> +  j();
>> +  c = 0;
>> +}
>
> Is this test specific to aarch64?

The bug needs SVE to be enabled, and the containing .exp harness
has special logic to force that where necessary (while not forcing
it unnecessarily).

But apart from that no.  I guess it'd also useful as a
gcc.c-torture/compile test, so I can put a copy there too
(without dg-options).

Thanks,
Richard

Re: [PATCH] recog: Fix a constrain_operands corner case [PR97144]

[H.J. Lu]

On Thu, Dec 31, 2020 at 8:21 AM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> "H.J. Lu" <hjl.tools@gmail.com> writes:
> > On Thu, Dec 31, 2020 at 7:57 AM Richard Sandiford via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >> aarch64's *add<mode>3_poly_1 has a pattern with the constraints:
> >>
> >>   "=...,r,&r"
> >>   "...,0,rk"
> >>   "...,Uai,Uat"
> >>
> >> i.e. the penultimate alternative requires operands 0 and 1 to match,
> >> but the final alternative does not allow them to match.
> >>
> >> The register allocators dealt with this correctly, and so used
> >> different input and output registers for instructions with Uat
> >> operands.  However, constrain_operands carried the penultimate
> >> alternative's matching rule over to the final alternative,
> >> so it would essentially ignore the earlyclobber.  This in turn
> >> allowed postreload to convert a correct Uat pairing into an
> >> incorrect one.
> >>
> >> The fix is simple: recompute the matching information for each
> >> alternative.
> >>
> >> Tested on aarch64-linux-gnu, aarch64_be-elf and x86_64-linux-gnu.
> >> OK to install?
> >>
> >> Richard
> >>
> >>
> >> gcc/
> >>         PR rtl-optimization/97144
> >>         * recog.c (constrain_operands): Initialize matching_operand
> >>         for each alternative, rather than only doing it once.
> >>
> >> gcc/testsuite/
> >>         PR rtl-optimization/97144
> >>         * gcc.target/aarch64/sve/pr97144.c: New test.
> >> ---
> >>  gcc/recog.c                                   |  8 +++---
> >>  .../gcc.target/aarch64/sve/pr97144.c          | 26 +++++++++++++++++++
> >>  2 files changed, 30 insertions(+), 4 deletions(-)
> >>  create mode 100644 gcc/testsuite/gcc.target/aarch64/sve/pr97144.c
> >>
> >> diff --git a/gcc/recog.c b/gcc/recog.c
> >> index e9aa1ba253d..29d6c491e15 100644
> >> --- a/gcc/recog.c
> >> +++ b/gcc/recog.c
> >> @@ -3022,10 +3022,7 @@ constrain_operands (int strict, alternative_mask alternatives)
> >>      return 1;
> >>
> >>    for (c = 0; c < recog_data.n_operands; c++)
> >> -    {
> >> -      constraints[c] = recog_data.constraints[c];
> >> -      matching_operands[c] = -1;
> >> -    }
> >> +    constraints[c] = recog_data.constraints[c];
> >>
> >>    do
> >>      {
> >> @@ -3045,6 +3042,9 @@ constrain_operands (int strict, alternative_mask alternatives)
> >>           continue;
> >>         }
> >>
> >> +      for (opno = 0; opno < recog_data.n_operands; opno++)
> >> +       matching_operands[opno] = -1;
> >> +
> >>        for (opno = 0; opno < recog_data.n_operands; opno++)
> >>         {
> >>           rtx op = recog_data.operand[opno];
> >> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pr97144.c b/gcc/testsuite/gcc.target/aarch64/sve/pr97144.c
> >> new file mode 100644
> >> index 00000000000..75245e2350b
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pr97144.c
> >> @@ -0,0 +1,26 @@
> >> +/* { dg-options "-O2 -ftree-vectorize" } */
> >> +
> >> +int a, b = 5, c = 3;
> >> +char d;
> >> +char e[1];
> >> +int f[] = {0, 0, 1};
> >> +short g;
> >> +char *h = e;
> >> +void i(void) { b = a; }
> >> +static void j(void) {
> >> +  h = e;
> >> +  if (f[2])
> >> +  k:
> >> +    for (;;) {
> >> +      for (c = 0; c <= 4; c++) {
> >> +        for (g = 0; g <= 4; g++)
> >> +          f[g + 4] &= 2;
> >> +      }
> >> +      if (d)
> >> +        goto k;
> >> +    }
> >> +}
> >> +void l(void) {
> >> +  j();
> >> +  c = 0;
> >> +}
> >
> > Is this test specific to aarch64?
>
> The bug needs SVE to be enabled, and the containing .exp harness
> has special logic to force that where necessary (while not forcing
> it unnecessarily).
>
> But apart from that no.  I guess it'd also useful as a
> gcc.c-torture/compile test, so I can put a copy there too
> (without dg-options).
>

Thanks.


-- 
H.J.