Re: [PATCH] target/riscv: fix inverted checks for ext_zb[abcs]

2022-03-01 Thread Alistair Francis
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]

2022-03-01 Thread Philipp Tomsich
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]

2022-02-28 Thread Vineet Gupta

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]

2022-02-03 Thread Alistair Francis
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]

2022-02-03 Thread Philipp Tomsich
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