target/riscv/insn_trans/trans_rvf.inc.c | 1 + 1 file changed, 1 insertion(+)
For FMV.W.X that moves the lower 32 bits of an integer register to a
floating-point register, Rd should encoded with NoN-boxing scheme.
Note: This applies to RV64 only.
Signed-off-by: Ian Jiang <ianjiang.ict@gmail.com>
---
target/riscv/insn_trans/trans_rvf.inc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/target/riscv/insn_trans/trans_rvf.inc.c b/target/riscv/insn_trans/trans_rvf.inc.c
index 172dbfa919..62b7a36567 100644
--- a/target/riscv/insn_trans/trans_rvf.inc.c
+++ b/target/riscv/insn_trans/trans_rvf.inc.c
@@ -368,6 +368,7 @@ static bool trans_fmv_w_x(DisasContext *ctx, arg_fmv_w_x *a)
#if defined(TARGET_RISCV64)
tcg_gen_mov_i64(cpu_fpr[a->rd], t0);
+ tcg_gen_ori_i64(cpu_fpr[a->rd], cpu_fpr[a->rd], 0xffffffff00000000ULL);
#else
tcg_gen_extu_i32_i64(cpu_fpr[a->rd], t0);
#endif
--
2.17.1
On 1/21/20 9:24 PM, Ian Jiang wrote: > For FMV.W.X that moves the lower 32 bits of an integer register to a > floating-point register, Rd should encoded with NoN-boxing scheme. > Note: This applies to RV64 only. > > Signed-off-by: Ian Jiang <ianjiang.ict@gmail.com> > --- > target/riscv/insn_trans/trans_rvf.inc.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/target/riscv/insn_trans/trans_rvf.inc.c b/target/riscv/insn_trans/trans_rvf.inc.c > index 172dbfa919..62b7a36567 100644 > --- a/target/riscv/insn_trans/trans_rvf.inc.c > +++ b/target/riscv/insn_trans/trans_rvf.inc.c > @@ -368,6 +368,7 @@ static bool trans_fmv_w_x(DisasContext *ctx, arg_fmv_w_x *a) > > #if defined(TARGET_RISCV64) > tcg_gen_mov_i64(cpu_fpr[a->rd], t0); > + tcg_gen_ori_i64(cpu_fpr[a->rd], cpu_fpr[a->rd], 0xffffffff00000000ULL); > #else > tcg_gen_extu_i32_i64(cpu_fpr[a->rd], t0); > #endif > This doesn't look right. There's nothing in the spec that says the nan-boxing is restricted to rv64. NaN-boxing is all about FLEN, not XLEN. r~
-- Ian Jiang Richard Henderson <richard.henderson@linaro.org> 于2020年1月22日周三 下午4:53写道: > > On 1/21/20 9:24 PM, Ian Jiang wrote: > > For FMV.W.X that moves the lower 32 bits of an integer register to a > > floating-point register, Rd should encoded with NoN-boxing scheme. > > Note: This applies to RV64 only. > > > > Signed-off-by: Ian Jiang <ianjiang.ict@gmail.com> > > --- > > target/riscv/insn_trans/trans_rvf.inc.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/target/riscv/insn_trans/trans_rvf.inc.c b/target/riscv/insn_trans/trans_rvf.inc.c > > index 172dbfa919..62b7a36567 100644 > > --- a/target/riscv/insn_trans/trans_rvf.inc.c > > +++ b/target/riscv/insn_trans/trans_rvf.inc.c > > @@ -368,6 +368,7 @@ static bool trans_fmv_w_x(DisasContext *ctx, arg_fmv_w_x *a) > > > > #if defined(TARGET_RISCV64) > > tcg_gen_mov_i64(cpu_fpr[a->rd], t0); > > + tcg_gen_ori_i64(cpu_fpr[a->rd], cpu_fpr[a->rd], 0xffffffff00000000ULL); > > #else > > tcg_gen_extu_i32_i64(cpu_fpr[a->rd], t0); > > #endif > > > > This doesn't look right. There's nothing in the spec that says the nan-boxing > is restricted to rv64. NaN-boxing is all about FLEN, not XLEN. > Why the translation of FLW has a NaN-boxing? file ./target/riscv/insn_trans/trans_rvf.inc.c 26 static bool trans_flw(DisasContext *ctx, arg_flw *a) 27 { ... 34 tcg_gen_qemu_ld_i64(cpu_fpr[a->rd], t0, ctx->mem_idx, MO_TEUL); 35 /* RISC-V requires NaN-boxing of narrower width floating point values */ 36 tcg_gen_ori_i64(cpu_fpr[a->rd], cpu_fpr[a->rd], 0xffffffff00000000ULL); ... 41 }
On 1/21/20 11:53 PM, Ian Jiang wrote: > -- > Ian Jiang > > Richard Henderson <richard.henderson@linaro.org> 于2020年1月22日周三 下午4:53写道: >> >> On 1/21/20 9:24 PM, Ian Jiang wrote: >>> For FMV.W.X that moves the lower 32 bits of an integer register to a >>> floating-point register, Rd should encoded with NoN-boxing scheme. >>> Note: This applies to RV64 only. >>> >>> Signed-off-by: Ian Jiang <ianjiang.ict@gmail.com> >>> --- >>> target/riscv/insn_trans/trans_rvf.inc.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/target/riscv/insn_trans/trans_rvf.inc.c b/target/riscv/insn_trans/trans_rvf.inc.c >>> index 172dbfa919..62b7a36567 100644 >>> --- a/target/riscv/insn_trans/trans_rvf.inc.c >>> +++ b/target/riscv/insn_trans/trans_rvf.inc.c >>> @@ -368,6 +368,7 @@ static bool trans_fmv_w_x(DisasContext *ctx, arg_fmv_w_x *a) >>> >>> #if defined(TARGET_RISCV64) >>> tcg_gen_mov_i64(cpu_fpr[a->rd], t0); >>> + tcg_gen_ori_i64(cpu_fpr[a->rd], cpu_fpr[a->rd], 0xffffffff00000000ULL); >>> #else >>> tcg_gen_extu_i32_i64(cpu_fpr[a->rd], t0); >>> #endif >>> >> >> This doesn't look right. There's nothing in the spec that says the nan-boxing >> is restricted to rv64. NaN-boxing is all about FLEN, not XLEN. >> > > Why the translation of FLW has a NaN-boxing? > > file ./target/riscv/insn_trans/trans_rvf.inc.c > 26 static bool trans_flw(DisasContext *ctx, arg_flw *a) > 27 { > ... > 34 tcg_gen_qemu_ld_i64(cpu_fpr[a->rd], t0, ctx->mem_idx, MO_TEUL); > 35 /* RISC-V requires NaN-boxing of narrower width floating point values */ > 36 tcg_gen_ori_i64(cpu_fpr[a->rd], cpu_fpr[a->rd], 0xffffffff00000000ULL); > ... > 41 } Eh? Obviously because we're loading a 32-bit value into a FLEN=64 bit register. (Except when we've selected a cpu without RVD, I suppose, but in that case this nan-boxing is both harmless and invisible.) r~
Richard Henderson <richard.henderson@linaro.org> 于2020年1月23日周四 上午1:11写道: > > On 1/21/20 11:53 PM, Ian Jiang wrote: > > -- > > Ian Jiang > > > > Richard Henderson <richard.henderson@linaro.org> 于2020年1月22日周三 下午4:53写道: > >> > >> On 1/21/20 9:24 PM, Ian Jiang wrote: > >>> For FMV.W.X that moves the lower 32 bits of an integer register to a > >>> floating-point register, Rd should encoded with NoN-boxing scheme. > >>> Note: This applies to RV64 only. > >>> > >>> Signed-off-by: Ian Jiang <ianjiang.ict@gmail.com> > >>> --- > >>> target/riscv/insn_trans/trans_rvf.inc.c | 1 + > >>> 1 file changed, 1 insertion(+) > >>> > >>> diff --git a/target/riscv/insn_trans/trans_rvf.inc.c b/target/riscv/insn_trans/trans_rvf.inc.c > >>> index 172dbfa919..62b7a36567 100644 > >>> --- a/target/riscv/insn_trans/trans_rvf.inc.c > >>> +++ b/target/riscv/insn_trans/trans_rvf.inc.c > >>> @@ -368,6 +368,7 @@ static bool trans_fmv_w_x(DisasContext *ctx, arg_fmv_w_x *a) > >>> > >>> #if defined(TARGET_RISCV64) > >>> tcg_gen_mov_i64(cpu_fpr[a->rd], t0); > >>> + tcg_gen_ori_i64(cpu_fpr[a->rd], cpu_fpr[a->rd], 0xffffffff00000000ULL); > >>> #else > >>> tcg_gen_extu_i32_i64(cpu_fpr[a->rd], t0); > >>> #endif > >>> > >> > >> This doesn't look right. There's nothing in the spec that says the nan-boxing > >> is restricted to rv64. NaN-boxing is all about FLEN, not XLEN. > >> > > > > Why the translation of FLW has a NaN-boxing? > > > > file ./target/riscv/insn_trans/trans_rvf.inc.c > > 26 static bool trans_flw(DisasContext *ctx, arg_flw *a) > > 27 { > > ... > > 34 tcg_gen_qemu_ld_i64(cpu_fpr[a->rd], t0, ctx->mem_idx, MO_TEUL); > > 35 /* RISC-V requires NaN-boxing of narrower width floating point values */ > > 36 tcg_gen_ori_i64(cpu_fpr[a->rd], cpu_fpr[a->rd], 0xffffffff00000000ULL); > > ... > > 41 } > > Eh? Obviously because we're loading a 32-bit value into a FLEN=64 bit register. FMV.W.X is the same with FLW at this point that filling a 64 bits float register based on 32 bits value. Besides, the RISCV simulator Spike makes NaN-boxing for FLW, FMV.W.W, FADD.S, FSUB.S, and others. It might be better that they have a coincident behavior? I am not sure about this and just want a discussion. > (Except when we've selected a cpu without RVD, I suppose, but in that case this > nan-boxing is both harmless and invisible.) > > > r~
On 1/22/20 2:48 PM, Ian Jiang wrote: > Richard Henderson <richard.henderson@linaro.org> 于2020年1月23日周四 上午1:11写道: >> >> On 1/21/20 11:53 PM, Ian Jiang wrote: >>> -- >>> Ian Jiang >>> >>> Richard Henderson <richard.henderson@linaro.org> 于2020年1月22日周三 下午4:53写道: >>>> >>>> On 1/21/20 9:24 PM, Ian Jiang wrote: >>>>> For FMV.W.X that moves the lower 32 bits of an integer register to a >>>>> floating-point register, Rd should encoded with NoN-boxing scheme. >>>>> Note: This applies to RV64 only. >>>>> >>>>> Signed-off-by: Ian Jiang <ianjiang.ict@gmail.com> >>>>> --- >>>>> target/riscv/insn_trans/trans_rvf.inc.c | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/target/riscv/insn_trans/trans_rvf.inc.c b/target/riscv/insn_trans/trans_rvf.inc.c >>>>> index 172dbfa919..62b7a36567 100644 >>>>> --- a/target/riscv/insn_trans/trans_rvf.inc.c >>>>> +++ b/target/riscv/insn_trans/trans_rvf.inc.c >>>>> @@ -368,6 +368,7 @@ static bool trans_fmv_w_x(DisasContext *ctx, arg_fmv_w_x *a) >>>>> >>>>> #if defined(TARGET_RISCV64) >>>>> tcg_gen_mov_i64(cpu_fpr[a->rd], t0); >>>>> + tcg_gen_ori_i64(cpu_fpr[a->rd], cpu_fpr[a->rd], 0xffffffff00000000ULL); >>>>> #else >>>>> tcg_gen_extu_i32_i64(cpu_fpr[a->rd], t0); >>>>> #endif >>>>> >>>> >>>> This doesn't look right. There's nothing in the spec that says the nan-boxing >>>> is restricted to rv64. NaN-boxing is all about FLEN, not XLEN. >>>> >>> >>> Why the translation of FLW has a NaN-boxing? >>> >>> file ./target/riscv/insn_trans/trans_rvf.inc.c >>> 26 static bool trans_flw(DisasContext *ctx, arg_flw *a) >>> 27 { >>> ... >>> 34 tcg_gen_qemu_ld_i64(cpu_fpr[a->rd], t0, ctx->mem_idx, MO_TEUL); >>> 35 /* RISC-V requires NaN-boxing of narrower width floating point values */ >>> 36 tcg_gen_ori_i64(cpu_fpr[a->rd], cpu_fpr[a->rd], 0xffffffff00000000ULL); >>> ... >>> 41 } >> >> Eh? Obviously because we're loading a 32-bit value into a FLEN=64 bit register. > FMV.W.X is the same with FLW at this point that filling a 64 bits > float register based on 32 bits value. > Besides, the RISCV simulator Spike makes NaN-boxing for FLW, FMV.W.W, > FADD.S, FSUB.S, and others. > It might be better that they have a coincident behavior? I am not sure > about this and just want a discussion. So, I wonder if we're talking past one another. In reply to the patch at the beginning of this thread, I was suggesting that the correct solution is #if defined(TARGET_RISCV64) tcg_gen_mov_i64(cpu_fpr[a->rd], t0); #else tcg_gen_extu_i32_i64(cpu_fpr[a->rd], t0); #endif + tcg_gen_ori_i64(cpu_fpr[a->rd], cpu_fpr[a->rd], + 0xffffffff00000000ULL); However, after the previous follow-up I wonder if it might be better to split out the nan-boxing to a helper: /* * RISC-V requires NaN-boxing of narrower width floating * point values. This applies when a 32-bit value is * assigned to a 64-bit FP register. Thus this does not * apply when the RVD extension is not present. */ static void gen_nanbox_fpr(DisasContext *ctx, int regno) { if (has_ext(ctx, RVD)) { tcg_gen_ori_i64(cpu_fpr[regno], cpu_fpr[regno], MAKE_64BIT_MASK(32, 32)); } } which could elide this when only RVF is enabled, and thus the upper 32-bits of the register are inaccessible. This helper would then be propagated to the existing uses within translate.c and insn_trans/trans_rvf.inc.c. As another cleanup, the ifdef above may be replaced with tcg_gen_extu_tl_i64(cpu_fpr[a->rd], t0); which will handle both RISCV64 and RISCV32. r~
Richard Henderson <richard.henderson@linaro.org> 于2020年1月23日周四 上午11:07写道: > > On 1/22/20 2:48 PM, Ian Jiang wrote: > > Richard Henderson <richard.henderson@linaro.org> 于2020年1月23日周四 上午1:11写道: > >> > >> On 1/21/20 11:53 PM, Ian Jiang wrote: > >>> -- > >>> Ian Jiang > >>> > >>> Richard Henderson <richard.henderson@linaro.org> 于2020年1月22日周三 下午4:53写道: > >>>> > >>>> On 1/21/20 9:24 PM, Ian Jiang wrote: > >>>>> For FMV.W.X that moves the lower 32 bits of an integer register to a > >>>>> floating-point register, Rd should encoded with NoN-boxing scheme. > >>>>> Note: This applies to RV64 only. > >>>>> > >>>>> Signed-off-by: Ian Jiang <ianjiang.ict@gmail.com> > >>>>> --- > >>>>> target/riscv/insn_trans/trans_rvf.inc.c | 1 + > >>>>> 1 file changed, 1 insertion(+) > >>>>> > >>>>> diff --git a/target/riscv/insn_trans/trans_rvf.inc.c b/target/riscv/insn_trans/trans_rvf.inc.c > >>>>> index 172dbfa919..62b7a36567 100644 > >>>>> --- a/target/riscv/insn_trans/trans_rvf.inc.c > >>>>> +++ b/target/riscv/insn_trans/trans_rvf.inc.c > >>>>> @@ -368,6 +368,7 @@ static bool trans_fmv_w_x(DisasContext *ctx, arg_fmv_w_x *a) > >>>>> > >>>>> #if defined(TARGET_RISCV64) > >>>>> tcg_gen_mov_i64(cpu_fpr[a->rd], t0); > >>>>> + tcg_gen_ori_i64(cpu_fpr[a->rd], cpu_fpr[a->rd], 0xffffffff00000000ULL); > >>>>> #else > >>>>> tcg_gen_extu_i32_i64(cpu_fpr[a->rd], t0); > >>>>> #endif > >>>>> > >>>> > >>>> This doesn't look right. There's nothing in the spec that says the nan-boxing > >>>> is restricted to rv64. NaN-boxing is all about FLEN, not XLEN. > >>>> > >>> > >>> Why the translation of FLW has a NaN-boxing? > >>> > >>> file ./target/riscv/insn_trans/trans_rvf.inc.c > >>> 26 static bool trans_flw(DisasContext *ctx, arg_flw *a) > >>> 27 { > >>> ... > >>> 34 tcg_gen_qemu_ld_i64(cpu_fpr[a->rd], t0, ctx->mem_idx, MO_TEUL); > >>> 35 /* RISC-V requires NaN-boxing of narrower width floating point values */ > >>> 36 tcg_gen_ori_i64(cpu_fpr[a->rd], cpu_fpr[a->rd], 0xffffffff00000000ULL); > >>> ... > >>> 41 } > >> > >> Eh? Obviously because we're loading a 32-bit value into a FLEN=64 bit register. > > FMV.W.X is the same with FLW at this point that filling a 64 bits > > float register based on 32 bits value. > > Besides, the RISCV simulator Spike makes NaN-boxing for FLW, FMV.W.W, > > FADD.S, FSUB.S, and others. > > It might be better that they have a coincident behavior? I am not sure > > about this and just want a discussion. > > So, I wonder if we're talking past one another. > > In reply to the patch at the beginning of this thread, I was suggesting that > the correct solution is > > #if defined(TARGET_RISCV64) > tcg_gen_mov_i64(cpu_fpr[a->rd], t0); > #else > tcg_gen_extu_i32_i64(cpu_fpr[a->rd], t0); > #endif > + tcg_gen_ori_i64(cpu_fpr[a->rd], cpu_fpr[a->rd], > + 0xffffffff00000000ULL); > > However, after the previous follow-up I wonder if it might be better to split > out the nan-boxing to a helper: > > /* > * RISC-V requires NaN-boxing of narrower width floating > * point values. This applies when a 32-bit value is > * assigned to a 64-bit FP register. Thus this does not > * apply when the RVD extension is not present. > */ > static void gen_nanbox_fpr(DisasContext *ctx, int regno) > { > if (has_ext(ctx, RVD)) { > tcg_gen_ori_i64(cpu_fpr[regno], cpu_fpr[regno], > MAKE_64BIT_MASK(32, 32)); > } > } > > which could elide this when only RVF is enabled, and thus > the upper 32-bits of the register are inaccessible. > > This helper would then be propagated to the existing uses within translate.c > and insn_trans/trans_rvf.inc.c. > > As another cleanup, the ifdef above may be replaced with > > tcg_gen_extu_tl_i64(cpu_fpr[a->rd], t0); > > which will handle both RISCV64 and RISCV32. Good point. But I am not clear where to call this new helper gen_nanbox_fpr(). Is there a position that could affect all floating-point instructions? So that we don't have to modify so many translating functions. Please give more details.
On 1/22/20 6:05 PM, Ian Jiang wrote: > But I am not clear where to call this new helper gen_nanbox_fpr(). Is > there a position that could affect all floating-point instructions? So > that we don't have to modify so many translating functions. Please > give more details. No, this will have to be called for each instruction individually. That said, all of the insns that use helper functions, such as fsqrt_s, should be doing the nan-boxing within the helper function. Thus you'll want a different helper function for use within fpu_helper.c. r~
Got it. As the first step, I just summit a new patch: [PATCH] riscv: Add helper to make NaN-boxing for FP register I'd like to carry out other fixes after this patch is reviewed. Richard Henderson <richard.henderson@linaro.org> 于2020年1月24日周五 上午2:53写道: > > On 1/22/20 6:05 PM, Ian Jiang wrote: > > But I am not clear where to call this new helper gen_nanbox_fpr(). Is > > there a position that could affect all floating-point instructions? So > > that we don't have to modify so many translating functions. Please > > give more details. > > No, this will have to be called for each instruction individually. > > That said, all of the insns that use helper functions, such as fsqrt_s, should > be doing the nan-boxing within the helper function. Thus you'll want a > different helper function for use within fpu_helper.c. > > > r~
© 2016 - 2024 Red Hat, Inc.