| Message ID | 20220203153946.2676353-1-philipp.tomsich@vrull.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv: fix inverted checks for ext_zb[abcs] | expand |
On Fri, Feb 4, 2022 at 1:42 AM Philipp Tomsich <philipp.tomsich@vrull.eu> 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 <philipp.tomsich@vrull.eu> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > > --- > 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 > >
Hi Alistair, On 2/3/22 16:59, Alistair Francis wrote: > On Fri, Feb 4, 2022 at 1:42 AM Philipp Tomsich <philipp.tomsich@vrull.eu> 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 <philipp.tomsich@vrull.eu> > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > >> >> --- >> 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 >> >> > >
On Tue, 1 Mar 2022 at 02:28, Vineet Gupta <vineet.gupta@linux.dev> wrote: > Hi Alistair, > > On 2/3/22 16:59, Alistair Francis wrote: > > On Fri, Feb 4, 2022 at 1:42 AM Philipp Tomsich <philipp.tomsich@vrull.eu> > 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 <philipp.tomsich@vrull.eu> > > > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > > > >> > >> --- > >> 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 > >> > >> > > > > > >
On Tue, Mar 1, 2022 at 11:28 AM Vineet Gupta <vineet.gupta@linux.dev> wrote: > > Hi Alistair, > > On 2/3/22 16:59, Alistair Francis wrote: > > On Fri, Feb 4, 2022 at 1:42 AM Philipp Tomsich <philipp.tomsich@vrull.eu> 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 <philipp.tomsich@vrull.eu> > > > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > > > >> > >> --- > >> 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 > >> > >> > > > > >
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)
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 <philipp.tomsich@vrull.eu> --- 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(-)