public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
From: "Cui, Lili" <lili.cui@intel.com>
To: "Beulich, Jan" <JBeulich@suse.com>, Binutils <binutils@sourceware.org>
Cc: "H.J. Lu" <hjl.tools@gmail.com>
Subject: RE: [PATCH 1/4] x86: rename vec_encoding and vex_encoding_*
Date: Sun, 18 Feb 2024 05:59:56 +0000	[thread overview]
Message-ID: <SJ0PR11MB56004CAC05FA61460BF562379E522@SJ0PR11MB5600.namprd11.prod.outlook.com> (raw)
In-Reply-To: <c5ea7548-e171-43ba-94c5-ffdb75be04cc@suse.com>

> Even with just VEX these weren't limited to vector insns. With APX the set of
> non-vector ones covered has greatly increased. Drop the vec_ prefix. Also drop
> the vex_ ones off of the enumerators, as they weren't appropriate anyway:
> Should have been vec_ then, too.
> 
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -445,16 +445,16 @@ struct _i386_insn
>      /* Disable instruction size optimization.  */
>      bool no_optimize;
> 
> -    /* How to encode vector instructions.  */
> +    /* How to encode instructions.  */
>      enum
>        {
> -	vex_encoding_default = 0,
> -	vex_encoding_vex,
> -	vex_encoding_vex3,
> -	vex_encoding_evex,
> -	vex_encoding_evex512,
> -	vex_encoding_error
> -      } vec_encoding;
> +	encoding_default = 0,
> +	encoding_vex,
> +	encoding_vex3,
> +	encoding_evex,
> +	encoding_evex512,

"encoding_evex512" seems redundant, duplicating with encoding_evex, do you know the reason for keeping it? Could you add some comments for " encoding_evex512", thanks.

Lili.

