diff mbox series

[v4,11/12] RISC-V: add helpers for handling immediates in U-type and I-type pairs

Message ID 20221207180821.2479987-12-heiko@sntech.de (mailing list archive)
State Superseded
Delegated to: Palmer Dabbelt
Headers show
Series Allow calls in alternatives | expand

Checks

Context Check Description
conchuod/patch_count success Link
conchuod/cover_letter success Series has a cover letter
conchuod/tree_selection success Guessed tree name to be fixes
conchuod/fixes_present success Fixes tag present in non-next series
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/kdoc success Errors and warnings before: 0 this patch: 0
conchuod/module_param success Was 0 now: 0
conchuod/alphanumeric_selects success Out of order selects before the patch: 45 and now 45
conchuod/build_rv32_defconfig success Build OK
conchuod/build_warn_rv64 success Errors and warnings before: 0 this patch: 0
conchuod/dtb_warn_rv64 success Errors and warnings before: 0 this patch: 0
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch success total: 0 errors, 0 warnings, 0 checks, 50 lines checked
conchuod/source_inline success Was 0 now: 0
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_fixes success No Fixes tag
conchuod/build_rv64_nommu_virt_defconfig success Build OK

Commit Message

Heiko Stuebner Dec. 7, 2022, 6:08 p.m. UTC 
From: Heiko Stuebner <heiko.stuebner@vrull.eu>

Used together U-type and I-type instructions can for example be used to
generate bigger jumps (i.e. in auipc+jalr pairs) by splitting the value
into an upper immediate (i.e. auipc) and a 12bit immediate (i.e. jalr).

Due to both immediates being considered signed this creates some corner
cases, so add some helper to prevent this from getting duplicated in
different places.

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

Comments

Conor Dooley Dec. 7, 2022, 8:07 p.m. UTC | #1 
On Wed, Dec 07, 2022 at 07:08:20PM +0100, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> 
> Used together U-type and I-type instructions can for example be used to
> generate bigger jumps (i.e. in auipc+jalr pairs) by splitting the value
> into an upper immediate (i.e. auipc) and a 12bit immediate (i.e. jalr).
> 
> Due to both immediates being considered signed this creates some corner
> cases, so add some helper to prevent this from getting duplicated in
> different places.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> ---
>  arch/riscv/include/asm/insn.h | 47 +++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h
> index 2a23890b4577..bb1e6120a560 100644
> --- a/arch/riscv/include/asm/insn.h
> +++ b/arch/riscv/include/asm/insn.h
> @@ -290,3 +290,50 @@ static __always_inline bool riscv_insn_is_branch(u32 code)
>  	(RVC_X(x_, RVC_B_IMM_5_OPOFF, RVC_B_IMM_5_MASK) << RVC_B_IMM_5_OFF) | \
>  	(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); })
> +
> +/*
> + * Put together one immediate from a U-type and I-type instruction pair.
> + *
> + * The U-type contains an upper immediate, meaning bits[31:12] with [11:0]
> + * being zero, while the I-type contains a 12bit immediate.
> + * Combined these can encode larger 32bit values and are used for example
> + * in auipc + jalr pairs to allow larger jumps.
> + *
> + * @utype_insn: instruction containing the upper immediate
> + * @itype_insn: instruction
> + * Return: combined immediate
> + */
> +static inline s32 riscv_insn_extract_utype_itype_imm(u32 utype_insn, u32 itype_insn)
> +{
> +	s32 imm;
> +
> +	imm = RV_EXTRACT_UTYPE_IMM(utype_insn);
> +	imm += RV_EXTRACT_ITYPE_IMM(itype_insn);
> +
> +	return imm;
> +}
> +
> +/*
> + * Update a set of two instructions (U-type + I-type) with an immediate value.
> + *
> + * Used for example in auipc+jalrs pairs the U-type instructions contains
> + * a 20bit upper immediate representing bits[31:12], while the I-type
> + * instruction contains a 12bit immediate representing bits[11:0].
> + *
> + * This also takes into account that both separate immediates are
> + * considered as signed values, so if the I-type immediate becomes
> + * negative (BIT(11) set) the U-type part gets adjusted.
> + *
> + * @insn: pointer to a set of two instructions
> + * @imm: the immediate to insert into the two instructions
> + */
> +static inline void riscv_insn_insert_utype_itype_imm(u32 *insn, s32 imm)
> +{
> +	/* drop possible old IMM values */
> +	insn[0] &= ~(RV_U_IMM_31_12_MASK);
> +	insn[1] &= ~(RV_I_IMM_11_0_MASK << RV_I_IMM_11_0_OPOFF);
> +
> +	/* add the adapted IMMs */
> +	insn[0] |= ((imm & RV_U_IMM_31_12_MASK) + ((imm & BIT(11)) << 1));
> +	insn[1] |= ((imm & RV_I_IMM_11_0_MASK) << RV_I_IMM_11_0_OPOFF);

If you send a v5, could you drop the extra ()s around some of these?
Otherwise:
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
I do like the comments a lot :) Saves going off to read specs...

