Re: [PATCH 06/13] [APX EGPR] Map reg/mem constraints in inline asm to non-EGPR constraint.

[Jakub Jelinek <jakub@redhat.com>]

On Thu, Aug 31, 2023 at 04:20:17PM +0800, Hongyu Wang via Gcc-patches wrote:
> From: Kong Lingling <lingling.kong@intel.com>
> 
> In inline asm, we do not know if the insn can use EGPR, so disable EGPR
> usage by default from mapping the common reg/mem constraint to non-EGPR
> constraints. Use a flag mapx-inline-asm-use-gpr32 to enable EGPR usage
> for inline asm.
> 
> gcc/ChangeLog:
> 
> 	* config/i386/i386.cc (INCLUDE_STRING): Add include for
> 	ix86_md_asm_adjust.
> 	(ix86_md_asm_adjust): When APX EGPR enabled without specifying the
> 	target option, map reg/mem constraints to non-EGPR constraints.
> 	* config/i386/i386.opt: Add option mapx-inline-asm-use-gpr32.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/i386/apx-inline-gpr-norex2.c: New test.
> ---
>  gcc/config/i386/i386.cc                       |  44 +++++++
>  gcc/config/i386/i386.opt                      |   5 +
>  .../gcc.target/i386/apx-inline-gpr-norex2.c   | 107 ++++++++++++++++++
>  3 files changed, 156 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/i386/apx-inline-gpr-norex2.c
> 
> diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc
> index d26d9ab0d9d..9460ebbfda4 100644
> --- a/gcc/config/i386/i386.cc
> +++ b/gcc/config/i386/i386.cc
> @@ -17,6 +17,7 @@ You should have received a copy of the GNU General Public License
>  along with GCC; see the file COPYING3.  If not see
>  <http://www.gnu.org/licenses/>.  */
>  
> +#define INCLUDE_STRING
>  #define IN_TARGET_CODE 1
>  
>  #include "config.h"
> @@ -23077,6 +23078,49 @@ ix86_md_asm_adjust (vec<rtx> &outputs, vec<rtx> & /*inputs*/,
>    bool saw_asm_flag = false;
>  
>    start_sequence ();
> +  /* TODO: Here we just mapped the general r/m constraints to non-EGPR
> +   constraints, will eventually map all the usable constraints in the future. */

I think there should be some constraint which explicitly has all the 32
GPRs, like there is one for just all 16 GPRs (h), so that regardless of
-mapx-inline-asm-use-gpr32 one can be explicit what the inline asm wants.

Also, what about the "g" constraint?  Shouldn't there be another for "g"
without r16..r31?  What about the various other memory
constraints ("<", "o", ...)?

> +  if (TARGET_APX_EGPR && !ix86_apx_inline_asm_use_gpr32)
> +    {
> +      /* Map "r" constraint in inline asm to "h" that disallows r16-r31
> +	 and replace only r, exclude Br and Yr.  */
> +      for (unsigned i = 0; i < constraints.length (); i++)
> +	{
> +	  std::string *s = new std::string (constraints[i]);

Doesn't this leak memory (all the time)?
I must say I don't really understand why you need to use std::string here,
but certainly it shouldn't leak.

> +	  size_t pos = s->find ('r');
> +	  while (pos != std::string::npos)
> +	    {
> +	      if (pos > 0
> +		  && (s->at (pos - 1) == 'Y' || s->at (pos - 1) == 'B'))
> +		pos = s->find ('r', pos + 1);
> +	      else
> +		{
> +		  s->replace (pos, 1, "h");
> +		  constraints[i] = (const char*) s->c_str ();

Formatting (space before *).  The usual way for constraints is ggc_strdup on
some string in a buffer.  Also, one could have several copies or r (or m, memory (doesn't
that appear just in clobbers?  And that doesn't look like something that
should be replaced), Bm, e.g. in various alternatives.  So, you
need to change them all, not just the first hit.  "r,r,r,m" and the like.
Normally, one would simply walk the constraint string, parsing the special
letters (+, =, & etc.) and single letter constraints and 2 letter
constraints using CONSTRAINT_LEN macro (tons of examples in GCC sources).
Either do it in 2 passes, first one counts how long constraint string one
will need after the adjustments (and whether to adjust something at all),
then if needed XALLOCAVEC it and adjust in there, or say use a
auto_vec<char, 32> for
it.

> +		  break;
> +		}
> +	    }
> +	}
> +      /* Also map "m/memory/Bm" constraint that may use GPR32, replace them with
> +	 "Bt/Bt/BT".  */
> +      for (unsigned i = 0; i < constraints.length (); i++)
> +	{
> +	  std::string *s = new std::string (constraints[i]);
> +	  size_t pos = s->find ("m");
> +	  size_t pos2 = s->find ("memory");
> +	  if (pos != std::string::npos)
> +	    {
> +	      if (pos > 0 && (s->at (pos - 1) == 'B'))
> +		  s->replace (pos - 1, 2, "BT");
> +	      else if (pos2 != std::string::npos)
> +		  s->replace (pos, 6, "Bt");
> +	      else
> +		  s->replace (pos, 1, "Bt");

Formatting, the s->replace calls are indented too much.

	Jakub