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/config/tc-i386.c
> > +++ b/gas/config/tc-i386.c
> > @@ -3672,9 +3672,10 @@ install_template (const insn_template *t)
> 
> What I'm surprised by is that you don't have any change to
> cpu_flags_match().
> I don't think you can get away without for dual VEX/EVEX templates. I hope
> further down you'll find a sufficient explanation of what I think is going to be
> needed.
> 

I think it is ok. Or am I missing something?

 $ cat a.s
       .allow_index_reg
        .text
_start:
        .text
        .arch .noapx_f
ldtilecfg  (%r31,%rdx,2)
        .arch .noamx_tile
ldtilecfg  (%rax,%rdx,2)

$ as --64 a.s -o a.o
a.s: Assembler messages:
a.s:9: Error: `ldtilecfg' is not supported on `x86_64.noapx_f'
a.s:11: Error: `ldtilecfg' is not supported on `x86_64.noapx_f.noamx_tile'

> > @@ -1834,13 +1844,21 @@ xtest, 0xf01d6, HLE|RTM, NoSuf, {}  // BMI2
> > instructions.
> >
> >  bzhi, 0xf5, BMI2,
> >
> Modrm|CheckOperandSize|Vex128|Space0F38|VexVVVV|SwapSources|No_
> bSuf|No
> > _wSuf|No_sSuf, { Reg32|Reg64, Reg32|Reg64|Unspecified|BaseIndex,
> > Reg32|Reg64 }
> > +bzhi, 0xf5, BMI2&APX_F_64,
> >
> +Modrm|CheckOperandSize|EVex128|Space0F38|VexVVVV|SwapSources|N
> o_bSuf|
> > +No_wSuf|No_sSuf, { Reg32|Reg64, Reg32|Reg64|Unspecified|BaseIndex,
> > +Reg32|Reg64 }
> 
> With the now two prereq patches sent a few minutes ago, this wants to follow
> what you have ...
> 

I changed them locally and it worked. I think the prereq patches are almost done, I'll tweak the apx patches after you check them in.

> > @@ -3126,8 +3170,8 @@ xresldtrk, 0xf20f01e9, TSXLDTRK, NoSuf, {}
> >
> >  // AMX instructions.
> >
> > -ldtilecfg, 0x49/0, AMX_TILE&x64,
> Modrm|Vex128|Space0F38|VexW0|NoSuf,
> > { Unspecified|BaseIndex } -sttilecfg, 0x6649/0, AMX_TILE&x64,
> > Modrm|Vex128|Space0F38|VexW0|NoSuf, { Unspecified|BaseIndex }
> > +ldtilecfg, 0x49/0, AMX_TILE&x64&(AMX_TILE|APX_F),
> > +Modrm|Vex128|EVex128|Space0F38|VexW0|NoSuf,
> { Unspecified|BaseIndex }
> > +sttilecfg, 0x6649/0, AMX_TILE&x64&(AMX_TILE|APX_F),
> > +Modrm|Vex128|EVex128|Space0F38|VexW0|NoSuf,
> { Unspecified|BaseIndex }
> 
> ... here, simplified to <feat>&(<feat>|APF_F). I would strongly recommend
> putting this in a macro, accompanied by a clarifying comment (not the least
> because (a && (a || b)) == a):
> 
> /* Many APX_F instructions require a 2nd feature to also be available.  When
>    the respective instructions were originally VEX-encoded, a single template
>    can cover both encodings.  While the expression below may look odd on
> the
>    surface (first of all we really mean "feat || (feat && APX_F)", but that is
>    nothing else than just "feat", and in turn the same as the expression below,
>    which is what i386-gen can grok), gas/config/tc-i386.c:cpu_flags_match()
>    will zap either of the inner two features depending on whether EVEX
>    encoding was determined to be necessary.  Only then will the actual feature
>    checking be carried out.  */
> #define APX_F(feat) feat&(feat|APF_F)
> 
Ok , I'll put them in opcodes/i386-opc.tbl.

> Since the list of bits we want to clear in cpu_flags_match()'s "any" is going to
> grow, we may want to consider using cpu_flags_and_not() there as well. But
> that can likely be a follow-on cleanup patch (if deemed desirable in the first
> place), so I would recommend not worrying about that right away.
> 
Sure.