> +}
> -- 
> 2.35.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Heiko Stuebner Dec. 7, 2022, 10:43 p.m. UTC | #2 
Am Mittwoch, 7. Dezember 2022, 21:07:39 CET schrieb Conor Dooley:
> On Wed, Dec 07, 2022 at 07:08:20PM +0100, Heiko Stuebner wrote:
> > From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> > 
> > Used together U-type and I-type instructions can for example be used to
> > generate bigger jumps (i.e. in auipc+jalr pairs) by splitting the value
> > into an upper immediate (i.e. auipc) and a 12bit immediate (i.e. jalr).
> > 
> > Due to both immediates being considered signed this creates some corner
> > cases, so add some helper to prevent this from getting duplicated in
> > different places.
> > 
> > Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> > ---
> >  arch/riscv/include/asm/insn.h | 47 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> > 
> > diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h
> > index 2a23890b4577..bb1e6120a560 100644
> > --- a/arch/riscv/include/asm/insn.h
> > +++ b/arch/riscv/include/asm/insn.h
> > @@ -290,3 +290,50 @@ static __always_inline bool riscv_insn_is_branch(u32 code)
> >  	(RVC_X(x_, RVC_B_IMM_5_OPOFF, RVC_B_IMM_5_MASK) << RVC_B_IMM_5_OFF) | \
> >  	(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); })
> > +
> > +/*
> > + * Put together one immediate from a U-type and I-type instruction pair.
> > + *
> > + * The U-type contains an upper immediate, meaning bits[31:12] with [11:0]
> > + * being zero, while the I-type contains a 12bit immediate.
> > + * Combined these can encode larger 32bit values and are used for example
> > + * in auipc + jalr pairs to allow larger jumps.
> > + *
> > + * @utype_insn: instruction containing the upper immediate
> > + * @itype_insn: instruction
> > + * Return: combined immediate
> > + */
> > +static inline s32 riscv_insn_extract_utype_itype_imm(u32 utype_insn, u32 itype_insn)
> > +{
> > +	s32 imm;
> > +
> > +	imm = RV_EXTRACT_UTYPE_IMM(utype_insn);
> > +	imm += RV_EXTRACT_ITYPE_IMM(itype_insn);
> > +
> > +	return imm;
> > +}
> > +
> > +/*
> > + * Update a set of two instructions (U-type + I-type) with an immediate value.
> > + *
> > + * Used for example in auipc+jalrs pairs the U-type instructions contains
> > + * a 20bit upper immediate representing bits[31:12], while the I-type
> > + * instruction contains a 12bit immediate representing bits[11:0].
> > + *
> > + * This also takes into account that both separate immediates are
> > + * considered as signed values, so if the I-type immediate becomes
> > + * negative (BIT(11) set) the U-type part gets adjusted.
> > + *
> > + * @insn: pointer to a set of two instructions
> > + * @imm: the immediate to insert into the two instructions
> > + */
> > +static inline void riscv_insn_insert_utype_itype_imm(u32 *insn, s32 imm)
> > +{
> > +	/* drop possible old IMM values */
> > +	insn[0] &= ~(RV_U_IMM_31_12_MASK);
> > +	insn[1] &= ~(RV_I_IMM_11_0_MASK << RV_I_IMM_11_0_OPOFF);
> > +
> > +	/* add the adapted IMMs */
> > +	insn[0] |= ((imm & RV_U_IMM_31_12_MASK) + ((imm & BIT(11)) << 1));
> > +	insn[1] |= ((imm & RV_I_IMM_11_0_MASK) << RV_I_IMM_11_0_OPOFF);
> 
> If you send a v5, could you drop the extra ()s around some of these?

will do

> Otherwise:
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> I do like the comments a lot :) Saves going off to read specs...

That was the intention with the big comments :-) .

And self-preservation ... Once my mind does a cache-flush of these things,
I don't want to have to dig through things to re-understand this again.


