diff mbox series

[v4,2/5] RISC-V: add helpers for J-type immediate handling

Message ID 20230109181755.2383085-3-heiko@sntech.de (mailing list archive)
State Superseded
Delegated to: Palmer Dabbelt
Headers show
Series Zbb string optimizations and call support in alternatives | expand

Commit Message

Heiko Stübner Jan. 9, 2023, 6:17 p.m. UTC 
From: Heiko Stuebner <heiko.stuebner@vrull.eu>

Similar to the helper for the u-type + i-type imm handling, add helper-
functions for j-type immediates.

While it also would be possible to open-code that bit of imm wiggling
using the macros already provided in insn.h, it's way better to have
consistency on how we handle this across all function encoding/decoding.

Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
---
 arch/riscv/include/asm/insn.h | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Comments

Conor Dooley Jan. 9, 2023, 10:22 p.m. UTC | #1 
On Mon, Jan 09, 2023 at 07:17:52PM +0100, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> 
> Similar to the helper for the u-type + i-type imm handling, add helper-
> functions for j-type immediates.
> 
> While it also would be possible to open-code that bit of imm wiggling
> using the macros already provided in insn.h, it's way better to have
> consistency on how we handle this across all function encoding/decoding.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> ---
>  arch/riscv/include/asm/insn.h | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h
> index 0455b4dcb0a7..9eea61a3028f 100644
> --- a/arch/riscv/include/asm/insn.h
> +++ b/arch/riscv/include/asm/insn.h
> @@ -301,6 +301,32 @@ static __always_inline bool riscv_insn_is_branch(u32 code)
>  	(RVC_X(x_, RVC_B_IMM_7_6_OPOFF, RVC_B_IMM_7_6_MASK) << RVC_B_IMM_7_6_OFF) | \
>  	(RVC_IMM_SIGN(x_) << RVC_B_IMM_SIGN_OFF); })
>  
> +/*
> + * Get the immediate from a J-type instruction.
> + *
> + * @insn: instruction to process
> + * Return: immediate
> + */
> +static inline s32 riscv_insn_extract_jtype_imm(u32 insn)
> +{
> +	return RV_EXTRACT_JTYPE_IMM(insn);
> +}
> +
> +/*
> + * Update a J-type instruction with an immediate value.
> + *
> + * @insn: pointer to the jtype instruction
> + * @imm: the immediate to insert into the instruction
> + */
> +static inline void riscv_insn_insert_jtype_imm(u32 *insn, s32 imm)
> +{
> +	*insn &= ~GENMASK(31, 12);
> +	*insn |= (((imm & (RV_J_IMM_10_1_MASK << RV_J_IMM_10_1_OFF)) << RV_I_IMM_11_0_OPOFF) |
                                                                           ^
RV_I? That should be RV_J_IMM_10_1_OPOFF, no?

> +		  ((imm & (RV_J_IMM_11_MASK << RV_J_IMM_11_OFF)) << RV_J_IMM_11_OPOFF) |
> +		  ((imm & (RV_J_IMM_19_12_OPOFF << RV_J_IMM_19_12_OFF)) << RV_J_IMM_19_12_OPOFF) |
> +		  ((imm & (1 << RV_J_IMM_SIGN_OFF)) << RV_J_IMM_SIGN_OPOFF));
> +}

Otherwise, seems fine?

