| Message ID | 20230214083833.44205-6-liweiwei@iscas.ac.cn |
|---|---|
| State | New |
| Headers | show |
| Series | target/riscv: Some updates to float point related extensions | expand |
On 2/14/23 05:38, Weiwei Li wrote: > Add dependence chain: > * V => Zve64d => Zve64f => Zve32f => F > * V => Zve64d => D > > Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> > Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> > --- > target/riscv/cpu.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index 9a89bea2a3..4797ef9c42 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -743,12 +743,27 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp) > return; > } > > - if (cpu->cfg.ext_v && !cpu->cfg.ext_d) { > - error_setg(errp, "V extension requires D extension"); > + /* The V vector extension depends on the Zve64d extension */ > + if (cpu->cfg.ext_v) { > + cpu->cfg.ext_zve64d = true; > + } > + > + /* The Zve64d extension depends on the Zve64f extension */ > + if (cpu->cfg.ext_zve64d) { > + cpu->cfg.ext_zve64f = true; > + } > + > + /* The Zve64f extension depends on the Zve32f extension */ > + if (cpu->cfg.ext_zve64f) { > + cpu->cfg.ext_zve32f = true; > + } > + > + if (cpu->cfg.ext_zve64d && !cpu->cfg.ext_d) { > + error_setg(errp, "Zve64d extensions require D extension"); > return; I'll be honest and confess that I wrote a short essay about the problems I have with this code. I gave up because in the end it's all stuff that we've been doing for a long time in riscv_cpu_validate_set_extensions(). I'll see if I can work in a redesign of that function and in how we're setting extensions automatically without checking user input and so on. For now I'll say that this error message seems weird because Zve64d was set to true without user input. So this ends up happening: $ ./qemu-system-riscv64 -M virt -cpu rv64,v=true,d=false qemu-system-riscv64: Zve64d extensions require D extension It's weird because the user didn't enabled Zve64d but the error message is complaining about it. Given that the root cause is that ext_v was set, and then we've set other extensions under the hood, a saner error message in this case would be "V extension requires D extension". Thanks, Daniel > } > > - if ((cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) && !cpu->cfg.ext_f) { > + if (cpu->cfg.ext_zve32f && !cpu->cfg.ext_f) { > error_setg(errp, "Zve32f/Zve64f extensions require F extension"); > return; > }
On 2023/2/14 21:21, Daniel Henrique Barboza wrote: > > > On 2/14/23 05:38, Weiwei Li wrote: >> Add dependence chain: >> * V => Zve64d => Zve64f => Zve32f => F >> * V => Zve64d => D >> >> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> >> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> >> --- >> target/riscv/cpu.c | 21 ++++++++++++++++++--- >> 1 file changed, 18 insertions(+), 3 deletions(-) >> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> index 9a89bea2a3..4797ef9c42 100644 >> --- a/target/riscv/cpu.c >> +++ b/target/riscv/cpu.c >> @@ -743,12 +743,27 @@ static void >> riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp) >> return; >> } >> - if (cpu->cfg.ext_v && !cpu->cfg.ext_d) { >> - error_setg(errp, "V extension requires D extension"); >> + /* The V vector extension depends on the Zve64d extension */ >> + if (cpu->cfg.ext_v) { >> + cpu->cfg.ext_zve64d = true; >> + } >> + >> + /* The Zve64d extension depends on the Zve64f extension */ >> + if (cpu->cfg.ext_zve64d) { >> + cpu->cfg.ext_zve64f = true; >> + } >> + >> + /* The Zve64f extension depends on the Zve32f extension */ >> + if (cpu->cfg.ext_zve64f) { >> + cpu->cfg.ext_zve32f = true; >> + } >> + >> + if (cpu->cfg.ext_zve64d && !cpu->cfg.ext_d) { >> + error_setg(errp, "Zve64d extensions require D extension"); >> return; > > I'll be honest and confess that I wrote a short essay about the > problems I have > with this code. I gave up because in the end it's all stuff that we've > been doing > for a long time in riscv_cpu_validate_set_extensions(). I'll see if I > can work in > a redesign of that function and in how we're setting extensions > automatically > without checking user input and so on. > > For now I'll say that this error message seems weird because Zve64d > was set to true > without user input. So this ends up happening: > > $ ./qemu-system-riscv64 -M virt -cpu rv64,v=true,d=false > qemu-system-riscv64: Zve64d extensions require D extension > > It's weird because the user didn't enabled Zve64d but the error > message is complaining > about it. Given that the root cause is that ext_v was set, and then > we've set other > extensions under the hood, a saner error message in this case would be > "V extension > requires D extension". > > > Thanks, > > > Daniel Thanks for your comments. V extension depends on Zve64d(which is actually parts of V). So Zve64d will be enabled when V is enabled. And in fact, only the instructions in the Zve64d part of V require D extension. To make it more readable, maybe it can be change to : "Zve64d (or V) extension requires D extension" Regards, Weiwei Li > > > >> } >> - if ((cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) && >> !cpu->cfg.ext_f) { >> + if (cpu->cfg.ext_zve32f && !cpu->cfg.ext_f) { >> error_setg(errp, "Zve32f/Zve64f extensions require F >> extension"); >> return; >> }
On 2/14/23 10:40, weiwei wrote: > > On 2023/2/14 21:21, Daniel Henrique Barboza wrote: >> >> >> On 2/14/23 05:38, Weiwei Li wrote: >>> Add dependence chain: >>> * V => Zve64d => Zve64f => Zve32f => F >>> * V => Zve64d => D >>> >>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> >>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> >>> --- >>> target/riscv/cpu.c | 21 ++++++++++++++++++--- >>> 1 file changed, 18 insertions(+), 3 deletions(-) >>> >>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >>> index 9a89bea2a3..4797ef9c42 100644 >>> --- a/target/riscv/cpu.c >>> +++ b/target/riscv/cpu.c >>> @@ -743,12 +743,27 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp) >>> return; >>> } >>> - if (cpu->cfg.ext_v && !cpu->cfg.ext_d) { >>> - error_setg(errp, "V extension requires D extension"); >>> + /* The V vector extension depends on the Zve64d extension */ >>> + if (cpu->cfg.ext_v) { >>> + cpu->cfg.ext_zve64d = true; >>> + } >>> + >>> + /* The Zve64d extension depends on the Zve64f extension */ >>> + if (cpu->cfg.ext_zve64d) { >>> + cpu->cfg.ext_zve64f = true; >>> + } >>> + >>> + /* The Zve64f extension depends on the Zve32f extension */ >>> + if (cpu->cfg.ext_zve64f) { >>> + cpu->cfg.ext_zve32f = true; >>> + } >>> + >>> + if (cpu->cfg.ext_zve64d && !cpu->cfg.ext_d) { >>> + error_setg(errp, "Zve64d extensions require D extension"); >>> return; >> >> I'll be honest and confess that I wrote a short essay about the problems I have >> with this code. I gave up because in the end it's all stuff that we've been doing >> for a long time in riscv_cpu_validate_set_extensions(). I'll see if I can work in >> a redesign of that function and in how we're setting extensions automatically >> without checking user input and so on. >> >> For now I'll say that this error message seems weird because Zve64d was set to true >> without user input. So this ends up happening: >> >> $ ./qemu-system-riscv64 -M virt -cpu rv64,v=true,d=false >> qemu-system-riscv64: Zve64d extensions require D extension >> >> It's weird because the user didn't enabled Zve64d but the error message is complaining >> about it. Given that the root cause is that ext_v was set, and then we've set other >> extensions under the hood, a saner error message in this case would be "V extension >> requires D extension". >> >> >> Thanks, >> >> >> Daniel > > Thanks for your comments. > > V extension depends on Zve64d(which is actually parts of V). So Zve64d will be enabled when V is enabled. > > And in fact, only the instructions in the Zve64d part of V require D extension. > > To make it more readable, maybe it can be change to : > > "Zve64d (or V) extension requires D extension" Yes, that looks better to me. Thanks, Daniel > > Regards, > > Weiwei Li > >> >> >> >>> } >>> - if ((cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) && !cpu->cfg.ext_f) { >>> + if (cpu->cfg.ext_zve32f && !cpu->cfg.ext_f) { >>> error_setg(errp, "Zve32f/Zve64f extensions require F extension"); >>> return; >>> } >
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index 9a89bea2a3..4797ef9c42 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -743,12 +743,27 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp) return; } - if (cpu->cfg.ext_v && !cpu->cfg.ext_d) { - error_setg(errp, "V extension requires D extension"); + /* The V vector extension depends on the Zve64d extension */ + if (cpu->cfg.ext_v) { + cpu->cfg.ext_zve64d = true; + } + + /* The Zve64d extension depends on the Zve64f extension */ + if (cpu->cfg.ext_zve64d) { + cpu->cfg.ext_zve64f = true; + } + + /* The Zve64f extension depends on the Zve32f extension */ + if (cpu->cfg.ext_zve64f) { + cpu->cfg.ext_zve32f = true; + } + + if (cpu->cfg.ext_zve64d && !cpu->cfg.ext_d) { + error_setg(errp, "Zve64d extensions require D extension"); return; } - if ((cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) && !cpu->cfg.ext_f) { + if (cpu->cfg.ext_zve32f && !cpu->cfg.ext_f) { error_setg(errp, "Zve32f/Zve64f extensions require F extension"); return; }