| Message ID | 20240723013012.1443-1-zhiwei_liu@linux.alibaba.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] target/riscv: Remove redundant insn length check for zama16b | expand |
On 7/23/24 11:30, LIU Zhiwei wrote: > Both trans_fld/fsd/flw/fsw and gen_load/store will never be a > translation function for compressed instructions, thus we can > remove instruction length check for them. > > Suggested-by: Alistair Francis <alistair.francis@wdc.com> > Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> That is both false (trans_fld is used from trans_c_fld), and not the takeaway you should have gotten (the operation of "fld" should not depend on the encoding). Perhaps FLD/FSD should depend on the ISA (RV32 vs RV64), but perhaps not. I cannot tell because I don't see a specification for Zama16b in https://wiki.riscv.org/display/HOME/RISC-V+Specification+Status r~
On Tue, Jul 23, 2024 at 12:13 PM Richard Henderson <richard.henderson@linaro.org> wrote: > > On 7/23/24 11:30, LIU Zhiwei wrote: > > Both trans_fld/fsd/flw/fsw and gen_load/store will never be a > > translation function for compressed instructions, thus we can > > remove instruction length check for them. > > > > Suggested-by: Alistair Francis <alistair.francis@wdc.com> > > Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> > > That is both false (trans_fld is used from trans_c_fld), and not the takeaway you should > have gotten (the operation of "fld" should not depend on the encoding). > > Perhaps FLD/FSD should depend on the ISA (RV32 vs RV64), but perhaps not. I cannot tell > because I don't see a specification for Zama16b in It doesn't seem to depend on the ISA xlen. The whole spec is just a simple one liner. Apparently that is an extension https://github.com/riscv/riscv-profiles/blob/1fe6f65f130c219c761142e74742d2409c173c40/src/rva23-profile.adoc?plain=1#L176 Alistair > > https://wiki.riscv.org/display/HOME/RISC-V+Specification+Status > > > r~ >
On 2024/7/23 10:11, Richard Henderson wrote: > On 7/23/24 11:30, LIU Zhiwei wrote: >> Both trans_fld/fsd/flw/fsw and gen_load/store will never be a >> translation function for compressed instructions, thus we can >> remove instruction length check for them. >> >> Suggested-by: Alistair Francis <alistair.francis@wdc.com> >> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> > > That is both false (trans_fld is used from trans_c_fld), Yes. I will fix it later. > and not the takeaway you should have gotten (the operation of "fld" > should not depend on the encoding Do you mean c_fld 和fld both applies to zama16b? > ). > > Perhaps FLD/FSD should depend on the ISA (RV32 vs RV64), but perhaps > not. I cannot tell because I don't see a specification for Zama16b in > > https://wiki.riscv.org/display/HOME/RISC-V+Specification+Status I think Zama16b first named in RVA23 profile as Alistair points out. The more detailed information about its meaning is in priviledged 1.13 specification. More exactly, in 3.6.4. Misaligned Atomicity Granule PMA. The specification said: "The misaligned atomicity granule PMA applies only to AMOs, loads and stores defined in the base ISAs, and loads and stores of no more than MXLEN bits defined in the F, D, and Q extensions. For an instruction in that set, if all accessed bytes lie within the same misaligned atomicity granule, the instruction will not raise an exception for reasons of address alignment, and the instruction will give rise to only one memory operation for the purposes of RVWMO—i.e., it will execute atomically." That's the reason why I do not apply zama16b to compressed instructions. Thanks, Zhiwei > > > r~
On 7/23/24 15:29, LIU Zhiwei wrote: > The more detailed information about its meaning is in priviledged 1.13 specification. More > exactly, in 3.6.4. Misaligned Atomicity Granule PMA. > > The specification said: > > "The misaligned atomicity granule PMA applies only to AMOs, loads and stores defined in the base > ISAs, and loads and stores of no more than MXLEN bits defined in the F, D, and Q extensions. For an > instruction in that set, if all accessed bytes lie within the same misaligned atomicity granule, the > instruction will not raise an exception for reasons of address alignment, and the instruction will give > rise to only one memory operation for the purposes of RVWMO—i.e., it will execute atomically." > > That's the reason why I do not apply zama16b to compressed instructions. Given the non-specificity of this paragraph, I think not specifically calling out compressed forms of the base ISA is simply a documentation error. In general, the compressed ISA is supposed to be a smaller encoding of the exact same instruction as the standard ISA. However! It does explicitly say "no more than MXLEN bits", which means that an RV32/RV64 check is appropriate for FLD/FSD, since MXLEN may be less than 64. In addition, your change for AMOs is incomplete. From the text: If a misaligned AMO accesses a region that does not specify a misaligned atomicity granule PMA, or if not all accessed bytes lie within the same misaligned atomicity granule, then an exception is raised. The second clause corresponds exactly with the Arm FEAT_LSE2. See check_lse2_align in target/arm/tcg/translate-a64.c. r~ PS: The first clause is similar to Arm access to pages marked as Device memory, for which all misaligned accesses trap. I didn't dig deep enough to see how PMAs are defined to suggest how that might be applied.
On 2024/7/23 13:59, Richard Henderson wrote: > On 7/23/24 15:29, LIU Zhiwei wrote: >> The more detailed information about its meaning is in priviledged >> 1.13 specification. More exactly, in 3.6.4. Misaligned Atomicity >> Granule PMA. >> >> The specification said: >> >> "The misaligned atomicity granule PMA applies only to AMOs, loads and >> stores defined in the base >> ISAs, and loads and stores of no more than MXLEN bits defined in the >> F, D, and Q extensions. For an >> instruction in that set, if all accessed bytes lie within the same >> misaligned atomicity granule, the >> instruction will not raise an exception for reasons of address >> alignment, and the instruction will give >> rise to only one memory operation for the purposes of RVWMO—i.e., it >> will execute atomically." >> >> That's the reason why I do not apply zama16b to compressed instructions. > Given the non-specificity of this paragraph, I think not specifically > calling out compressed forms of the base ISA is simply a documentation > error. In general, the compressed ISA is supposed to be a smaller > encoding of the exact same instruction as the standard ISA. I will confirm this with the RISC-V community. Thanks. > > However! It does explicitly say "no more than MXLEN bits", which > means that an RV32/RV64 check is appropriate for FLD/FSD, since MXLEN > may be less than 64. Yes. That's true. Although I don't know why MXLEN is needed as F, D or Q don't depend on MXLEN. We can implement D extension on RV32 CPU. > > In addition, your change for AMOs is incomplete. From the text: > > If a misaligned AMO accesses a region that does not specify a > misaligned > atomicity granule PMA, or if not all accessed bytes lie within the same > misaligned atomicity granule, then an exception is raised. > > The second clause corresponds exactly with the Arm FEAT_LSE2. > See check_lse2_align in target/arm/tcg/translate-a64.c. > > > r~ > > > PS: The first clause is similar to Arm access to pages marked as > Device memory, for which all misaligned accesses trap. I didn't dig > deep enough to see how PMAs are defined to suggest how that might be > applied. It's more complex than I once thought. I will do more work before sending next version. Thanks, Zhiwei
On 2024/7/23 13:59, Richard Henderson wrote: > On 7/23/24 15:29, LIU Zhiwei wrote: >> The more detailed information about its meaning is in priviledged >> 1.13 specification. More exactly, in 3.6.4. Misaligned Atomicity >> Granule PMA. >> >> The specification said: >> >> "The misaligned atomicity granule PMA applies only to AMOs, loads and >> stores defined in the base >> ISAs, and loads and stores of no more than MXLEN bits defined in the >> F, D, and Q extensions. For an >> instruction in that set, if all accessed bytes lie within the same >> misaligned atomicity granule, the >> instruction will not raise an exception for reasons of address >> alignment, and the instruction will give >> rise to only one memory operation for the purposes of RVWMO—i.e., it >> will execute atomically." >> >> That's the reason why I do not apply zama16b to compressed instructions. > Given the non-specificity of this paragraph, I think not specifically > calling out compressed forms of the base ISA is simply a documentation > error. In general, the compressed ISA is supposed to be a smaller > encoding of the exact same instruction as the standard ISA. Yes, it's a documentation error. We will fix in the specification. https://github.com/riscv/riscv-isa-manual/pull/1557 Zhiwei > > However! It does explicitly say "no more than MXLEN bits", which > means that an RV32/RV64 check is appropriate for FLD/FSD, since MXLEN > may be less than 64. > > In addition, your change for AMOs is incomplete. From the text: > > If a misaligned AMO accesses a region that does not specify a > misaligned > atomicity granule PMA, or if not all accessed bytes lie within the same > misaligned atomicity granule, then an exception is raised. > > The second clause corresponds exactly with the Arm FEAT_LSE2. > See check_lse2_align in target/arm/tcg/translate-a64.c. > > > r~ > > > PS: The first clause is similar to Arm access to pages marked as > Device memory, for which all misaligned accesses trap. I didn't dig > deep enough to see how PMAs are defined to suggest how that might be > applied.
On 7/24/24 16:32, LIU Zhiwei wrote: >> However! It does explicitly say "no more than MXLEN bits", which means that an RV32/ >> RV64 check is appropriate for FLD/FSD, since MXLEN may be less than 64. > Yes. That's true. Although I don't know why MXLEN is needed as F, D or Q don't depend on > MXLEN. We can implement D extension on RV32 CPU. Implementation of D on RV32 does not mean that FLD/FST is atomic -- it can be implemented with two 32-bit loads/stores under the hood. r~
On Thu, Jul 25, 2024 at 11:53 AM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote: > > > On 2024/7/23 13:59, Richard Henderson wrote: > > On 7/23/24 15:29, LIU Zhiwei wrote: > >> The more detailed information about its meaning is in priviledged > >> 1.13 specification. More exactly, in 3.6.4. Misaligned Atomicity > >> Granule PMA. > >> > >> The specification said: > >> > >> "The misaligned atomicity granule PMA applies only to AMOs, loads and > >> stores defined in the base > >> ISAs, and loads and stores of no more than MXLEN bits defined in the > >> F, D, and Q extensions. For an > >> instruction in that set, if all accessed bytes lie within the same > >> misaligned atomicity granule, the > >> instruction will not raise an exception for reasons of address > >> alignment, and the instruction will give > >> rise to only one memory operation for the purposes of RVWMO—i.e., it > >> will execute atomically." > >> > >> That's the reason why I do not apply zama16b to compressed instructions. > > Given the non-specificity of this paragraph, I think not specifically > > calling out compressed forms of the base ISA is simply a documentation > > error. In general, the compressed ISA is supposed to be a smaller > > encoding of the exact same instruction as the standard ISA. > > Yes, it's a documentation error. We will fix in the specification. > > https://github.com/riscv/riscv-isa-manual/pull/1557 Thanks for getting that clarified What's the status of a followup patch? We should fix this before the release Alistair
On 2024/7/31 17:38, Alistair Francis wrote: > On Thu, Jul 25, 2024 at 11:53 AM LIU Zhiwei > <zhiwei_liu@linux.alibaba.com> wrote: >> >> On 2024/7/23 13:59, Richard Henderson wrote: >>> On 7/23/24 15:29, LIU Zhiwei wrote: >>>> The more detailed information about its meaning is in priviledged >>>> 1.13 specification. More exactly, in 3.6.4. Misaligned Atomicity >>>> Granule PMA. >>>> >>>> The specification said: >>>> >>>> "The misaligned atomicity granule PMA applies only to AMOs, loads and >>>> stores defined in the base >>>> ISAs, and loads and stores of no more than MXLEN bits defined in the >>>> F, D, and Q extensions. For an >>>> instruction in that set, if all accessed bytes lie within the same >>>> misaligned atomicity granule, the >>>> instruction will not raise an exception for reasons of address >>>> alignment, and the instruction will give >>>> rise to only one memory operation for the purposes of RVWMO—i.e., it >>>> will execute atomically." >>>> >>>> That's the reason why I do not apply zama16b to compressed instructions. >>> Given the non-specificity of this paragraph, I think not specifically >>> calling out compressed forms of the base ISA is simply a documentation >>> error. In general, the compressed ISA is supposed to be a smaller >>> encoding of the exact same instruction as the standard ISA. >> Yes, it's a documentation error. We will fix in the specification. >> >> https://github.com/riscv/riscv-isa-manual/pull/1557 > Thanks for getting that clarified > > What's the status of a followup patch? We should fix this before the release I will send the follow patch as soon as possible. And resolve another comment Richard gives later as it need some work on PBMT(I think). Thanks, Zhiwei > > Alistair
diff --git a/target/riscv/insn_trans/trans_rvd.c.inc b/target/riscv/insn_trans/trans_rvd.c.inc index 1f5fac65a2..0ac42c3223 100644 --- a/target/riscv/insn_trans/trans_rvd.c.inc +++ b/target/riscv/insn_trans/trans_rvd.c.inc @@ -47,7 +47,7 @@ static bool trans_fld(DisasContext *ctx, arg_fld *a) REQUIRE_FPU; REQUIRE_EXT(ctx, RVD); - if (ctx->cfg_ptr->ext_zama16b && (ctx->cur_insn_len != 2)) { + if (ctx->cfg_ptr->ext_zama16b) { memop |= MO_ATOM_WITHIN16; } @@ -67,7 +67,7 @@ static bool trans_fsd(DisasContext *ctx, arg_fsd *a) REQUIRE_FPU; REQUIRE_EXT(ctx, RVD); - if (ctx->cfg_ptr->ext_zama16b && (ctx->cur_insn_len != 2)) { + if (ctx->cfg_ptr->ext_zama16b) { memop |= MO_ATOM_WITHIN16; } diff --git a/target/riscv/insn_trans/trans_rvf.c.inc b/target/riscv/insn_trans/trans_rvf.c.inc index f771aa1939..0222a728df 100644 --- a/target/riscv/insn_trans/trans_rvf.c.inc +++ b/target/riscv/insn_trans/trans_rvf.c.inc @@ -48,7 +48,7 @@ static bool trans_flw(DisasContext *ctx, arg_flw *a) REQUIRE_FPU; REQUIRE_EXT(ctx, RVF); - if (ctx->cfg_ptr->ext_zama16b && (ctx->cur_insn_len != 2)) { + if (ctx->cfg_ptr->ext_zama16b) { memop |= MO_ATOM_WITHIN16; } @@ -70,7 +70,7 @@ static bool trans_fsw(DisasContext *ctx, arg_fsw *a) REQUIRE_FPU; REQUIRE_EXT(ctx, RVF); - if (ctx->cfg_ptr->ext_zama16b && (ctx->cur_insn_len != 2)) { + if (ctx->cfg_ptr->ext_zama16b) { memop |= MO_ATOM_WITHIN16; } diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc index 98e3806d5e..fab5c06719 100644 --- a/target/riscv/insn_trans/trans_rvi.c.inc +++ b/target/riscv/insn_trans/trans_rvi.c.inc @@ -268,7 +268,7 @@ static bool gen_load(DisasContext *ctx, arg_lb *a, MemOp memop) { bool out; - if (ctx->cfg_ptr->ext_zama16b && (ctx->cur_insn_len != 2)) { + if (ctx->cfg_ptr->ext_zama16b) { memop |= MO_ATOM_WITHIN16; } decode_save_opc(ctx); @@ -369,7 +369,7 @@ static bool gen_store_i128(DisasContext *ctx, arg_sb *a, MemOp memop) static bool gen_store(DisasContext *ctx, arg_sb *a, MemOp memop) { - if (ctx->cfg_ptr->ext_zama16b && (ctx->cur_insn_len != 2)) { + if (ctx->cfg_ptr->ext_zama16b) { memop |= MO_ATOM_WITHIN16; } decode_save_opc(ctx);
Both trans_fld/fsd/flw/fsw and gen_load/store will never be a translation function for compressed instructions, thus we can remove instruction length check for them. Suggested-by: Alistair Francis <alistair.francis@wdc.com> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> --- target/riscv/insn_trans/trans_rvd.c.inc | 4 ++-- target/riscv/insn_trans/trans_rvf.c.inc | 4 ++-- target/riscv/insn_trans/trans_rvi.c.inc | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-)