Re: [PATCH v2 1/6] target/riscv: Add properties for BF16 extensions

[Rob Bradford]

On Thu, 2023-06-15 at 21:14 +0800, Weiwei Li wrote:
> 
> On 2023/6/15 20:58, Rob Bradford wrote:
> > On Thu, 2023-06-15 at 14:32 +0800, Weiwei Li wrote:
> > > Add ext_zfbfmin/zvfbfmin/zvfbfwma properties.
> > > Add require check for BF16 extensions.
> > > 
> > > Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> > > Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> > > Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > > ---
> > >   target/riscv/cpu.c     | 20 ++++++++++++++++++++
> > >   target/riscv/cpu_cfg.h |  3 +++
> > >   2 files changed, 23 insertions(+)
> > > 
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index 881bddf393..dc6b2f72f6 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -1059,6 +1059,11 @@ void
> > > riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> > >           return;
> > >       }
> > >   
> > > +    if (cpu->cfg.ext_zfbfmin && !riscv_has_ext(env, RVF)) {
> > > +        error_setg(errp, "Zfbfmin extension depends on F
> > > extension");
> > > +        return;
> > > +    }
> > > +
> > >       if (riscv_has_ext(env, RVD) && !riscv_has_ext(env, RVF)) {
> > >           error_setg(errp, "D extension requires F extension");
> > >           return;
> > > @@ -1109,6 +1114,21 @@ void
> > > riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> > >           return;
> > >       }
> > >   
> > > +    if (cpu->cfg.ext_zvfbfmin && !cpu->cfg.ext_zfbfmin) {
> > > +        error_setg(errp, "Zvfbfmin extension depends on Zfbfmin
> > > extension");
> > > +        return;
> > > +    }
> > > +
> > > +    if (cpu->cfg.ext_zvfbfmin && !cpu->cfg.ext_zve32f) {
> > > +        error_setg(errp, "Zvfbfmin extension depends on Zve32f
> > > extension");
> > > +        return;
> > > +    }
> > I don't think this is correct - from the spec:
> > 
> > "This extension [Zvfbfmin] depends on the Zfbfmin extension and
> > either
> > the "V" extension or the Zve32f embedded vector extension."
> > 
> > So this should be:
> > 
> > +    if (cpu->cfg.ext_zvfbfmin && !(cpu->cfg.ext_zve32f || cpu-
> > > cfg.ext_v) {
> > +        error_setg(errp, "Zvfbfmin extension depends on Zve32f or
> > V
> > extension");
> > +        return;
> > +    }
> 
> Zve32f will be enabled when V is enabled. So we can simply check
> Zve32f 
> here.

Great, thank you for the clarification - I missed that this this
enforced directly above.

Cheers,

Rob

> 
> Regards,
> 
> Weiwei Li
> 
> > Cheers,
> > 
> > Rob
> > 
> > > +
> > > +    if (cpu->cfg.ext_zvfbfwma && !cpu->cfg.ext_zvfbfmin) {
> > > +        error_setg(errp, "Zvfbfwma extension depends on Zvfbfmin
> > > extension");
> > > +        return;
> > > +    }
> > > +
> > >       /* Set the ISA extensions, checks should have happened
> > > above */
> > >       if (cpu->cfg.ext_zhinx) {
> > >           cpu->cfg.ext_zhinxmin = true;
> > > diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> > > index c4a627d335..7d16f32720 100644
> > > --- a/target/riscv/cpu_cfg.h
> > > +++ b/target/riscv/cpu_cfg.h
> > > @@ -75,6 +75,7 @@ struct RISCVCPUConfig {
> > >       bool ext_svpbmt;
> > >       bool ext_zdinx;
> > >       bool ext_zawrs;
> > > +    bool ext_zfbfmin;
> > >       bool ext_zfh;
> > >       bool ext_zfhmin;
> > >       bool ext_zfinx;
> > > @@ -84,6 +85,8 @@ struct RISCVCPUConfig {
> > >       bool ext_zve64f;
> > >       bool ext_zve64d;
> > >       bool ext_zmmul;
> > > +    bool ext_zvfbfmin;
> > > +    bool ext_zvfbfwma;
> > >       bool ext_zvfh;
> > >       bool ext_zvfhmin;
> > >       bool ext_smaia;
>