Home Original page

[PATCH] Support APX NF

Cui, Lili lili.cui@intel.com
Thu Feb 29 12:00:26 GMT 2024 
> 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.
More information about the Binutils mailing list