diff mbox series

[v3] constraint: fix relaxed memory and repeated constraint handling

Message ID yw8ja5z5jtzm.fsf@arm.com
State Accepted
Headers
Series [v3] constraint: fix relaxed memory and repeated constraint handling |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Victor Do Nascimento April 18, 2023, 11:43 a.m. UTC 
  The function `constrain_operands' lacked the logic to consider relaxed
memory constraints when "traditional" memory constraints were not
satisfied, creating potential issues as observed during the reload
compilation pass.

In addition, it was observed that while `constrain_operands' chooses
to disregard constraints when more than one alternative is provided,
e.g. "m,r" using CONSTRAINT__UNKNOWN, it has no checks in place to
determine whether the multiple constraints in a given string are in
fact repetitions of the same constraint and should thus in fact be
treated as a single constraint, as ought to be the case for something
like "m,m".

Both of these issues are dealt with here, thus ensuring that we get
appropriate pattern matching.

Tested on aarch64-linux-gnu & x86_64-linux-gnu.  OK to install?

Victor

gcc/
	* lra-constraints.cc (constraint_unique): New.
	(process_address_1): Apply constraint_unique test.
	* recog.cc (constrain_operands): Allow relaxed memory
	constaints.
---
 gcc/lra-constraints.cc | 39 ++++++++++++++++++++++++++++++++++++---
 gcc/recog.cc           |  3 ++-
 2 files changed, 38 insertions(+), 4 deletions(-)

Comments

Richard Sandiford April 18, 2023, 1:02 p.m. UTC | #1 
"Victor L. Do Nascimento" <victor.donascimento@arm.com> writes:
> The function `constrain_operands' lacked the logic to consider relaxed
> memory constraints when "traditional" memory constraints were not
> satisfied, creating potential issues as observed during the reload
> compilation pass.
>
> In addition, it was observed that while `constrain_operands' chooses
> to disregard constraints when more than one alternative is provided,
> e.g. "m,r" using CONSTRAINT__UNKNOWN, it has no checks in place to
> determine whether the multiple constraints in a given string are in
> fact repetitions of the same constraint and should thus in fact be
> treated as a single constraint, as ought to be the case for something
> like "m,m".
>
> Both of these issues are dealt with here, thus ensuring that we get
> appropriate pattern matching.
>
> Tested on aarch64-linux-gnu & x86_64-linux-gnu.  OK to install?
>
> Victor
>
> gcc/
> 	* lra-constraints.cc (constraint_unique): New.
> 	(process_address_1): Apply constraint_unique test.
> 	* recog.cc (constrain_operands): Allow relaxed memory
> 	constaints.

OK, thanks.

Richard

