RE: [PATCH v3 4/9] Support APX GPR32 with extend evex prefix

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

> >>>>> @@ -14233,6 +14276,12 @@ static bool check_register (const
> >>>>> reg_entry
> >>>> *r)
> >>>>>        if (!cpu_arch_flags.bitfield.cpuapx_f
> >>>>>  	  || flag_code != CODE_64BIT)
> >>>>>  	return false;
> >>>>> +
> >>>>> +      /* When using RegRex2, dual VEX/EVEX templates need to be
> >>>>> + marked as
> >>>> EVEX.
> >>>>> +	 For the later install_template function.  */
> >>>>> +      if (current_templates->start->opcode_modifier.vex
> >>>>> +	  && current_templates->start->opcode_modifier.evex)
> >>>>> +	i.vec_encoding = vex_encoding_evex;
> >>>>
> >>>> I'm afraid I don't understand the 2nd sentence of the comment. This
> >>>> may be related to my question regarding cpu_flags_match() further up.
> >>>>
> >>>> The first sentence isn't quite correct either - you don't mark any
> >>>> template here (and you can't, because we don't even know yet which
> >>>> template we're going to use).
> >>>>
> >>>> Finally - do you really need the .evex check here? (I won't exclude
> >>>> that this yields a better diagnostic in certain cases, but this
> >>>> wants clarifying if so.)
> >>>>
> >>>
> >>> If you look at install_template(), you'll see that before this
> >>> function we
> >> need to know if the current encoding is evex.
> >>
> >> "This function" being check_register()? If so, then no, we can't know
> >> up front whether EVEX encoding is going to be needed, as operand
> >> parsing happens ahead of template selection. If instead you mean
> >> "that function" and hence install_template(), then yes, we need to know
> whether to use EVEX there.
> >> Yet how does that result in a need for the .evex check here? (Or
> >> maybe your reply was really to the first of the three parts of my
> >> earlier one?)
> >>
> >
> > Agree with you, put them here is unreasonable.
> >
> > For example
> >
> > vtestps (%r27),%ymm6
> >
> > we should report unsupported  Egpr. But without .evex check, it will report
> "Error: no EVEX encoding for `vtestps'"
> >
> >> But anyway - as said earlier on, using current_templates here looks
> >> wrong in the first place. check_register() deals with only a
> >> register, without regard to the context it is used in (with the sole exception
> of allow_pseudo_reg).
> >> May I remind you that earlier on I already indicated that I suspect
> >> you'll need a new enumerator to put in i.vec_encoding for this new
> purpose?
> >>
> >
> > If we don't put it in check_register(), we need to add a for loop at the
> beginning of the install_template() to check RegRex2. Do you think it is okay?
> Or create a function for it.
> >
> > for (unsigned int op = 0; op < i.operands; op++)
> >     {
> >       if (i.types[op].bitfield.class != Reg)
> >         continue;
> >
> >       if (i.op[op].regs->reg_flags & RegRex2)
> >         i.vec_encoding = vex_encoding_evex;
> >     }
> >
> >   if ((i.index_reg && (i.index_reg->reg_flags & RegRex2))
> >       || (i.base_reg && (i.base_reg->reg_flags & RegRex2)))
> >     i.vec_encoding = vex_encoding_evex;
> 
> As a last resort this may be an option. But until my suggestion wasn't at least
> tried or demonstrated to be worse, I don't think the above would be
> acceptable.
> 

> >> May I remind you that earlier on I already indicated that I suspect
> >> you'll need a new enumerator to put in i.vec_encoding for this new
> purpose?

Jan, I didn't get your point, I think the enumerator vex_encoding_evex works well, the question is how to filter if Egpr is used in the current instruction, we should make a choice before install_template, whether it's an evex template or a vex template.

Thanks,
Lili