[3/6] Arm64: check tied operand specifier in aarch64-gen

  Make sure that field actually matches the specified operands. Don't
follow existing F_PSEUDO checking in using assertions, though. Print
meaingful error messages, thus - while not having a line number
available - at least providing some indication of where things are
wrong.

Fix SVE2.1's extq accordingly, but don't extend the testsuite there:
There are further issues with its operands (SVE_Zm_imm4 doesn't look to
be correct to use there, as that describes an indexed vector register,
while here a separate vector register and immediate operand are to be
specified).
  

On Fri, Feb 23, 2024 at 12:29:00PM +0100, Jan Beulich wrote:
> Make sure that field actually matches the specified operands. Don't
> follow existing F_PSEUDO checking in using assertions, though. Print
> meaingful error messages, thus - while not having a line number
> available - at least providing some indication of where things are
> wrong.

This new check should be helpful.  However, some mnemonics have a lot of
variants, so could you also add the opcode (and maybe the mask) to the new
error messages? For example:

extq (0x05602400,0xfff0fc00): operands 1 and 2 match, but tied=0

> Fix SVE2.1's extq accordingly, but don't extend the testsuite there:
> There are further issues with its operands (SVE_Zm_imm4 doesn't look to
> be correct to use there, as that describes an indexed vector register,
> while here a separate vector register and immediate operand are to be
> specified).
> 
> --- a/opcodes/aarch64-gen.c
> +++ b/opcodes/aarch64-gen.c
> @@ -129,6 +129,7 @@ read_table (const struct aarch64_opcode*
>    const struct aarch64_opcode *ent = table;
>    opcode_node **new_ent;
>    unsigned int index = initialize_index (table);
> +  unsigned int errors = 0;
>  
>    if (!ent->name)
>      return;
> @@ -140,6 +141,8 @@ read_table (const struct aarch64_opcode*
>  
>    do
>      {
> +      bool match = false;
> +
>        /* F_PSEUDO needs to be used together with F_ALIAS to indicate an alias
>  	 opcode is a programmer friendly pseudo instruction available only in
>  	 the assembly code (thus will not show up in the disassembly).  */
> @@ -150,12 +153,45 @@ read_table (const struct aarch64_opcode*
>  	  index++;
>  	  continue;
>  	}
> +
> +      /* Check tied_operand against operands[].  */
> +      for (unsigned int i = 1; i < ARRAY_SIZE (ent->operands); ++i)
> +	{
> +	  if (ent->operands[i] == AARCH64_OPND_NIL)
> +	    break;
> +
> +	  if (ent->operands[i] != ent->operands[0])
> +	    continue;
> +	  match = true;
> +
> +	  if (i != ent->tied_operand)
> +	    {
> +	      fprintf (stderr, "%s: operands 1 and %u match, but tied=%u\n",
> +		       ent->name, i + 1, ent->tied_operand);
> +	      ++errors;
> +	    }
> +	}
> +      if (!match && ent->tied_operand
> +	  /* SME LDR/STR (array vector) tie together inner immediates only.  */
> +	  && ent->iclass != sme_ldr && ent->iclass != sme_str)
> +	{
> +	  fprintf (stderr, "%s: no operands match, but tied=%u\n",
> +		   ent->name, ent->tied_operand);
> +	  ++errors;
> +	}
> +
>        *new_ent = new_opcode_node ();
>        (*new_ent)->opcode = ent->opcode;
>        (*new_ent)->mask = ent->mask;
>        (*new_ent)->index = index++;
>        new_ent = &((*new_ent)->next);
>      } while ((++ent)->name);
> +
> +  if (errors)
> +    {
> +      fprintf (stderr, "%u errors, exiting\n", errors);
> +      xexit (3);
> +    }
>  }
>  
>  static inline void
> --- a/opcodes/aarch64-tbl.h
> +++ b/opcodes/aarch64-tbl.h
> @@ -6375,7 +6375,7 @@ const struct aarch64_opcode aarch64_opco
>    SVE2p1_INSNC("fminqv",0x6417a000, 0xff3fe000, sve2_urqvs, 0, OP3 (Vd, SVE_Pg3, SVE_Zn), OP_SVE_vUS_HSD_HSD, F_OPD_SIZE, C_SCAN_MOVPRFX, 0),
>  
>    SVE2p1_INSN("dupq",0x05202400, 0xffe0fc00, sve_index1, 0, OP2 (SVE_Zd, SVE_Zn_5_INDEX), OP_SVE_VV_BHSD, 0, 0),
> -  SVE2p1_INSN("extq",0x05602400, 0xfff0fc00, sve_misc, 0, OP3 (SVE_Zd, SVE_Zd, SVE_Zm_imm4), OP_SVE_BBB, 0, 0),
> +  SVE2p1_INSN("extq",0x05602400, 0xfff0fc00, sve_misc, 0, OP3 (SVE_Zd, SVE_Zd, SVE_Zm_imm4), OP_SVE_BBB, 0, 1),
>    SVE2p1_INSNC("ld1q",0xc400a000, 0xffe0e000, sve_misc, 0, OP3 (SVE_Zt, SVE_Pg3, SVE_ADDR_ZX), OP_SVE_SZS_QD, 0, C_SCAN_MOVPRFX, 0),
>    SVE2p1_INSNC("ld2q",0xa490e000, 0xfff0e000, sve_misc, 0, OP3 (SME_Zt2, SVE_Pg3, SVE_ADDR_RI_S4x2xVL), OP_SVE_QZU, 0, C_SCAN_MOVPRFX, 0),
>    SVE2p1_INSNC("ld3q",0xa510e000, 0xfff0e000, sve_misc, 0, OP3 (SME_Zt3, SVE_Pg3, SVE_ADDR_RI_S4x2xVL), OP_SVE_QZU, 0, C_SCAN_MOVPRFX, 0),
>
  

On 15.03.2024 17:09, Andrew Carlotti wrote:
> On Fri, Feb 23, 2024 at 12:29:00PM +0100, Jan Beulich wrote:
>> Make sure that field actually matches the specified operands. Don't
>> follow existing F_PSEUDO checking in using assertions, though. Print
>> meaingful error messages, thus - while not having a line number
>> available - at least providing some indication of where things are
>> wrong.
> 
> This new check should be helpful.  However, some mnemonics have a lot of
> variants, so could you also add the opcode (and maybe the mask) to the new
> error messages? For example:
> 
> extq (0x05602400,0xfff0fc00): operands 1 and 2 match, but tied=0

Hmm, yes, I could do that for disambiguation; not sure if the mask value
really is needed there - I fear overloading the message. What I was
instead hoping for though is an idea how to report the line number. Imo
that would be far better, just that the way the header file is used
doesn't easily lend itself to doing so.

Jan
  

On 23/02/2024 11:29, Jan Beulich wrote:
> Make sure that field actually matches the specified operands. Don't
> follow existing F_PSEUDO checking in using assertions, though. Print
> meaingful error messages, thus - while not having a line number
> available - at least providing some indication of where things are
> wrong.
> 
> Fix SVE2.1's extq accordingly, but don't extend the testsuite there:
> There are further issues with its operands (SVE_Zm_imm4 doesn't look to
> be correct to use there, as that describes an indexed vector register,
> while here a separate vector register and immediate operand are to be
> specified).
> 
> --- a/opcodes/aarch64-gen.c
> +++ b/opcodes/aarch64-gen.c
> @@ -129,6 +129,7 @@ read_table (const struct aarch64_opcode*
>    const struct aarch64_opcode *ent = table;
>    opcode_node **new_ent;
>    unsigned int index = initialize_index (table);
> +  unsigned int errors = 0;
>  
>    if (!ent->name)
>      return;
> @@ -140,6 +141,8 @@ read_table (const struct aarch64_opcode*
>  
>    do
>      {
> +      bool match = false;
> +
>        /* F_PSEUDO needs to be used together with F_ALIAS to indicate an alias
>  	 opcode is a programmer friendly pseudo instruction available only in
>  	 the assembly code (thus will not show up in the disassembly).  */
> @@ -150,12 +153,45 @@ read_table (const struct aarch64_opcode*
>  	  index++;
>  	  continue;
>  	}
> +
> +      /* Check tied_operand against operands[].  */
> +      for (unsigned int i = 1; i < ARRAY_SIZE (ent->operands); ++i)
> +	{
> +	  if (ent->operands[i] == AARCH64_OPND_NIL)
> +	    break;
> +
> +	  if (ent->operands[i] != ent->operands[0])
> +	    continue;
> +	  match = true;
> +
> +	  if (i != ent->tied_operand)
> +	    {
> +	      fprintf (stderr, "%s: operands 1 and %u match, but tied=%u\n",
> +		       ent->name, i + 1, ent->tied_operand);
> +	      ++errors;
> +	    }

I'm not sure I follow this.  It looks like you're testing that if one operand is tied to operand 0, then no other operand may overlap that, eg that

	extq z3.b, z3.b, z3.b, #5

is an illegal instruction.  But I don't see anything in the instruction description that prohibits that.  While it may not be sensible, it's not obvious to me that it's prohibited.

   
> +	}
> +      if (!match && ent->tied_operand
> +	  /* SME LDR/STR (array vector) tie together inner immediates only.  */
> +	  && ent->iclass != sme_ldr && ent->iclass != sme_str)
> +	{
> +	  fprintf (stderr, "%s: no operands match, but tied=%u\n",
> +		   ent->name, ent->tied_operand);
> +	  ++errors;
> +	}
> +
>        *new_ent = new_opcode_node ();
>        (*new_ent)->opcode = ent->opcode;
>        (*new_ent)->mask = ent->mask;
>        (*new_ent)->index = index++;
>        new_ent = &((*new_ent)->next);
>      } while ((++ent)->name);
> +
> +  if (errors)
> +    {
> +      fprintf (stderr, "%u errors, exiting\n", errors);
> +      xexit (3);
> +    }
>  }
>  
>  static inline void
> --- a/opcodes/aarch64-tbl.h
> +++ b/opcodes/aarch64-tbl.h
> @@ -6375,7 +6375,7 @@ const struct aarch64_opcode aarch64_opco
>    SVE2p1_INSNC("fminqv",0x6417a000, 0xff3fe000, sve2_urqvs, 0, OP3 (Vd, SVE_Pg3, SVE_Zn), OP_SVE_vUS_HSD_HSD, F_OPD_SIZE, C_SCAN_MOVPRFX, 0),
>  
>    SVE2p1_INSN("dupq",0x05202400, 0xffe0fc00, sve_index1, 0, OP2 (SVE_Zd, SVE_Zn_5_INDEX), OP_SVE_VV_BHSD, 0, 0),
> -  SVE2p1_INSN("extq",0x05602400, 0xfff0fc00, sve_misc, 0, OP3 (SVE_Zd, SVE_Zd, SVE_Zm_imm4), OP_SVE_BBB, 0, 0),
> +  SVE2p1_INSN("extq",0x05602400, 0xfff0fc00, sve_misc, 0, OP3 (SVE_Zd, SVE_Zd, SVE_Zm_imm4), OP_SVE_BBB, 0, 1),
>    SVE2p1_INSNC("ld1q",0xc400a000, 0xffe0e000, sve_misc, 0, OP3 (SVE_Zt, SVE_Pg3, SVE_ADDR_ZX), OP_SVE_SZS_QD, 0, C_SCAN_MOVPRFX, 0),
>    SVE2p1_INSNC("ld2q",0xa490e000, 0xfff0e000, sve_misc, 0, OP3 (SME_Zt2, SVE_Pg3, SVE_ADDR_RI_S4x2xVL), OP_SVE_QZU, 0, C_SCAN_MOVPRFX, 0),
>    SVE2p1_INSNC("ld3q",0xa510e000, 0xfff0e000, sve_misc, 0, OP3 (SME_Zt3, SVE_Pg3, SVE_ADDR_RI_S4x2xVL), OP_SVE_QZU, 0, C_SCAN_MOVPRFX, 0),
> 

R.
  

On 20.03.2024 17:51, Richard Earnshaw (lists) wrote:
> On 23/02/2024 11:29, Jan Beulich wrote:
>> Make sure that field actually matches the specified operands. Don't
>> follow existing F_PSEUDO checking in using assertions, though. Print
>> meaingful error messages, thus - while not having a line number
>> available - at least providing some indication of where things are
>> wrong.
>>
>> Fix SVE2.1's extq accordingly, but don't extend the testsuite there:
>> There are further issues with its operands (SVE_Zm_imm4 doesn't look to
>> be correct to use there, as that describes an indexed vector register,
>> while here a separate vector register and immediate operand are to be
>> specified).
>>
>> --- a/opcodes/aarch64-gen.c
>> +++ b/opcodes/aarch64-gen.c
>> @@ -129,6 +129,7 @@ read_table (const struct aarch64_opcode*
>>    const struct aarch64_opcode *ent = table;
>>    opcode_node **new_ent;
>>    unsigned int index = initialize_index (table);
>> +  unsigned int errors = 0;
>>  
>>    if (!ent->name)
>>      return;
>> @@ -140,6 +141,8 @@ read_table (const struct aarch64_opcode*
>>  
>>    do
>>      {
>> +      bool match = false;
>> +
>>        /* F_PSEUDO needs to be used together with F_ALIAS to indicate an alias
>>  	 opcode is a programmer friendly pseudo instruction available only in
>>  	 the assembly code (thus will not show up in the disassembly).  */
>> @@ -150,12 +153,45 @@ read_table (const struct aarch64_opcode*
>>  	  index++;
>>  	  continue;
>>  	}
>> +
>> +      /* Check tied_operand against operands[].  */
>> +      for (unsigned int i = 1; i < ARRAY_SIZE (ent->operands); ++i)
>> +	{
>> +	  if (ent->operands[i] == AARCH64_OPND_NIL)
>> +	    break;
>> +
>> +	  if (ent->operands[i] != ent->operands[0])
>> +	    continue;
>> +	  match = true;
>> +
>> +	  if (i != ent->tied_operand)
>> +	    {
>> +	      fprintf (stderr, "%s: operands 1 and %u match, but tied=%u\n",
>> +		       ent->name, i + 1, ent->tied_operand);
>> +	      ++errors;
>> +	    }
> 
> I'm not sure I follow this.  It looks like you're testing that if one operand is tied to operand 0, then no other operand may overlap that, eg that
> 
> 	extq z3.b, z3.b, z3.b, #5
> 
> is an illegal instruction.  But I don't see anything in the instruction description that prohibits that.  While it may not be sensible, it's not obvious to me that it's prohibited.

No, that's no what is being tested. Here we're looking at operand types, i.e.
for extq with

SVE2p1_INSN("extq",0x05602400, 0xfff0fc00, sve_misc, 0, OP3 (SVE_Zd, SVE_Zd, SVE_Zm_imm4), OP_SVE_BBB, 0, 1),

the type of the first two operands is the same, while that of the 3rd is
different. Actual register types / numbers don't come into play here.

Jan