Re: [PATCH] constraint: fix relaxed memory and repeated constraint handling

[Richard Sandiford <richard.sandiford@arm.com>]

"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.
> ---
>  gcc/lra-constraints.cc | 43 +++++++++++++++++++++++++++++++++++++++---
>  gcc/recog.cc           |  3 ++-
>  2 files changed, 42 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/lra-constraints.cc b/gcc/lra-constraints.cc
> index dbfaf0485..c9c1653c0 100644
> --- a/gcc/lra-constraints.cc
> +++ b/gcc/lra-constraints.cc
> @@ -3448,6 +3448,45 @@ skip_constraint_modifiers (const char *str)
>        }
>  }
>  
> +/*  Takes a string of 0 or more comma-separated constraints and the
> +    constraint_num correspondig to the first constraint.  When more
> +    than one constraint 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").  */

Minor formatting nit: indentation should be to "/* " rather than "/*  ".

> +static bool
> +constraint_unique (const char *cstr, enum constraint_num ca)
> +{
> +   enum constraint_num cb;
> +   for (;;)
> +     {
> +       /* Skip past current constraint and any whitespace which may
> +	  precede the end-of-line or separator characters.  */
> +       cstr = skip_constraint_modifiers (cstr
> +					 + CONSTRAINT_LEN (cstr[0], cstr));
> +       /* If end of string reached and no disagreement found, we have
> +	  uniqueness.  */
> +       if (*cstr == '\0')
> +	 return true;
> +       /* skip_constraint_modifiers does not handle commas, handle
> +	  case manually.  */
> +       if (*cstr == ',')
> +	 cstr++;
> +       /* Get next constraint.  */
> +       cstr =  skip_constraint_modifiers (cstr);
> +       cb = lookup_constraint ((*cstr == '\0' || *cstr == ',') ? "X" : cstr);
> +
> +       /* If mismatch found, break out of loop.  */
> +       if (cb != ca)
> +	 return false;
> +
> +       /* If *cstr == '\0', we don't want to reach the
> +	  skip_constraint_modifiers statement again as that will
> +	  advance the pointer past the end of the string.  */
> +       if (*cstr == '\0')
> +	 return true;
> +     }
> +}

How about rearranging this a bit to something like:

  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);
        }
      if (ca == CONSTRAINT__UNKNOWN)
        ca = cb
      else if (ca != cb)
        return false;
      if (*cstr == '\0')
        return true;
      if (*cstr == ',')
        cstr += 1;
    }

That way we only do one lookup per loop iteration.  It also avoids
CONSTRAINT_LEN for the "empty, followed by comma" case.

If that works, the patch is OK with those changes once another
approved-for-GCC-13 patch needs it.  OK for GCC 14 otherwise.

Thanks,
Richard

> +
>  /* 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 +3546,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))
>      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 200cf4214..3ddeab59d 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