Re: [PATCH] target/riscv: fix inverted checks for ext_zb[abcs]
On Tue, Mar 1, 2022 at 11:28 AM Vineet Gupta wrote: > > Hi Alistair, > > On 2/3/22 16:59, Alistair Francis wrote: > > On Fri, Feb 4, 2022 at 1:42 AM Philipp Tomsich > > wrote: > >> > >> While changing to the use of cfg_ptr, the conditions for REQUIRE_ZB[ABCS] > >> inadvertently became inverted and slipped through the initial testing > >> (which > >> used RV64GC_XVentanaCondOps as a target). > >> This fixes the regression. > >> > >> Tested against SPEC2017 w/ GCC 12 (prerelease) for RV64GC_zba_zbb_zbc_zbs. > >> > >> Fixes: 718143c126 ("target/riscv: add a MAINTAINERS entry for > >> XVentanaCondOps") > >> > >> Signed-off-by: Philipp Tomsich > > > > Reviewed-by: Alistair Francis > > > >> > >> --- > >> We may want to squash this onto the affected commit, if it hasn't made > >> it beyond the next-tree, yet. > > > > Yeah, agreed. I'll squash it in > > > > Alistair > > Has this already been committed upstream. I was running into weird issue > related to bitmanip and seems this was missing in my local tree. > > Also the "Fixes: " entry in changelog doesn't seem OK; the issue seems > to have been introduced in f2a32bec8f0da99 ("target/riscv: access cfg > structure through DisasContext") Thanks, I have fixed the fixes tag and the indentation, I'll try and send a PR this week or early next week. Alistair > > Thx, > -Vineet > > > > >> > >> target/riscv/insn_trans/trans_rvb.c.inc | 8 > >> 1 file changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/target/riscv/insn_trans/trans_rvb.c.inc > >> b/target/riscv/insn_trans/trans_rvb.c.inc > >> index f9bd3b7ec4..e3c6b459d6 100644 > >> --- a/target/riscv/insn_trans/trans_rvb.c.inc > >> +++ b/target/riscv/insn_trans/trans_rvb.c.inc > >> @@ -19,25 +19,25 @@ > >>*/ > >> > >> #define REQUIRE_ZBA(ctx) do {\ > >> -if (ctx->cfg_ptr->ext_zba) { \ > >> +if (!ctx->cfg_ptr->ext_zba) { \ > >> return false;\ > >> }\ > >> } while (0) > >> > >> #define REQUIRE_ZBB(ctx) do {\ > >> -if (ctx->cfg_ptr->ext_zbb) { \ > >> +if (!ctx->cfg_ptr->ext_zbb) { \ > >> return false;\ > >> }\ > >> } while (0) > >> > >> #define REQUIRE_ZBC(ctx) do {\ > >> -if (ctx->cfg_ptr->ext_zbc) { \ > >> +if (!ctx->cfg_ptr->ext_zbc) { \ > >> return false;\ > >> }\ > >> } while (0) > >> > >> #define REQUIRE_ZBS(ctx) do {\ > >> -if (ctx->cfg_ptr->ext_zbs) { \ > >> +if (!ctx->cfg_ptr->ext_zbs) { \ > >> return false;\ > >> }\ > >> } while (0) > >> -- > >> 2.34.1 > >> > >> > > > > >
Re: [PATCH] target/riscv: fix inverted checks for ext_zb[abcs]
On Tue, 1 Mar 2022 at 02:28, Vineet Gupta wrote: > Hi Alistair, > > On 2/3/22 16:59, Alistair Francis wrote: > > On Fri, Feb 4, 2022 at 1:42 AM Philipp Tomsich > wrote: > >> > >> While changing to the use of cfg_ptr, the conditions for > REQUIRE_ZB[ABCS] > >> inadvertently became inverted and slipped through the initial testing > (which > >> used RV64GC_XVentanaCondOps as a target). > >> This fixes the regression. > >> > >> Tested against SPEC2017 w/ GCC 12 (prerelease) for > RV64GC_zba_zbb_zbc_zbs. > >> > >> Fixes: 718143c126 ("target/riscv: add a MAINTAINERS entry for > XVentanaCondOps") > >> > >> Signed-off-by: Philipp Tomsich > > > > Reviewed-by: Alistair Francis > > > >> > >> --- > >> We may want to squash this onto the affected commit, if it hasn't made > >> it beyond the next-tree, yet. > > > > Yeah, agreed. I'll squash it in > > > > Alistair > > Has this already been committed upstream. I was running into weird issue > related to bitmanip and seems this was missing in my local tree. > After checking master now, this has not made it onto master yet. Note that rc0 is planned for 2 weeks from now, so I am not overly concerned yet. Philipp. > Also the "Fixes: " entry in changelog doesn't seem OK; the issue seems > to have been introduced in f2a32bec8f0da99 ("target/riscv: access cfg > structure through DisasContext") > > Thx, > -Vineet > > > > >> > >> target/riscv/insn_trans/trans_rvb.c.inc | 8 > >> 1 file changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/target/riscv/insn_trans/trans_rvb.c.inc > b/target/riscv/insn_trans/trans_rvb.c.inc > >> index f9bd3b7ec4..e3c6b459d6 100644 > >> --- a/target/riscv/insn_trans/trans_rvb.c.inc > >> +++ b/target/riscv/insn_trans/trans_rvb.c.inc > >> @@ -19,25 +19,25 @@ > >>*/ > >> > >> #define REQUIRE_ZBA(ctx) do {\ > >> -if (ctx->cfg_ptr->ext_zba) { \ > >> +if (!ctx->cfg_ptr->ext_zba) { \ > >> return false;\ > >> }\ > >> } while (0) > >> > >> #define REQUIRE_ZBB(ctx) do {\ > >> -if (ctx->cfg_ptr->ext_zbb) { \ > >> +if (!ctx->cfg_ptr->ext_zbb) { \ > >> return false;\ > >> }\ > >> } while (0) > >> > >> #define REQUIRE_ZBC(ctx) do {\ > >> -if (ctx->cfg_ptr->ext_zbc) { \ > >> +if (!ctx->cfg_ptr->ext_zbc) { \ > >> return false;\ > >> }\ > >> } while (0) > >> > >> #define REQUIRE_ZBS(ctx) do {\ > >> -if (ctx->cfg_ptr->ext_zbs) { \ > >> +if (!ctx->cfg_ptr->ext_zbs) { \ > >> return false;\ > >> }\ > >> } while (0) > >> -- > >> 2.34.1 > >> > >> > > > > > >
Re: [PATCH] target/riscv: fix inverted checks for ext_zb[abcs]
Hi Alistair, On 2/3/22 16:59, Alistair Francis wrote: On Fri, Feb 4, 2022 at 1:42 AM Philipp Tomsich wrote: While changing to the use of cfg_ptr, the conditions for REQUIRE_ZB[ABCS] inadvertently became inverted and slipped through the initial testing (which used RV64GC_XVentanaCondOps as a target). This fixes the regression. Tested against SPEC2017 w/ GCC 12 (prerelease) for RV64GC_zba_zbb_zbc_zbs. Fixes: 718143c126 ("target/riscv: add a MAINTAINERS entry for XVentanaCondOps") Signed-off-by: Philipp Tomsich Reviewed-by: Alistair Francis --- We may want to squash this onto the affected commit, if it hasn't made it beyond the next-tree, yet. Yeah, agreed. I'll squash it in Alistair Has this already been committed upstream. I was running into weird issue related to bitmanip and seems this was missing in my local tree. Also the "Fixes: " entry in changelog doesn't seem OK; the issue seems to have been introduced in f2a32bec8f0da99 ("target/riscv: access cfg structure through DisasContext") Thx, -Vineet target/riscv/insn_trans/trans_rvb.c.inc | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/target/riscv/insn_trans/trans_rvb.c.inc b/target/riscv/insn_trans/trans_rvb.c.inc index f9bd3b7ec4..e3c6b459d6 100644 --- a/target/riscv/insn_trans/trans_rvb.c.inc +++ b/target/riscv/insn_trans/trans_rvb.c.inc @@ -19,25 +19,25 @@ */ #define REQUIRE_ZBA(ctx) do {\ -if (ctx->cfg_ptr->ext_zba) { \ +if (!ctx->cfg_ptr->ext_zba) { \ return false;\ }\ } while (0) #define REQUIRE_ZBB(ctx) do {\ -if (ctx->cfg_ptr->ext_zbb) { \ +if (!ctx->cfg_ptr->ext_zbb) { \ return false;\ }\ } while (0) #define REQUIRE_ZBC(ctx) do {\ -if (ctx->cfg_ptr->ext_zbc) { \ +if (!ctx->cfg_ptr->ext_zbc) { \ return false;\ }\ } while (0) #define REQUIRE_ZBS(ctx) do {\ -if (ctx->cfg_ptr->ext_zbs) { \ +if (!ctx->cfg_ptr->ext_zbs) { \ return false;\ }\ } while (0) -- 2.34.1
Re: [PATCH] target/riscv: fix inverted checks for ext_zb[abcs]
On Fri, Feb 4, 2022 at 1:42 AM Philipp Tomsich wrote: > > While changing to the use of cfg_ptr, the conditions for REQUIRE_ZB[ABCS] > inadvertently became inverted and slipped through the initial testing (which > used RV64GC_XVentanaCondOps as a target). > This fixes the regression. > > Tested against SPEC2017 w/ GCC 12 (prerelease) for RV64GC_zba_zbb_zbc_zbs. > > Fixes: 718143c126 ("target/riscv: add a MAINTAINERS entry for > XVentanaCondOps") > > Signed-off-by: Philipp Tomsich Reviewed-by: Alistair Francis > > --- > We may want to squash this onto the affected commit, if it hasn't made > it beyond the next-tree, yet. Yeah, agreed. I'll squash it in Alistair > > target/riscv/insn_trans/trans_rvb.c.inc | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/target/riscv/insn_trans/trans_rvb.c.inc > b/target/riscv/insn_trans/trans_rvb.c.inc > index f9bd3b7ec4..e3c6b459d6 100644 > --- a/target/riscv/insn_trans/trans_rvb.c.inc > +++ b/target/riscv/insn_trans/trans_rvb.c.inc > @@ -19,25 +19,25 @@ > */ > > #define REQUIRE_ZBA(ctx) do {\ > -if (ctx->cfg_ptr->ext_zba) { \ > +if (!ctx->cfg_ptr->ext_zba) { \ > return false;\ > }\ > } while (0) > > #define REQUIRE_ZBB(ctx) do {\ > -if (ctx->cfg_ptr->ext_zbb) { \ > +if (!ctx->cfg_ptr->ext_zbb) { \ > return false;\ > }\ > } while (0) > > #define REQUIRE_ZBC(ctx) do {\ > -if (ctx->cfg_ptr->ext_zbc) { \ > +if (!ctx->cfg_ptr->ext_zbc) { \ > return false;\ > }\ > } while (0) > > #define REQUIRE_ZBS(ctx) do {\ > -if (ctx->cfg_ptr->ext_zbs) { \ > +if (!ctx->cfg_ptr->ext_zbs) { \ > return false;\ > }\ > } while (0) > -- > 2.34.1 > >
[PATCH] target/riscv: fix inverted checks for ext_zb[abcs]
While changing to the use of cfg_ptr, the conditions for REQUIRE_ZB[ABCS] inadvertently became inverted and slipped through the initial testing (which used RV64GC_XVentanaCondOps as a target). This fixes the regression. Tested against SPEC2017 w/ GCC 12 (prerelease) for RV64GC_zba_zbb_zbc_zbs. Fixes: 718143c126 ("target/riscv: add a MAINTAINERS entry for XVentanaCondOps") Signed-off-by: Philipp Tomsich --- We may want to squash this onto the affected commit, if it hasn't made it beyond the next-tree, yet. target/riscv/insn_trans/trans_rvb.c.inc | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/target/riscv/insn_trans/trans_rvb.c.inc b/target/riscv/insn_trans/trans_rvb.c.inc index f9bd3b7ec4..e3c6b459d6 100644 --- a/target/riscv/insn_trans/trans_rvb.c.inc +++ b/target/riscv/insn_trans/trans_rvb.c.inc @@ -19,25 +19,25 @@ */ #define REQUIRE_ZBA(ctx) do {\ -if (ctx->cfg_ptr->ext_zba) { \ +if (!ctx->cfg_ptr->ext_zba) { \ return false;\ }\ } while (0) #define REQUIRE_ZBB(ctx) do {\ -if (ctx->cfg_ptr->ext_zbb) { \ +if (!ctx->cfg_ptr->ext_zbb) { \ return false;\ }\ } while (0) #define REQUIRE_ZBC(ctx) do {\ -if (ctx->cfg_ptr->ext_zbc) { \ +if (!ctx->cfg_ptr->ext_zbc) { \ return false;\ }\ } while (0) #define REQUIRE_ZBS(ctx) do {\ -if (ctx->cfg_ptr->ext_zbs) { \ +if (!ctx->cfg_ptr->ext_zbs) { \ return false;\ }\ } while (0) -- 2.34.1