[PATCH 1/1] target/riscv: Remove redundant insn length check for zama16b

[PATCH 1/1] target/riscv: Remove redundant insn length check for zama16b

[LIU Zhiwei]

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(-)

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);
-- 
2.25.1

Re: [PATCH 1/1] target/riscv: Remove redundant insn length check for zama16b

[Richard Henderson]

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~

Re: [PATCH 1/1] target/riscv: Remove redundant insn length check for zama16b

[Alistair Francis]

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~
>

Re: [PATCH 1/1] target/riscv: Remove redundant insn length check for zama16b

[LIU Zhiwei]

[-- Attachment #1: Type: text/plain, Size: 1690 bytes --]


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~

[-- Attachment #2: Type: text/html, Size: 3048 bytes --]

Re: [PATCH 1/1] target/riscv: Remove redundant insn length check for zama16b

[Richard Henderson]

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.

Re: [PATCH 1/1] target/riscv: Remove redundant insn length check for zama16b

[LIU 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.
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

Re: [PATCH 1/1] target/riscv: Remove redundant insn length check for zama16b

[LIU 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.

Re: [PATCH 1/1] target/riscv: Remove redundant insn length check for zama16b

[Richard Henderson]

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~

Re: [PATCH 1/1] target/riscv: Remove redundant insn length check for zama16b

[Alistair Francis]

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

Re: [PATCH 1/1] target/riscv: Remove redundant insn length check for zama16b

[LIU Zhiwei]

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