RE: [PATCH 2/8] Support APX GPR32 with extend evex prefix

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

> Without seeing how this is used I can't comment on it. It feels though as if you
> may not have fully understood my earlier reply: Even prior to APX we already
> have cases where one CPU specifier in the opcode table using | means "both"
> and another means "either". I think we want to split that, and thus simplify
> the logic in cpu_flags_match(). That's separate prereq work, of course.
> 
> Thing is that prior to 734dfd1cc966 this would have been prohibitively
> expensive in terms of table size growth. But now we can afford having two
> i386_cpu_attr fields, one meaning "all of these", the other meaning "any of
> these". To limit churn in the opcode table, I'd be inclined to continue to
> express CPU requirements in a single field there, using e.g.
> (CpuA|CpuB)&CpuC&CpuD (Cpu prefixes re-added here for clarity, even if
> they aren't present in the opcode table anymore).
> 
> I'd be happy to do that prereq work (if we can agree on the approach), but it
> may mean a little bit of delay, as after my vacation I need to catch up with a
> few other thinghs first.
> 

It would be great if you could do this (agree with the approach), it is not hurry, it may take a long time to commit the APX  to the master.

> >>> +  if (i.prefix[DATA_PREFIX] != 0)
> >
> >>> +    {
> >
> >>> +      i.tm.opcode_modifier.opcodeprefix = PREFIX_0X66;
> >
> >>> +      i.prefix[DATA_PREFIX] = 0;
> >
> >>> +    }
> >
> >>
> >
> >> While this looks to be correct for the case when the prefix was
> >> derived from an
> >
> >> insn template and the use of 16-bit operands, I don't think it is
> >> uniformly
> >
> >> correct when "data16" was used as a prefix explicitly. In such a case
> >> either
> >
> >> REX2 encoding needs to be used, or an error needs emitting.
> >
> >>
> >
> >> You may further want to assert that i.tm.opcode_modifier.opcodeprefix
> >> is still
> >
> >> zero ahead of the assignment.
> >
> >>
> >
> >
> >
> > For REX2 encoding, we add no special handling, just follow REX.
> >
> > For EVEX-promoted encoding, such as “data16
> aand   %r25d,0x123(%r31,%rax,4)”, the following existing code will report an
> error.
> >
> >
> >
> >   if (is_any_vex_encoding (&i.tm)
> >
> >       || i.tm.operand_types[i.imm_operands].bitfield.class >= RegMMX
> >
> >       || i.tm.operand_types[i.imm_operands + 1].bitfield.class >=
> > RegMMX)
> >
> >     {
> >
> >       /* Check for data size prefix on VEX/XOP/EVEX encoded and SIMD
> > insns.  */
> >
> >       if (i.prefix[DATA_PREFIX])
> >
> >         {
> >
> >           as_bad (_("data size prefix invalid with `%s'"), insn_name
> > (&i.tm));
> >
> >           return;
> >
> >         }
> 
> Thinking of it, I may need to revise my earlier comment some: RAO-INT insns
> are a bad example here, since despite being legacy-encoded they don't permit
> a data16 prefix to specify 16-bit operand size. Consider the same for e.g.
> AND. The legacy form permits use of data16 (leaving aside that it's better /
> clearer to simply use 16-bit register names), so the promoted forms likely
> ought to permit such as well. IOW perhaps the code you have is correct, but
> the check you quote may need adjusting.
> 

I listed 5 instructions to check the data16 prefix, is the last one what you want?

$ cat add.s

and %ebx, %eax
data16 and %ebx, %eax
and %ebx, %r16d
data16 and %ebx, %r16d
{evex} data16 and %ebx, %r16d

$ objdump -dw  and.o

   0:   21 d8                   and    %ebx,%eax
   2:   66 21 d8                and    %bx,%ax
   5:   d5 10 21 d8             and    %ebx,%r16d
   9:   66 d5 10 21 d8          and    %bx,%r16w
   Error: data size prefix invalid with `and'

> >
> > I rebased master and as you predicted this doesn't work, one entry
> > contains both VEX and EVEX, VEX requires No_egpr=1 and EVEX requires
> > No_egpr=0,
> >
> > Finally I chose to add "No_egpr=1" for it. And added the following judgment
> in check_EgprOperands.
> >
> >
> >
> > check_EgprOperands (const insn_template *t)
> >
> > {
> >
> > -  if (t->opcode_modifier.noegpr)
> >
> > if (t->opcode_modifier.noegpr && !need_evex_encoding())
> 
> I'll need to look at this in context, so I can't comment right here.
> 
> Also, just to mention it: Something's wrong with your reply (also visible in the
> list archive copy), harming readability quite a bit. There are extra blank lines
> between any two real ones.
> 
Oh, sorry, I tried inserting a table into the email and changed it to HTML format, but forgot to change it back.

Thanks,
Lili.