> +
>  /*
>   * Put together one immediate from a U-type and I-type instruction pair.
>   *
> -- 
> 2.35.1
>
Andrew Jones Jan. 10, 2023, 8:44 a.m. UTC | #2 
On Mon, Jan 09, 2023 at 07:17:52PM +0100, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> 
> Similar to the helper for the u-type + i-type imm handling, add helper-
> functions for j-type immediates.
> 
> While it also would be possible to open-code that bit of imm wiggling
> using the macros already provided in insn.h, it's way better to have
> consistency on how we handle this across all function encoding/decoding.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> ---
>  arch/riscv/include/asm/insn.h | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h
> index 0455b4dcb0a7..9eea61a3028f 100644
> --- a/arch/riscv/include/asm/insn.h
> +++ b/arch/riscv/include/asm/insn.h
> @@ -301,6 +301,32 @@ static __always_inline bool riscv_insn_is_branch(u32 code)
>  	(RVC_X(x_, RVC_B_IMM_7_6_OPOFF, RVC_B_IMM_7_6_MASK) << RVC_B_IMM_7_6_OFF) | \
>  	(RVC_IMM_SIGN(x_) << RVC_B_IMM_SIGN_OFF); })
>  
> +/*
> + * Get the immediate from a J-type instruction.
> + *
> + * @insn: instruction to process
> + * Return: immediate
> + */
> +static inline s32 riscv_insn_extract_jtype_imm(u32 insn)
> +{
> +	return RV_EXTRACT_JTYPE_IMM(insn);
> +}
> +
> +/*
> + * Update a J-type instruction with an immediate value.
> + *
> + * @insn: pointer to the jtype instruction
> + * @imm: the immediate to insert into the instruction
> + */
> +static inline void riscv_insn_insert_jtype_imm(u32 *insn, s32 imm)
> +{
> +	*insn &= ~GENMASK(31, 12);
> +	*insn |= (((imm & (RV_J_IMM_10_1_MASK << RV_J_IMM_10_1_OFF)) << RV_I_IMM_11_0_OPOFF) |
                                                                        ^ RV_J_IMM_10_1_OPOFF
									(as pointed out by Conor)

> +		  ((imm & (RV_J_IMM_11_MASK << RV_J_IMM_11_OFF)) << RV_J_IMM_11_OPOFF) |
> +		  ((imm & (RV_J_IMM_19_12_OPOFF << RV_J_IMM_19_12_OFF)) << RV_J_IMM_19_12_OPOFF) |
                           ^ RV_J_IMM_19_12_MASK


> +		  ((imm & (1 << RV_J_IMM_SIGN_OFF)) << RV_J_IMM_SIGN_OPOFF));

nit: can drop the outermost ()

> +}
> +
>  /*
>   * Put together one immediate from a U-type and I-type instruction pair.
>   *
> -- 
> 2.35.1
> 

Thanks,
drew
Conor Dooley Jan. 10, 2023, 8:54 a.m. UTC | #3 
On Tue, Jan 10, 2023 at 09:44:25AM +0100, Andrew Jones wrote:
> > +	*insn |= (((imm & (RV_J_IMM_10_1_MASK << RV_J_IMM_10_1_OFF)) << RV_I_IMM_11_0_OPOFF) |
>                                                                         ^ RV_J_IMM_10_1_OPOFF
> 									(as pointed out by Conor)

> > +		  ((imm & (RV_J_IMM_11_MASK << RV_J_IMM_11_OFF)) << RV_J_IMM_11_OPOFF) |
> > +		  ((imm & (RV_J_IMM_19_12_OPOFF << RV_J_IMM_19_12_OFF)) << RV_J_IMM_19_12_OPOFF) |
>                            ^ RV_J_IMM_19_12_MASK

You'd think that having seen one I'd have picked up on the other one
being wrong too...
Jisheng Zhang Jan. 11, 2023, 2:43 p.m. UTC | #4 
On Tue, Jan 10, 2023 at 08:54:33AM +0000, Conor Dooley wrote:
> On Tue, Jan 10, 2023 at 09:44:25AM +0100, Andrew Jones wrote:
> > > +	*insn |= (((imm & (RV_J_IMM_10_1_MASK << RV_J_IMM_10_1_OFF)) << RV_I_IMM_11_0_OPOFF) |
> >                                                                         ^ RV_J_IMM_10_1_OPOFF
> > 									(as pointed out by Conor)
> 
> > > +		  ((imm & (RV_J_IMM_11_MASK << RV_J_IMM_11_OFF)) << RV_J_IMM_11_OPOFF) |
> > > +		  ((imm & (RV_J_IMM_19_12_OPOFF << RV_J_IMM_19_12_OFF)) << RV_J_IMM_19_12_OPOFF) |
> >                            ^ RV_J_IMM_19_12_MASK
> 
> You'd think that having seen one I'd have picked up on the other one
> being wrong too...
> 

we also need RV_X() usage or do similar ;) 

[1] https://lore.kernel.org/linux-riscv/20221204174632.3677-2-jszhang@kernel.org/
diff mbox series

Patch

diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h
index 0455b4dcb0a7..9eea61a3028f 100644
--- a/arch/riscv/include/asm/insn.h
+++ b/arch/riscv/include/asm/insn.h
@@ -301,6 +301,32 @@  static __always_inline bool riscv_insn_is_branch(u32 code)
 	(RVC_X(x_, RVC_B_IMM_7_6_OPOFF, RVC_B_IMM_7_6_MASK) << RVC_B_IMM_7_6_OFF) | \
 	(RVC_IMM_SIGN(x_) << RVC_B_IMM_SIGN_OFF); })
 
+/*
+ * Get the immediate from a J-type instruction.
+ *
+ * @insn: instruction to process
+ * Return: immediate
+ */
+static inline s32 riscv_insn_extract_jtype_imm(u32 insn)
+{
+	return RV_EXTRACT_JTYPE_IMM(insn);
+}
+
+/*
+ * Update a J-type instruction with an immediate value.
+ *
+ * @insn: pointer to the jtype instruction
+ * @imm: the immediate to insert into the instruction
+ */
+static inline void riscv_insn_insert_jtype_imm(u32 *insn, s32 imm)
+{
+	*insn &= ~GENMASK(31, 12);
+	*insn |= (((imm & (RV_J_IMM_10_1_MASK << RV_J_IMM_10_1_OFF)) << RV_I_IMM_11_0_OPOFF) |
+		  ((imm & (RV_J_IMM_11_MASK << RV_J_IMM_11_OFF)) << RV_J_IMM_11_OPOFF) |
+		  ((imm & (RV_J_IMM_19_12_OPOFF << RV_J_IMM_19_12_OFF)) << RV_J_IMM_19_12_OPOFF) |
+		  ((imm & (1 << RV_J_IMM_SIGN_OFF)) << RV_J_IMM_SIGN_OPOFF));
+}
+
 /*
  * Put together one immediate from a U-type and I-type instruction pair.
  *