RE: [PATCH 4/8] Support APX NDD

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

> Subject: Re: [PATCH 4/8] Support APX NDD
> 
> On 22.10.2023 16:05, Cui, Lili wrote:
> >>> @@ -8825,6 +8853,9 @@ build_modrm_byte (void)
> >>>        break;
> >>>    if (v >= dest)
> >>>      v = ~0;
> >>> +  if (i.tm.opcode_space == SPACE_EVEXMAP4
> >>> +      && i.tm.opcode_modifier.vexvvvv)
> >>> +    v = dest;
> >>>    if (i.tm.extension_opcode != None)
> >>>      {
> >>>        if (dest != source)
> >>> @@ -9088,6 +9119,9 @@ build_modrm_byte (void)
> >>>        set_rex_vrex (i.op[op].regs, REX_B, false);
> >>>  	}
> >>>
> >>> +      if (i.tm.opcode_space == SPACE_EVEXMAP4
> >>> +	  && i.tm.opcode_modifier.vexvvvv)
> >>> +	dest--;
> >>>        if (op == dest)
> >>>  	dest = ~0;
> >>>        if (op == source)
> >>
> >> These two changes are at the very least problematic with .insn, whose
> >> behavior may not change. I'd also prefer if we could get away with
> >> just one change to the function. Did you consider alternatives? We
> >> could re- widen VexVVVV, such that the value 2 indicates that the
> destination is encoded there.
> >> That then also has no chance of conflicting with .insn.
> >>
> > I added value 2 for NDD, if it's ok, I will create another patch to move
> (i.tm.extension_opcode != None) to VexVVVVDEST branch, and use value 3
> instead of SWAP_SOURCES, maybe name it VexVVVVSRC1, or just VexVVVVOP1,
> VexVVVVOP2 and VexVVVVOP3?
> 
> Hard to tell without actually seeing the code. What you did quote below used
> just SRC/DEST.
> 

Ok. Let review the current changes first.

> >   /* How to encode VEX.vvvv:
> >      0: VEX.vvvv must be 1111b.
> >      1: VEX.vvvv encodes one of the register operands.
> >      2: VEX.vvvv encodes as the dest register operands.
> >    */
> > #define VexVVVVSRC   1
> > #define VexVVVVDEST  2
> >   VexVVVV,
> 
> For readability maybe insert underscores (and if SRC remains un-numbered,
> maybe also DST in place of DEST)?

Done.

> 
> >>> --- /dev/null
> >>> +++ b/gas/testsuite/gas/i386/x86-64-apx-ndd.s
> >>> @@ -0,0 +1,156 @@
> >>> +# Check 64bit APX NDD instructions with evex prefix encoding
> >>> +
> >>> +	.allow_index_reg
> >>> +	.text
> >>> +_start:
> >>> +cmovge 0x90909090(%eax),%edx,%r8d
> >>> +cmovle 0x90909090(%eax),%edx,%r8d
> >>> +cmovg  0x90909090(%eax),%edx,%r8d
> >>> +imul   0x90909(%eax),%edx,%r8d
> >>> +imul   0x909(%rax,%r31,8),%rdx,%r25
> >>
> >> What about imul by immediate? The present spec is quite unclear there:
> >> The insn page says {ND=ZU} and the table says 0/1 in the ND column.
> >>
> >
> > We don't support it yet, I put it in RFC.
> > ...
> > 2. Support APX ZU   -- In progress
> > 3. Support APX CCMP and CTEST -- In progress ...
> >
> > About 0/1 in the ND column, it means ZU can be 0/1.
> >
> > IMUL with opcodes 0x69 and 0x6B in map 0 and SETcc instructions
> > Although these instructions do not support NDD, the EVEX.ND bit is
> > used to control whether its destination register has its upper bits (namely,
> bits [63:OSIZE]) zeroed when OSIZE is 8b or 16b.
> > That is, if EVEX.ND = 1, the upper bits are always zeroed; otherwise,
> > they keep the old values when OSIZE is 8b or 16b. For these instructions,
> EVEX.[V4,V3,V2,V1,V0] must be all zero.
> 
> So ZU indeed isn't just a typo there. For 32- and 64-bit forms, is EVEX.ND then
> simply being ignored? The ZU really is meaningful only for 16-bit forms, aiui ...
> 

EVEX.ZU should be ignored for 32-bit and 64-bit forms. For imul (in spec 6.30 IMUL), EVEX.ND stands for ND or ZU. I think ZU makes sense for both the 16-bit form (imul) and the 8-bit form (setcc, I'm not sure if imul supports it yet).

> >>> +.byte 0x62,0xf4,0xfc,0x08,0xff,0xc0  #inc %rax .byte
> >>> +0x62,0xf4,0xec,0x08,0xff,0xc0  #bad
> >>
> >> As before, please avoid .byte whenever possible. And please have a
> >> more detailed comment as to what is being encoded, when .byte cannot be
> avoided.
> >> Plus, if at all possible, have "bad" tests live in separate testcases from
> "good"
> >> ones.
> >>
> >
> > This case wants to test that inc supports evex format without GPR32,  patch
> part II 1/6 will cover it. The first case has been removed and the second case
> has been added to x86-64-apx-evex-promoted-bad.s.
> 
> That's all fine, but still wants expressing with .insn rather than .byte, if at all
> possible.
> 

Got your point, for the first case we can use .insn instead, for the second case need .byte. 

Thanks,
Lili.