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

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

> Subject: Re: [PATCH V2 3/8] Support APX GPR32 with extend evex prefix
> 
> On 03.11.2023 17:50, Cui, Lili wrote:
> > --- a/gas/testsuite/gas/i386/x86-64-inval-movbe.l
> > +++ b/gas/testsuite/gas/i386/x86-64-inval-movbe.l
> > @@ -1,29 +1,30 @@
> >  .*: Assembler messages:
> > -.*:4: Error: .*
> >  .*:5: Error: .*
> >  .*:6: Error: .*
> >  .*:7: Error: .*
> >  .*:8: Error: .*
> > -.*:11: Error: .*
> > +.*:9: Error: .*
> >  .*:12: Error: .*
> >  .*:13: Error: .*
> >  .*:14: Error: .*
> >  .*:15: Error: .*
> > +.*:16: Error: .*
> >  GAS LISTING .*
> >
> >
> >  [ 	]*1[ 	]+\# Check illegal movbe in 64bit mode\.
> >  [ 	]*2[ 	]+\.text
> > -[ 	]*3[ 	]+foo:
> > -[ 	]*4[ 	]+movbe	\(%rcx\),%bl
> > -[ 	]*5[ 	]+movbe	%ecx,%ebx
> > -[ 	]*6[ 	]+movbe	%bx,%rcx
> > -[ 	]*7[ 	]+movbe	%rbx,%rcx
> > -[ 	]*8[ 	]+movbe	%bl,\(%rcx\)
> > -[ 	]*9[ 	]+
> > -[ 	]*10[ 	]+\.intel_syntax noprefix
> > -[ 	]*11[ 	]+movbe bl, byte ptr \[rcx\]
> > -[ 	]*12[ 	]+movbe ebx, ecx
> > -[ 	]*13[ 	]+movbe rcx, bx
> > -[ 	]*14[ 	]+movbe rcx, rbx
> > -[ 	]*15[ 	]+movbe byte ptr \[rcx\], bl
> > +[ 	]*3[ 	]+\.arch \.noapx_f
> > +[ 	]*4[ 	]+foo:
> > +[ 	]*5[ 	]+movbe	\(%rcx\),%bl
> > +[ 	]*6[ 	]+movbe	%ecx,%ebx
> > +[ 	]*7[ 	]+movbe	%bx,%rcx
> > +[ 	]*8[ 	]+movbe	%rbx,%rcx
> > +[ 	]*9[ 	]+movbe	%bl,\(%rcx\)
> > +[ 	]*10[ 	]+
> > +[ 	]*11[ 	]+\.intel_syntax noprefix
> > +[ 	]*12[ 	]+movbe bl, byte ptr \[rcx\]
> > +[ 	]*13[ 	]+movbe ebx, ecx
> > +[ 	]*14[ 	]+movbe rcx, bx
> > +[ 	]*15[ 	]+movbe rcx, rbx
> > +[ 	]*16[ 	]+movbe byte ptr \[rcx\], bl
> 
> To avoid the need to fiddle with this file, did you consider changing the test's
> command line options instead? In any event ...
> 

Do you mean disabling apx_f with the command line "#as..."?  I tried "#as -march=+noapx_f" , but not worked.

> > --- a/gas/testsuite/gas/i386/x86-64-inval-movbe.s
> > +++ b/gas/testsuite/gas/i386/x86-64-inval-movbe.s
> > @@ -1,5 +1,6 @@
> >  # Check illegal movbe in 64bit mode.
> >  	.text
> > +	.arch .noapx_f
> >  foo:
> >  	movbe	(%rcx),%bl
> >  	movbe	%ecx,%ebx
> 
> ... the comment here wants expanding, to (briefly) mention the deliberate
> exclusion of APX.
> 

Done. 

> > --- a/opcodes/i386-dis-evex-len.h
> > +++ b/opcodes/i386-dis-evex-len.h
> > @@ -62,6 +62,16 @@ static const struct dis386 evex_len_table[][3] = {
> >      { REG_TABLE (REG_EVEX_0F38C7_L_2) },
> >    },
> >
> > +  /* EVEX_LEN_0F38F2 */
> > +  {
> > +    { "andnS",		{ Gdq, VexGdq, Edq }, 0 },
> > +  },
> 
> There's no sign of a prefix decode step here.
> 

The prefix decoding step is in the NF patch and its dependent patches (Part II 2/6). Both are suspended currently.

> > --- a/opcodes/i386-dis-evex-mod.h
> > +++ b/opcodes/i386-dis-evex-mod.h
> > @@ -1 +1,43 @@
> >  /* Nothing at present.  */
> 
> This comment needs to go away when new stuff is added here. However, I'm
> sure I requested before that no new entries be put here which have only one
> of their two slots populated. The reg-only and mem-only aspects can be
> expressed via proper choice of operand specifiers, at least in almost all cases.
> Note how you already use ...
> 
> > +  /* MOD_EVEX_MAP4_DA_PREFIX_1 */
> > +  {
> > +    { Bad_Opcode },
> > +    { "encodekey128", { Gd, Ed }, 0 },  },
> > +  /* MOD_EVEX_MAP4_DB_PREFIX_1 */
> > +  {
> > +    { Bad_Opcode },
> > +    { "encodekey256", { Gd, Ed }, 0 },  },
> > +  /* MOD_EVEX_MAP4_DC_PREFIX_1 */
> > +  {
> > +    { "aesenc128kl",    { XM, M }, 0 },
> > +  },
> > +  /* MOD_EVEX_MAP4_DD_PREFIX_1 */
> > +  {
> > +    { "aesdec128kl",    { XM, M }, 0 },
> > +  },
> > +  /* MOD_EVEX_MAP4_DE_PREFIX_1 */
> > +  {
> > +    { "aesenc256kl",    { XM, M }, 0 },
> > +  },
> > +  /* MOD_EVEX_MAP4_DF_PREFIX_1 */
> > +  {
> > +    { "aesdec256kl",    { XM, M }, 0 },
> > +  },
> > +  /* MOD_EVEX_MAP4_F8_PREFIX_1 */
> > +  {
> > +    { "enqcmds",	{ Gva, M },  0 },
> > +  },
> > +  /* MOD_EVEX_MAP4_F8_PREFIX_2 */
> > +  {
> > +    { "movdir64b",	{ Gva, M }, 0 },
> > +  },
> > +  /* MOD_EVEX_MAP4_F8_PREFIX_3 */
> > +  {
> > +    { "enqcmd",		{ Gva, M }, 0 },
> > +  },
> 
> ... M in many entries anyway. These can move one level up without needing
> further adjustment.
> 
> For all of the above, however, an EVEX.W decode step looks to be missing.
> Interestingly the doc consistently omits the (presumably) .W0 suffix for these,
> having merely a trailing dot there. The issue (doc and/or code) is present
> elsewhere as well.
> 

Do you mean that all EVEX promoted instructions need to be explicitly .W0 in the doc? in the case of, I'll handle it uniformly in get_valid_dis386.

Thanks,
Lili.