RE: [PATCH V2] Support APX NF

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

> >>>> On 04.03.2024 09:15, Cui, Lili wrote:
> >>>>> @@ -7218,6 +7229,11 @@ parse_insn (const char *line, char
> >> *mnemonic,
> >>>> bool prefix_only)
> >>>>>  		  /* {rex2} */
> >>>>>  		  i.rex2_encoding = true;
> >>>>>  		  break;
> >>>>> +		case Prefix_NF:
> >>>>> +		  /* {nf} */
> >>>>> +		  i.has_nf = true;
> >>>>> +		  i.encoding = encoding_evex;
> >>>>> +		  break;
> >>>>
> >>>> It's not quite as easy, I'm afraid: Have you thought of the "{vex} {nf} ..."
> >>>> case? (I think I previously indicated that their combination,
> >>>> actually in either order, needs properly rejecting.) Without having
> >>>> spent much thought on it, perhaps it would suffice to check here
> >>>> that the field is still encoding_default, and leave the value alone
> >>>> otherwise (in order to reject bad combinations elsewhere).
> >>>>
> >>> Oh, good point. Do you think it's ok to report bad outside of "switch"?
> >>>
> >>>                 case Prefix_NF:
> >>>                   /* {nf} */
> >>>                   i.has_nf = true;
> >>> -                 i.encoding = encoding_evex;
> >>> +                 if (i.encoding == encoding_default)
> >>> +                   i.encoding = encoding_evex;
> >>>                   break;
> >>>                 case Prefix_NoOptimize:
> >>>                   /* {nooptimize} */
> >>>                   i.no_optimize = true;
> >>>                   break;
> >>>                 default:
> >>>                   abort ();
> >>>                 }
> >>> +             if (i.has_nf && i.encoding != encoding_evex)
> >>> +               {
> >>> +                 as_bad (_("{nf} cannot be combined with {vex}/{vex3}"));
> >>> +                 return NULL;
> >>> +               }
> >>
> >> Where the checking is done is secondary to me. My primary point is
> >> that the above isn't correct: There are more encoding variants than
> >> just vex, vex3, and evex.
> >>
> >> That said, reporting the error right here may lead to multiple
> >> identical errors being reported for a single line of input. This
> >> wants avoiding, at which point placing the check here is ruled out.
> >>
> >
> > Sorry,  I didn’t get you here, do you think the current implementation
> > is wrong? I think for parse_insn stage i.encodin only has those
> > encoding variants,
> 
> Hmm, you're right - operands weren't parsed yet at this point. While ...
> 
> > do you think we need to move further back? Maybe put them together with
> REX/REX2 prefix checking?
> >
> >       /* Check for explicit REX prefix.  */
> >       if (i.prefix[REX_PREFIX] || i.rex_encoding)
> >         {
> >           as_bad (_("REX prefix invalid with `%s'"), insn_name (&i.tm));
> >           return;
> >         }
> >
> >       /* Check for explicit REX2 prefix.  */
> >       if (i.rex2_encoding)
> >         {
> >           as_bad (_("{rex2} prefix invalid with `%s'"), insn_name (&i.tm));
> >           return;
> >         }
> 
> ... this would overall seem to be a more logical place to have such checks, I'd
> like to take back my earlier comment then: By having the check in
> parse_insn(), .insn handling is then also covered right away.
> 
> Jan

Ok.

Thanks,
Lili.