Heiko
Andrew Jones Dec. 8, 2022, 2:38 p.m. UTC | #3 
On Wed, Dec 07, 2022 at 07:08:20PM +0100, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> 
> Used together U-type and I-type instructions can for example be used to
> generate bigger jumps (i.e. in auipc+jalr pairs) by splitting the value
> into an upper immediate (i.e. auipc) and a 12bit immediate (i.e. jalr).
> 
> Due to both immediates being considered signed this creates some corner
> cases, so add some helper to prevent this from getting duplicated in
> different places.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> ---
>  arch/riscv/include/asm/insn.h | 47 +++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h
> index 2a23890b4577..bb1e6120a560 100644
> --- a/arch/riscv/include/asm/insn.h
> +++ b/arch/riscv/include/asm/insn.h
> @@ -290,3 +290,50 @@ static __always_inline bool riscv_insn_is_branch(u32 code)
>  	(RVC_X(x_, RVC_B_IMM_5_OPOFF, RVC_B_IMM_5_MASK) << RVC_B_IMM_5_OFF) | \
>  	(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); })
> +
> +/*
> + * Put together one immediate from a U-type and I-type instruction pair.
> + *
> + * The U-type contains an upper immediate, meaning bits[31:12] with [11:0]
> + * being zero, while the I-type contains a 12bit immediate.
> + * Combined these can encode larger 32bit values and are used for example
> + * in auipc + jalr pairs to allow larger jumps.
> + *
> + * @utype_insn: instruction containing the upper immediate
> + * @itype_insn: instruction
> + * Return: combined immediate
> + */
> +static inline s32 riscv_insn_extract_utype_itype_imm(u32 utype_insn, u32 itype_insn)

Might be nice to keep the interfaces of these two complementing functions
consistent by taking a u32 *insn here, like below. Or, below, taking
&utype_insn and &itype_insn, like here.

> +{
> +	s32 imm;
> +
> +	imm = RV_EXTRACT_UTYPE_IMM(utype_insn);
> +	imm += RV_EXTRACT_ITYPE_IMM(itype_insn);
> +
> +	return imm;
> +}
> +
> +/*
> + * Update a set of two instructions (U-type + I-type) with an immediate value.
> + *
> + * Used for example in auipc+jalrs pairs the U-type instructions contains
> + * a 20bit upper immediate representing bits[31:12], while the I-type
> + * instruction contains a 12bit immediate representing bits[11:0].
> + *
> + * This also takes into account that both separate immediates are
> + * considered as signed values, so if the I-type immediate becomes
> + * negative (BIT(11) set) the U-type part gets adjusted.
> + *
> + * @insn: pointer to a set of two instructions
> + * @imm: the immediate to insert into the two instructions
> + */
> +static inline void riscv_insn_insert_utype_itype_imm(u32 *insn, s32 imm)
> +{
> +	/* drop possible old IMM values */
> +	insn[0] &= ~(RV_U_IMM_31_12_MASK);
> +	insn[1] &= ~(RV_I_IMM_11_0_MASK << RV_I_IMM_11_0_OPOFF);
> +
> +	/* add the adapted IMMs */
> +	insn[0] |= ((imm & RV_U_IMM_31_12_MASK) + ((imm & BIT(11)) << 1));
> +	insn[1] |= ((imm & RV_I_IMM_11_0_MASK) << RV_I_IMM_11_0_OPOFF);
> +}
> -- 
> 2.35.1
>

