[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v4 5/5] target/riscv: add vector amo operations

From: LIU Zhiwei
Subject: Re: [PATCH v4 5/5] target/riscv: add vector amo operations
Date: Sat, 29 Feb 2020 21:16:14 +0800
User-agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.4.2

On 2020/2/29 2:46, Richard Henderson wrote:
On 2/28/20 1:19 AM, LIU Zhiwei wrote:
+static void vext_##NAME##_noatomic_op(void *vs3, target_ulong addr,      \
+        uint32_t wd, uint32_t idx, CPURISCVState *env, uintptr_t retaddr)\
+{                                                                        \
+    ETYPE ret;                                                           \
+    target_ulong tmp;                                                    \
+    int mmu_idx = cpu_mmu_index(env, false);                             \
+    tmp = cpu_ld##SUF##_mmuidx_ra(env, addr, mmu_idx, retaddr);          \
+    ret = DO_OP((ETYPE)(MTYPE)tmp, *((ETYPE *)vs3 + H(idx)));            \
+    cpu_st##SUF##_mmuidx_ra(env, addr, ret, mmu_idx, retaddr);           \
+    if (wd) {                                                            \
+        *((ETYPE *)vs3 + H(idx)) = (target_long)(MTYPE)tmp;              \
The target_long cast is wrong; should be ETYPE.
"If the AMO memory element width is less than SEW, the value returned from 
  is sign-extended to fill SEW"

So just use (target_long) to sign-extended. As you see, instructions like


have the uint64_t as ETYPE.  And it can't sign-extend the value from memory by
Casting to target_long doesn't help -- it becomes signed at a variable size,
possibly larger than MTYPE.

In addition, I think you're performing the operation at the wrong length.
text of the ISA document could be clearer, but

   # If SEW > 32 bits, the value returned from memory
   # is sign-extended to fill SEW.

You are performing the operation in ETYPE, but it should be done in MTYPE and
only afterward extended to ETYPE.
Yes, I  made a mistake.It should be MTYPE.
For minu/maxu, you're right that you need an unsigned for the operation.  But
then you need a signed type of the same width for the extension.

One possibility is to *always* make MTYPE a signed type, but for the two cases
that require an unsigned type, provide it.  E.g.

static void vext_##NAME##_noatomic_op(void *vs3,
     target_ulong addr, uint32_t wd, uint32_t idx,
     CPURISCVState *env, uintptr_t retaddr)
     typedef int##ESZ##_t ETYPE;
     typedef int##MSZ##_t MTYPE;
     typedef uint##MSZ##_t UMTYPE;
     ETYPE *pe3 = (ETYPE *)vs3 + H(idx);
     MTYPE a = *pe3, b = cpu_ld##SUF##_data(env, addr);
     a = DO_OP(a, b);
     cpu_st##SUF##_data(env, addr, a);
     if (wd) {
         *pe3 = a;

/* Signed min/max */
#define DO_MAX(N, M)  ((N) >= (M) ? (N) : (M))
#define DO_MIN(N, M)  ((N) >= (M) ? (M) : (N))

/* Unsigned min/max */

GEN_VEXT_AMO_NOATOMIC_OP(vamomaxuw_v_d, 64, 32, H8, DO_MAXU, l)
GEN_VEXT_AMO_NOATOMIC_OP(vamomaxud_v_d, 64, 64, H8, DO_MAXU, q)
Perfect. I will try it.

The missing aligned address check is the only remaining exception that the
helper_atomic_* functions would raise, since you have properly checked for
read+write.  So it might be possible to get away with using the helpers, but I
don't like it.
Do you mean write my own helpers to implement atomic operations?

What's the meaning of " but I don't like it. "?
I don't like re-using helpers in an incorrect way.

But I do think it would be better to write your own helpers for the atomic
paths.  They need not check quite so much, since we have already done the
validation above.  You pretty much only need to use tlb_vaddr_to_host.

If that gets too ugly, we can talk about rearranging
accel/tcg/atomic_template.h so that it could be reused.
Good idea.  Perhaps use tlb_vaddr_to_host instead of atomic_mmu_lookup
to define another macro like GEN_ATOMIC_HELPER?
Alternately, we could simply *always* use the non-atomic helpers, and raise
exit_atomic if PARALLEL.
Yes, it's the simplest way.
However I prefer try to define something like GEN_ATOMIC_HELPER in
I'll think about this some more.
In the short-term, I think non-atomic is the best we can do.
I will accept your advice. Thanks.

Best Regards,


reply via email to

[Prev in Thread] Current Thread [Next in Thread]