RE: [PATCH] Support APX NF

["Cui, Lili" <lili.cui@intel.com>]

> On 27.02.2024 10:01, Cui, Lili wrote:
> > @@ -415,6 +416,9 @@ struct _i386_insn
> >      /* Compressed disp8*N attribute.  */
> >      unsigned int memshift;
> >
> > +    /* No CSPAZO flags update.  */
> > +    bool has_nf;
> > +
> >      /* Prefer load or store in encoding.  */
> >      enum
> >        {
> 
> There's a group of booleans further up and another one further down. Is there
> any reason not to leverage an available padding slot there?
> 

it is better to put it together with has_egpr.

> > @@ -6627,6 +6635,9 @@ md_assemble (char *line)
> >  	case unsupported_EGPR_for_addressing:
> >  	  err_msg = _("extended GPR cannot be used as base/index");
> >  	  break;
> > +	case unsupported_nf:
> > +	  err_msg = _("unsupported NF");
> > +	  break;
> 
> No tests showing this new error message in action? I'm once again a little
> worried about the resulting overall wording of the diagnostic.

 I will add invalid test cases for the instructions that don't support NF.

> 
> > @@ -7187,6 +7198,10 @@ parse_insn (const char *line, char *mnemonic,
> bool prefix_only)
> >  		  /* {rex2} */
> >  		  i.rex2_encoding = true;
> >  		  break;
> > +		case Prefix_NF:
> > +		  /* {NF} */
> > +		  i.has_nf = true;
> > +		  break;
> >  		case Prefix_NoOptimize:
> >  		  /* {nooptimize} */
> >  		  i.no_optimize = true;
> 
> Nit: Preferably {nf} in the comment, matching comments in context.
> 

Ok.

> > @@ -8860,6 +8880,9 @@ match_template (char mnem_suffix)
> >  		  goto check_operands_345;
> >  		}
> >  	      else if (t->opcode_space != SPACE_BASE
> > +		       /* Map0 and map1 are promoted to MAP4 when NF is
> enabled.
> > +			*/
> > +		       && !t->opcode_modifier.nf
> >  		       && (t->opcode_space != SPACE_0F
> >  			   /* MOV to/from CR/DR/TR, as an exception, follow
> >  			      the base opcode space encoding model.  */
> 
> I don't understand this: How does a template permitting NF matter here?
> I could see the immediately preceding "else if" become something along the
> lines of
> 
> 	      else if (is_cpu (t, CpuAPX_F) && (i.operands == 3 || i.has_nf))
> 
> But I admit I didn't fully think this through. It's just that the change as is looks
> wrong to me.
> 

I was also dissatisfied with this place yesterday and then modified it to:

              else if (t->opcode_space != SPACE_BASE
                       /* For EVEX-promoted instructions, opcode_space is
                          promoted to MAP4.  */
                       && (t->opcode_space != SPACE_EVEXMAP4
                           || t->mnem_off == MN_movbe)
                       && (t->opcode_space != SPACE_0F
                           /* MOV to/from CR/DR/TR, as an exception, follow
                              the base opcode space encoding model.  */
                           || (t->base_opcode | 7) != 0x27))

For EVEX-promoted instructions, opcode_space is promoted to MAP4. The old judgment no longer fit for EVEX promoted instructions. However, the logic of this place is still not good.

Thanks,
Lili.