> ---
>  gcc/lra-constraints.cc | 39 ++++++++++++++++++++++++++++++++++++---
>  gcc/recog.cc           |  3 ++-
>  2 files changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/lra-constraints.cc b/gcc/lra-constraints.cc
> index dd4f68bbfc0..a210d6e0697 100644
> --- a/gcc/lra-constraints.cc
> +++ b/gcc/lra-constraints.cc
> @@ -3448,6 +3448,41 @@ skip_constraint_modifiers (const char *str)
>        }
>  }
>  
> +/* Takes a string of 0 or more comma-separated constraints.  When more
> +   than one constraint is present, evaluate whether they all correspond
> +   to a single, repeated constraint (e.g. "r,r") or whether we have
> +   more than one distinct constraints (e.g. "r,m").  */
> +static bool
> +constraint_unique (const char *cstr)
> +{
> +  enum constraint_num ca, cb;
> +  ca = CONSTRAINT__UNKNOWN;
> +  for (;;)
> +    {
> +      cstr = skip_constraint_modifiers (cstr);
> +      if (*cstr == '\0' || *cstr == ',')
> +	cb = CONSTRAINT_X;
> +      else
> +	{
> +	  cb = lookup_constraint (cstr);
> +	  if (cb == CONSTRAINT__UNKNOWN)
> +	    return false;
> +	  cstr += CONSTRAINT_LEN (cstr[0], cstr);
> +	}
> +      /* Handle the first iteration of the loop.  */
> +      if (ca == CONSTRAINT__UNKNOWN)
> +	ca = cb;
> +      /* Handle the general case of comparing ca with subsequent
> +	 constraints.  */
> +      else if (ca != cb)
> +	return false;
> +      if (*cstr == '\0')
> +	return true;
> +      if (*cstr == ',')
> +	cstr += 1;
> +    }
> +}
> +
>  /* Major function to make reloads for an address in operand NOP or
>     check its correctness (If CHECK_ONLY_P is true). The supported
>     cases are:
> @@ -3507,9 +3542,7 @@ process_address_1 (int nop, bool check_only_p,
>       operand has one address constraint, probably all others constraints are
>       address ones.  */
>    if (constraint[0] != '\0' && get_constraint_type (cn) != CT_ADDRESS
> -      && *skip_constraint_modifiers (constraint
> -				     + CONSTRAINT_LEN (constraint[0],
> -						       constraint)) != '\0')
> +      && !constraint_unique (constraint))
>      cn = CONSTRAINT__UNKNOWN;
>    if (insn_extra_address_constraint (cn)
>        /* When we find an asm operand with an address constraint that
> diff --git a/gcc/recog.cc b/gcc/recog.cc
> index 200cf4214f1..3ddeab59d92 100644
> --- a/gcc/recog.cc
> +++ b/gcc/recog.cc
> @@ -3234,7 +3234,8 @@ constrain_operands (int strict, alternative_mask alternatives)
>  		  else if (constraint_satisfied_p (op, cn))
>  		    win = 1;
>  
> -		  else if (insn_extra_memory_constraint (cn)
> +		  else if ((insn_extra_memory_constraint (cn)
> +			    || insn_extra_relaxed_memory_constraint (cn))
>  			   /* Every memory operand can be reloaded to fit.  */
>  			   && ((strict < 0 && MEM_P (op))
>  			       /* Before reload, accept what reload can turn
Jeff Law April 18, 2023, 1:37 p.m. UTC | #2 
On 4/18/23 07:02, Richard Sandiford via Gcc-patches wrote:
> "Victor L. Do Nascimento" <victor.donascimento@arm.com> writes:
>> The function `constrain_operands' lacked the logic to consider relaxed
>> memory constraints when "traditional" memory constraints were not
>> satisfied, creating potential issues as observed during the reload
>> compilation pass.
>>
>> In addition, it was observed that while `constrain_operands' chooses
>> to disregard constraints when more than one alternative is provided,
>> e.g. "m,r" using CONSTRAINT__UNKNOWN, it has no checks in place to
>> determine whether the multiple constraints in a given string are in
>> fact repetitions of the same constraint and should thus in fact be
>> treated as a single constraint, as ought to be the case for something
>> like "m,m".
>>
>> Both of these issues are dealt with here, thus ensuring that we get
>> appropriate pattern matching.
>>
>> Tested on aarch64-linux-gnu & x86_64-linux-gnu.  OK to install?
>>
>> Victor
>>
>> gcc/
>> 	* lra-constraints.cc (constraint_unique): New.
>> 	(process_address_1): Apply constraint_unique test.
>> 	* recog.cc (constrain_operands): Allow relaxed memory
>> 	constaints.
> 
> OK, thanks.
Does Victor have write access?  If not you should probably cover the 
commit for him.  If Victor is going to be making regular contributions, 
then we should probably get him write access going forward.

jeff
Richard Sandiford April 18, 2023, 4:13 p.m. UTC | #3 
Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> On 4/18/23 07:02, Richard Sandiford via Gcc-patches wrote:
>> "Victor L. Do Nascimento" <victor.donascimento@arm.com> writes:
>>> The function `constrain_operands' lacked the logic to consider relaxed
>>> memory constraints when "traditional" memory constraints were not
>>> satisfied, creating potential issues as observed during the reload
>>> compilation pass.
>>>
>>> In addition, it was observed that while `constrain_operands' chooses
>>> to disregard constraints when more than one alternative is provided,
>>> e.g. "m,r" using CONSTRAINT__UNKNOWN, it has no checks in place to
>>> determine whether the multiple constraints in a given string are in
>>> fact repetitions of the same constraint and should thus in fact be
>>> treated as a single constraint, as ought to be the case for something
>>> like "m,m".
>>>
>>> Both of these issues are dealt with here, thus ensuring that we get
>>> appropriate pattern matching.
>>>
>>> Tested on aarch64-linux-gnu & x86_64-linux-gnu.  OK to install?
>>>
>>> Victor
>>>
>>> gcc/
>>> 	* lra-constraints.cc (constraint_unique): New.
>>> 	(process_address_1): Apply constraint_unique test.
>>> 	* recog.cc (constrain_operands): Allow relaxed memory
>>> 	constaints.
>> 
>> OK, thanks.
> Does Victor have write access?  If not you should probably cover the 
> commit for him.

Ah, right, thanks.  I've pushed it now.

> If Victor is going to be making regular contributions, 
> then we should probably get him write access going forward.

Yeah.

Richard
diff mbox series

Patch

diff --git a/gcc/lra-constraints.cc b/gcc/lra-constraints.cc
index dd4f68bbfc0..a210d6e0697 100644
--- a/gcc/lra-constraints.cc
+++ b/gcc/lra-constraints.cc
@@ -3448,6 +3448,41 @@  skip_constraint_modifiers (const char *str)
       }
 }
 
+/* Takes a string of 0 or more comma-separated constraints.  When more
+   than one constraint is present, evaluate whether they all correspond
+   to a single, repeated constraint (e.g. "r,r") or whether we have
+   more than one distinct constraints (e.g. "r,m").  */
+static bool
+constraint_unique (const char *cstr)
+{
+  enum constraint_num ca, cb;
+  ca = CONSTRAINT__UNKNOWN;
+  for (;;)
+    {
+      cstr = skip_constraint_modifiers (cstr);
+      if (*cstr == '\0' || *cstr == ',')
+	cb = CONSTRAINT_X;
+      else
+	{
+	  cb = lookup_constraint (cstr);
+	  if (cb == CONSTRAINT__UNKNOWN)
+	    return false;
+	  cstr += CONSTRAINT_LEN (cstr[0], cstr);
+	}
+      /* Handle the first iteration of the loop.  */
+      if (ca == CONSTRAINT__UNKNOWN)
+	ca = cb;
+      /* Handle the general case of comparing ca with subsequent
+	 constraints.  */
+      else if (ca != cb)
+	return false;
+      if (*cstr == '\0')
+	return true;
+      if (*cstr == ',')
+	cstr += 1;
+    }
+}
+
 /* Major function to make reloads for an address in operand NOP or
    check its correctness (If CHECK_ONLY_P is true). The supported
    cases are:
@@ -3507,9 +3542,7 @@  process_address_1 (int nop, bool check_only_p,
      operand has one address constraint, probably all others constraints are
      address ones.  */
   if (constraint[0] != '\0' && get_constraint_type (cn) != CT_ADDRESS
-      && *skip_constraint_modifiers (constraint
-				     + CONSTRAINT_LEN (constraint[0],
-						       constraint)) != '\0')
+      && !constraint_unique (constraint))
     cn = CONSTRAINT__UNKNOWN;
   if (insn_extra_address_constraint (cn)
       /* When we find an asm operand with an address constraint that
diff --git a/gcc/recog.cc b/gcc/recog.cc
index 200cf4214f1..3ddeab59d92 100644
--- a/gcc/recog.cc
+++ b/gcc/recog.cc
@@ -3234,7 +3234,8 @@  constrain_operands (int strict, alternative_mask alternatives)
 		  else if (constraint_satisfied_p (op, cn))
 		    win = 1;
 
-		  else if (insn_extra_memory_constraint (cn)
+		  else if ((insn_extra_memory_constraint (cn)
+			    || insn_extra_relaxed_memory_constraint (cn))
 			   /* Every memory operand can be reloaded to fit.  */
 			   && ((strict < 0 && MEM_P (op))
 			       /* Before reload, accept what reload can turn