Otherwise

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Prabhakar Dec. 11, 2022, 8:45 p.m. UTC | #4 
On Wed, Dec 7, 2022 at 6:09 PM Heiko Stuebner <heiko@sntech.de> wrote:
>
> From: Heiko Stuebner <heiko.stuebner@vrull.eu>
>
> Used together U-type and I-type instructions can for example be used to
> generate bigger jumps (i.e. in auipc+jalr pairs) by splitting the value
> into an upper immediate (i.e. auipc) and a 12bit immediate (i.e. jalr).
>
> Due to both immediates being considered signed this creates some corner
> cases, so add some helper to prevent this from getting duplicated in
> different places.
>
> Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> ---
>  arch/riscv/include/asm/insn.h | 47 +++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Cheers,
Prabhakar
Heiko Stuebner Dec. 22, 2022, 1:46 p.m. UTC | #5 
Am Donnerstag, 8. Dezember 2022, 15:38:11 CET schrieb Andrew Jones:
> On Wed, Dec 07, 2022 at 07:08:20PM +0100, Heiko Stuebner wrote:
> > From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> > 
> > Used together U-type and I-type instructions can for example be used to
> > generate bigger jumps (i.e. in auipc+jalr pairs) by splitting the value
> > into an upper immediate (i.e. auipc) and a 12bit immediate (i.e. jalr).
> > 
> > Due to both immediates being considered signed this creates some corner
> > cases, so add some helper to prevent this from getting duplicated in
> > different places.
> > 
> > Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> > ---
> >  arch/riscv/include/asm/insn.h | 47 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> > 
> > diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h
> > index 2a23890b4577..bb1e6120a560 100644
> > --- a/arch/riscv/include/asm/insn.h
> > +++ b/arch/riscv/include/asm/insn.h
> > @@ -290,3 +290,50 @@ static __always_inline bool riscv_insn_is_branch(u32 code)
> >  	(RVC_X(x_, RVC_B_IMM_5_OPOFF, RVC_B_IMM_5_MASK) << RVC_B_IMM_5_OFF) | \
> >  	(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); })
> > +
> > +/*
> > + * Put together one immediate from a U-type and I-type instruction pair.
> > + *
> > + * The U-type contains an upper immediate, meaning bits[31:12] with [11:0]
> > + * being zero, while the I-type contains a 12bit immediate.
> > + * Combined these can encode larger 32bit values and are used for example
> > + * in auipc + jalr pairs to allow larger jumps.
> > + *
> > + * @utype_insn: instruction containing the upper immediate
> > + * @itype_insn: instruction
> > + * Return: combined immediate
> > + */
> > +static inline s32 riscv_insn_extract_utype_itype_imm(u32 utype_insn, u32 itype_insn)
> 
> Might be nice to keep the interfaces of these two complementing functions
> consistent by taking a u32 *insn here, like below. Or, below, taking
> &utype_insn and &itype_insn, like here.

I did go with separating the instructions in the insert function, simply
because I think it's way nicer to not impose the fixed structure
(2 u32 array) on every possible caller .

Heiko
diff mbox series

Patch

diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h
index 2a23890b4577..bb1e6120a560 100644
--- a/arch/riscv/include/asm/insn.h
+++ b/arch/riscv/include/asm/insn.h
@@ -290,3 +290,50 @@  static __always_inline bool riscv_insn_is_branch(u32 code)
 	(RVC_X(x_, RVC_B_IMM_5_OPOFF, RVC_B_IMM_5_MASK) << RVC_B_IMM_5_OFF) | \
 	(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); })
+
+/*
+ * Put together one immediate from a U-type and I-type instruction pair.
+ *
+ * The U-type contains an upper immediate, meaning bits[31:12] with [11:0]
+ * being zero, while the I-type contains a 12bit immediate.
+ * Combined these can encode larger 32bit values and are used for example
+ * in auipc + jalr pairs to allow larger jumps.
+ *
+ * @utype_insn: instruction containing the upper immediate
+ * @itype_insn: instruction
+ * Return: combined immediate
+ */
+static inline s32 riscv_insn_extract_utype_itype_imm(u32 utype_insn, u32 itype_insn)
+{
+	s32 imm;
+
+	imm = RV_EXTRACT_UTYPE_IMM(utype_insn);
+	imm += RV_EXTRACT_ITYPE_IMM(itype_insn);
+
+	return imm;
+}
+
+/*
+ * Update a set of two instructions (U-type + I-type) with an immediate value.
+ *
+ * Used for example in auipc+jalrs pairs the U-type instructions contains
+ * a 20bit upper immediate representing bits[31:12], while the I-type
+ * instruction contains a 12bit immediate representing bits[11:0].
+ *
+ * This also takes into account that both separate immediates are
+ * considered as signed values, so if the I-type immediate becomes
+ * negative (BIT(11) set) the U-type part gets adjusted.
+ *
+ * @insn: pointer to a set of two instructions
+ * @imm: the immediate to insert into the two instructions
+ */
+static inline void riscv_insn_insert_utype_itype_imm(u32 *insn, s32 imm)
+{
+	/* drop possible old IMM values */
+	insn[0] &= ~(RV_U_IMM_31_12_MASK);
+	insn[1] &= ~(RV_I_IMM_11_0_MASK << RV_I_IMM_11_0_OPOFF);
+
+	/* add the adapted IMMs */
+	insn[0] |= ((imm & RV_U_IMM_31_12_MASK) + ((imm & BIT(11)) << 1));
+	insn[1] |= ((imm & RV_I_IMM_11_0_MASK) << RV_I_IMM_11_0_OPOFF);
+}