> +	encoding_error
> +      } encoding;
> 
>      /* REP prefix.  */
>      const char *rep_prefix;
> @@ -1885,8 +1885,8 @@ static const i386_cpu_flags avx512 = CPU
> 
>  static INLINE bool need_evex_encoding (const insn_template *t)  {
> -  return i.vec_encoding == vex_encoding_evex
> -	|| i.vec_encoding == vex_encoding_evex512
> +  return i.encoding == encoding_evex
> +	|| i.encoding == encoding_evex512
>  	|| (t->opcode_modifier.vex && i.has_egpr)
>  	|| i.mask.reg;
>  }
> @@ -2489,7 +2489,7 @@ static INLINE int
>  fits_in_imm4 (offsetT num)
>  {
>    /* Despite the name, check for imm3 if we're dealing with EVEX.  */
> -  return (num & (i.vec_encoding != vex_encoding_evex ? 0xf : 7)) == num;
> +  return (num & (i.encoding != encoding_evex ? 0xf : 7)) == num;
>  }
> 
>  static i386_operand_type
> @@ -3800,7 +3800,7 @@ build_vex_prefix (const insn_template *t
>    /* Use 2-byte VEX prefix by swapping destination and source operand
>       if there are more than 1 register operand.  */
>    if (i.reg_operands > 1
> -      && i.vec_encoding != vex_encoding_vex3
> +      && i.encoding != encoding_vex3
>        && i.dir_encoding == dir_encoding_default
>        && i.operands == i.reg_operands
>        && operand_type_equal (&i.types[0], &i.types[i.operands - 1]) @@ -
> 3829,7 +3829,7 @@ build_vex_prefix (const insn_template *t
>    /* Use 2-byte VEX prefix by swapping commutative source operands if there
>       are no memory operands and at least 3 register ones.  */
>    if (i.reg_operands >= 3
> -      && i.vec_encoding != vex_encoding_vex3
> +      && i.encoding != encoding_vex3
>        && i.reg_operands == i.operands - i.imm_operands
>        && i.tm.opcode_modifier.vex
>        && i.tm.opcode_modifier.commutative @@ -3892,7 +3892,7 @@
> build_vex_prefix (const insn_template *t
> 
>    /* Use 2-byte VEX prefix if possible.  */
>    if (w == 0
> -      && i.vec_encoding != vex_encoding_vex3
> +      && i.encoding != encoding_vex3
>        && i.tm.opcode_space == SPACE_0F
>        && (i.rex & (REX_W | REX_X | REX_B)) == 0)
>      {
> @@ -4758,7 +4758,7 @@ optimize_encoding (void)
>  	   && (i.tm.opcode_modifier.vex
>  	       || ((!i.mask.reg || i.mask.zeroing)
>  		   && i.tm.opcode_modifier.evex
> -		   && (i.vec_encoding != vex_encoding_evex
> +		   && (i.encoding != encoding_evex
>  		       || cpu_arch_isa_flags.bitfield.cpuavx512vl
>  		       || is_cpu (&i.tm, CpuAVX512VL)
>  		       || (i.tm.operand_types[2].bitfield.zmmword
> @@ -4808,12 +4808,12 @@ optimize_encoding (void)
>         */
>        if (i.tm.opcode_modifier.evex)
>  	{
> -	  if (i.vec_encoding != vex_encoding_evex)
> +	  if (i.encoding != encoding_evex)
>  	    {
>  	      i.tm.opcode_modifier.vex = VEX128;
>  	      i.tm.opcode_modifier.vexw = VEXW0;
>  	      i.tm.opcode_modifier.evex = 0;
> -	      i.vec_encoding = vex_encoding_vex;
> +	      i.encoding = encoding_vex;
>  	      i.mask.reg = NULL;
>  	    }
>  	  else if (optimize > 1)
> @@ -4836,7 +4836,7 @@ optimize_encoding (void)
>  	    i.types[j].bitfield.ymmword = 0;
>  	  }
>      }
> -  else if (i.vec_encoding != vex_encoding_evex
> +  else if (i.encoding != encoding_evex
>  	   && !i.types[0].bitfield.zmmword
>  	   && !i.types[1].bitfield.zmmword
>  	   && !i.mask.reg
> @@ -4978,7 +4978,7 @@ optimize_encoding (void)
>  	   && i.tm.opcode_modifier.vex
>  	   && !(i.op[0].regs->reg_flags & RegRex)
>  	   && i.op[0].regs->reg_type.bitfield.xmmword
> -	   && i.vec_encoding != vex_encoding_vex3)
> +	   && i.encoding != encoding_vex3)
>      {
>        /* Optimize: -Os:
>           vpbroadcastq %xmmN, %xmmM  ->
> vpunpcklqdq %xmmN, %xmmN, %xmmM (N < 8) @@ -6839,10 +6839,10
> @@ md_assemble (char *line)
>    if (optimize && !i.no_optimize && i.tm.opcode_modifier.optimize)
>      optimize_encoding ();
> 
> -  /* Past optimization there's no need to distinguish vex_encoding_evex and
> -     vex_encoding_evex512 anymore.  */
> -  if (i.vec_encoding == vex_encoding_evex512)
> -    i.vec_encoding = vex_encoding_evex;
> +  /* Past optimization there's no need to distinguish encoding_evex and
> +     encoding_evex512 anymore.  */
> +  if (i.encoding == encoding_evex512)
> +    i.encoding = encoding_evex;
> 
>    if (use_unaligned_vector_move)
>      encode_with_unaligned_vector_move (); @@ -7136,15 +7136,15 @@
> parse_insn (const char *line, char *mnem
>  		  break;
>  		case Prefix_VEX:
>  		  /* {vex} */
> -		  i.vec_encoding = vex_encoding_vex;
> +		  i.encoding = encoding_vex;
>  		  break;
>  		case Prefix_VEX3:
>  		  /* {vex3} */
> -		  i.vec_encoding = vex_encoding_vex3;
> +		  i.encoding = encoding_vex3;
>  		  break;
>  		case Prefix_EVEX:
>  		  /* {evex} */
> -		  i.vec_encoding = vex_encoding_evex;
> +		  i.encoding = encoding_evex;
>  		  break;
>  		case Prefix_REX:
>  		  /* {rex} */
> @@ -8260,7 +8260,7 @@ check_VecOperands (const insn_template *  static
> int  VEX_check_encoding (const insn_template *t)  {
> -  if (i.vec_encoding == vex_encoding_error)
> +  if (i.encoding == encoding_error)
>      {
>        i.error = unsupported;
>        return 1;
> @@ -8277,8 +8277,8 @@ VEX_check_encoding (const insn_template
>        return 1;
>      }
> 
> -  if (i.vec_encoding == vex_encoding_evex
> -      || i.vec_encoding == vex_encoding_evex512)
> +  if (i.encoding == encoding_evex
> +      || i.encoding == encoding_evex512)
>      {
>        /* This instruction must be encoded with EVEX prefix.  */
>        if (!t->opcode_modifier.evex)
> @@ -8292,7 +8292,7 @@ VEX_check_encoding (const insn_template
>    if (!t->opcode_modifier.vex)
>      {
>        /* This instruction template doesn't have VEX prefix.  */
> -      if (i.vec_encoding != vex_encoding_default)
> +      if (i.encoding != encoding_default)
>  	{
>  	  i.error = no_vex_encoding;
>  	  return 1;
> @@ -8967,7 +8967,7 @@ match_template (char mnem_suffix)
>  	 Note that the semantics have not been changed.  */
>        if (optimize
>  	  && !i.no_optimize
> -	  && i.vec_encoding != vex_encoding_evex
> +	  && i.encoding != encoding_evex
>  	  && ((t + 1 < current_templates.end
>  	       && !t[1].opcode_modifier.evex
>  	       && t[1].opcode_space <= SPACE_0F38 @@ -10157,7 +10157,7
> @@ process_operands (void)
>        if (dot_insn () && i.reg_operands == 2)
>  	{
>  	  gas_assert (is_any_vex_encoding (&i.tm)
> -		      || i.vec_encoding != vex_encoding_default);
> +		      || i.encoding != encoding_default);
>  	  i.vex.register_specifier = i.op[i.operands - 1].regs;
>  	}
>      }
> @@ -10167,7 +10167,7 @@ process_operands (void)
>  	      == InstanceNone)
>      {
>        gas_assert (is_any_vex_encoding (&i.tm)
> -		  || i.vec_encoding != vex_encoding_default);
> +		  || i.encoding != encoding_default);
>        i.vex.register_specifier = i.op[i.operands - 1].regs;
>      }
> 
> @@ -10275,7 +10275,7 @@ build_modrm_byte (void)
>  	}
>        exp->X_add_number |= register_number (i.op[reg_slot].regs)
>  			   << (3 + !(i.tm.opcode_modifier.evex
> -				     || i.vec_encoding == vex_encoding_evex));
> +				     || i.encoding == encoding_evex));
>      }
> 
>    if (i.tm.opcode_modifier.vexvvvv == VexVVVV_DST) @@ -12548,20
> +12548,20 @@ s_insn (int dummy ATTRIBUTE_UNUSED)
>      }
> 
>    if (vex || xop
> -      ? i.vec_encoding == vex_encoding_evex
> +      ? i.encoding == encoding_evex
>        : evex
> -	? i.vec_encoding == vex_encoding_vex
> -	  || i.vec_encoding == vex_encoding_vex3
> -	: i.vec_encoding != vex_encoding_default)
> +	? i.encoding == encoding_vex
> +	  || i.encoding == encoding_vex3
> +	: i.encoding != encoding_default)
>      {
>        as_bad (_("pseudo-prefix conflicts with encoding specifier"));
>        goto bad;
>      }
> 
> -  if (line > end && i.vec_encoding == vex_encoding_default)
> -    i.vec_encoding = evex ? vex_encoding_evex : vex_encoding_vex;
> +  if (line > end && i.encoding == encoding_default)
> +    i.encoding = evex ? encoding_evex : encoding_vex;
> 
> -  if (i.vec_encoding != vex_encoding_default)
> +  if (i.encoding != encoding_default)
>      {
>        /* Only address size and segment override prefixes are permitted with
>           VEX/XOP/EVEX encodings.  */
> @@ -12892,20 +12892,20 @@ s_insn (int dummy ATTRIBUTE_UNUSED)
>  	  goto done;
>  	}
> 
> -      /* No need to distinguish vex_encoding_evex and vex_encoding_evex512.
> */
> -      if (i.vec_encoding == vex_encoding_evex512)
> -	i.vec_encoding = vex_encoding_evex;
> +      /* No need to distinguish encoding_evex and encoding_evex512.  */
> +      if (i.encoding == encoding_evex512)
> +	i.encoding = encoding_evex;
> 
>        /* Are we to emit ModR/M encoding?  */
>        if (!i.short_form
>  	  && (i.mem_operands
> -	      || i.reg_operands > (i.vec_encoding != vex_encoding_default)
> +	      || i.reg_operands > (i.encoding != encoding_default)
>  	      || i.tm.extension_opcode != None))
>  	i.tm.opcode_modifier.modrm = 1;
> 
>        if (!i.tm.opcode_modifier.modrm
>  	  && (i.reg_operands
> -	      > i.short_form + 0U + (i.vec_encoding != vex_encoding_default)
> +	      > i.short_form + 0U + (i.encoding != encoding_default)
>  	      || i.mem_operands))
>  	{
>  	  as_bad (_("too many register/memory operands")); @@ -12944,7
> +12944,7 @@ s_insn (int dummy ATTRIBUTE_UNUSED)
>  	    }
>  	  /* Fall through.  */
>  	case 3:
> -	  if (i.vec_encoding != vex_encoding_default)
> +	  if (i.encoding != encoding_default)
>  	    {
>  	      i.tm.opcode_modifier.vexvvvv = 1;
>  	      break;
> @@ -13000,13 +13000,13 @@ s_insn (int dummy ATTRIBUTE_UNUSED)
>  	      || i.index_reg->reg_type.bitfield.ymmword
>  	      || i.index_reg->reg_type.bitfield.zmmword))
>  	{
> -	  if (i.vec_encoding == vex_encoding_default)
> +	  if (i.encoding == encoding_default)
>  	    {
>  	      as_bad (_("VSIB unavailable with legacy encoding"));
>  	      goto done;
>  	    }
> 
> -	  if (i.vec_encoding == vex_encoding_evex
> +	  if (i.encoding == encoding_evex
>  	      && i.reg_operands > 1)
>  	    {
>  	      /* We could allow two register operands, encoding the 2nd one in
> @@ -13026,7 +13026,7 @@ s_insn (int dummy ATTRIBUTE_UNUSED)
>        for (j = i.imm_operands; j < i.operands; ++j)
>  	{
>  	  /* Look for 8-bit operands that use old registers.  */
> -	  if (i.vec_encoding != vex_encoding_default
> +	  if (i.encoding != encoding_default
>  	      && flag_code == CODE_64BIT
>  	      && i.types[j].bitfield.class == Reg
>  	      && i.types[j].bitfield.byte
> @@ -13085,7 +13085,7 @@ s_insn (int dummy ATTRIBUTE_UNUSED)
>  	case  4: combined.bitfield.dword = 1; break;
>  	}
> 
> -      if (i.vec_encoding == vex_encoding_default)
> +      if (i.encoding == encoding_default)
>  	{
>  	  if (flag_code == CODE_64BIT && combined.bitfield.qword)
>  	    i.rex |= REX_W;
> @@ -13162,7 +13162,7 @@ s_insn (int dummy ATTRIBUTE_UNUSED)
>        if (i.memshift >= 32)
>  	i.memshift = 0;
>        else if (!evex)
> -	i.vec_encoding = vex_encoding_error;
> +	i.encoding = encoding_error;
> 
>        if (i.disp_operands && !optimize_disp (&i.tm))
>  	goto done;
> @@ -13228,8 +13228,8 @@ s_insn (int dummy ATTRIBUTE_UNUSED)
>       potential special casing there.  */
>    i.tm.base_opcode |= val;
> 
> -  if (i.vec_encoding == vex_encoding_error
> -      || (i.vec_encoding != vex_encoding_evex
> +  if (i.encoding == encoding_error
> +      || (i.encoding != encoding_evex
>  	  ? i.broadcast.type || i.broadcast.bytes
>  	    || i.rounding.type != rc_none
>  	    || i.mask.reg
> @@ -13340,10 +13340,10 @@ RC_SAE_specifier (const char *pstr)
>  	      return NULL;
>  	    }
> 
> -	  if (i.vec_encoding == vex_encoding_default)
> -	    i.vec_encoding = vex_encoding_evex512;
> -	  else if (i.vec_encoding != vex_encoding_evex
> -		   && i.vec_encoding != vex_encoding_evex512)
> +	  if (i.encoding == encoding_default)
> +	    i.encoding = encoding_evex512;
> +	  else if (i.encoding != encoding_evex
> +		   && i.encoding != encoding_evex512)
>  	    return NULL;
> 
>  	  i.rounding.type = RC_NamesTable[j].type; @@ -13405,10 +13405,10
> @@ check_VecOperations (char *op_string)
>  		}
>  	      op_string++;
> 
> -	      if (i.vec_encoding == vex_encoding_default)
> -		i.vec_encoding = vex_encoding_evex;
> -	      else if (i.vec_encoding != vex_encoding_evex
> -		       && i.vec_encoding != vex_encoding_evex512)
> +	      if (i.encoding == encoding_default)
> +		i.encoding = encoding_evex;
> +	      else if (i.encoding != encoding_evex
> +		       && i.encoding != encoding_evex512)
>  		goto unknown_vec_op;
> 
>  	      i.broadcast.type = bcst_type;
> @@ -15676,11 +15676,11 @@ static bool check_register (const reg_en
>        if (vector_size < VSZ512)
>  	return false;
> 
> -      if (i.vec_encoding == vex_encoding_default)
> -	i.vec_encoding = vex_encoding_evex512;
> -      else if (i.vec_encoding != vex_encoding_evex
> -	       && i.vec_encoding != vex_encoding_evex512)
> -	i.vec_encoding = vex_encoding_error;
> +      if (i.encoding == encoding_default)
> +	i.encoding = encoding_evex512;
> +      else if (i.encoding != encoding_evex
> +	       && i.encoding != encoding_evex512)
> +	i.encoding = encoding_error;
>      }
> 
>    if (vector_size < VSZ256 && r->reg_type.bitfield.ymmword) @@ -15706,11
> +15706,11 @@ static bool check_register (const reg_en
>  	  || flag_code != CODE_64BIT)
>  	return false;
> 
> -      if (i.vec_encoding == vex_encoding_default
> -	  || i.vec_encoding == vex_encoding_evex512)
> -	i.vec_encoding = vex_encoding_evex;
> -      else if (i.vec_encoding != vex_encoding_evex)
> -	i.vec_encoding = vex_encoding_error;
> +      if (i.encoding == encoding_default
> +	  || i.encoding == encoding_evex512)
> +	i.encoding = encoding_evex;
> +      else if (i.encoding != encoding_evex)
> +	i.encoding = encoding_error;
>      }
> 
>    if (r->reg_flags & RegRex2)
> --- a/gas/config/tc-i386-intel.c
> +++ b/gas/config/tc-i386-intel.c
> @@ -209,10 +209,10 @@ operatorT i386_operator (const char *nam
>  	      || i386_types[j].sz[0] > 8
>  	      || (i386_types[j].sz[0] & (i386_types[j].sz[0] - 1)))
>  	    return O_illegal;
> -	  if (i.vec_encoding == vex_encoding_default)
> -	    i.vec_encoding = vex_encoding_evex;
> -	  else if (i.vec_encoding != vex_encoding_evex
> -		   && i.vec_encoding != vex_encoding_evex512)
> +	  if (i.encoding == encoding_default)
> +	    i.encoding = encoding_evex;
> +	  else if (i.encoding != encoding_evex
> +		   && i.encoding != encoding_evex512)
>  	    return O_illegal;
>  	  if (!i.broadcast.bytes && !i.broadcast.type)
>  	    {


  reply	other threads:[~2024-02-18  6:00 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-16  9:56 [PATCH 0/4] x86/APX: misc adjustments Jan Beulich
2024-02-16  9:57 ` [PATCH 1/4] x86: rename vec_encoding and vex_encoding_* Jan Beulich
2024-02-18  5:59   ` Cui, Lili [this message]
2024-02-19  7:54     ` Jan Beulich
2024-02-20  9:19       ` Cui, Lili
2024-02-16  9:58 ` [PATCH 2/4] x86/APX: respect {vex}/{vex3} Jan Beulich
2024-02-18  7:55   ` Cui, Lili
2024-02-19  8:00     ` Jan Beulich
2024-02-20 10:12       ` Cui, Lili
2024-02-20 10:30         ` Jan Beulich
2024-02-20 15:59           ` Michael Matz
2024-02-20 16:52             ` H.J. Lu
2024-02-16  9:58 ` [PATCH 3/4] x86/APX: correct .insn opcode space determination when REX2 is needed Jan Beulich
2024-02-16  9:59 ` [PATCH 4/4] x86/APX: optimize certain XOR and SUB forms Jan Beulich

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=SJ0PR11MB56004CAC05FA61460BF562379E522@SJ0PR11MB5600.namprd11.prod.outlook.com \
    --to=lili.cui@intel.com \
    --cc=JBeulich@suse.com \
    --cc=binutils@sourceware.org \
    --cc=hjl.tools@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).