* Re: [PATCH] target/riscv: fix inverted checks for ext_zb[abcs]
2022-03-01 1:28 ` Vineet Gupta
@ 2022-03-01 1:28 ` Vineet Gupta
2022-03-01 8:45 ` Philipp Tomsich
2022-03-02 7:53 ` Alistair Francis
2 siblings, 0 replies; 6+ messages in thread
From: Vineet Gupta @ 2022-03-01 1:28 UTC (permalink / raw)
To: Alistair Francis, Philipp Tomsich
Cc: Richard Henderson, Alistair Francis,
qemu-devel@nongnu.org Developers, gnu-toolchain
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
>>
>>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] target/riscv: fix inverted checks for ext_zb[abcs]
2022-03-01 1:28 ` Vineet Gupta
2022-03-01 1:28 ` Vineet Gupta
@ 2022-03-01 8:45 ` Philipp Tomsich
2022-03-02 7:53 ` Alistair Francis
2 siblings, 0 replies; 6+ messages in thread
From: Philipp Tomsich @ 2022-03-01 8:45 UTC (permalink / raw)
To: Vineet Gupta
Cc: Alistair Francis, Richard Henderson, Alistair Francis,
qemu-devel@nongnu.org Developers, gnu-toolchain
[-- Attachment #1: Type: text/plain, Size: 3179 bytes --]
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
> >>
> >>
> >
> >
>
>
[-- Attachment #2: Type: text/html, Size: 4939 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] target/riscv: fix inverted checks for ext_zb[abcs]
2022-03-01 1:28 ` Vineet Gupta
2022-03-01 1:28 ` Vineet Gupta
2022-03-01 8:45 ` Philipp Tomsich
@ 2022-03-02 7:53 ` Alistair Francis
2 siblings, 0 replies; 6+ messages in thread
From: Alistair Francis @ 2022-03-02 7:53 UTC (permalink / raw)
To: Vineet Gupta
Cc: Richard Henderson, gnu-toolchain, Alistair Francis,
qemu-devel@nongnu.org Developers, Philipp Tomsich
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
> >>
> >>
> >
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread