On 2/28/20 1:19 AM, LIU Zhiwei wrote:
+#define GEN_VEXT_AMO_NOATOMIC_OP(NAME, ETYPE, MTYPE, H, DO_OP, SUF) \
+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
memory
is sign-extended to fill SEW"
So just use (target_long) to sign-extended. As you see, instructions like
vamominud
have the uint64_t as ETYPE. And it can't sign-extend the value from memory by
(ETYPE)(MTYPE)tmp.
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.
The
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.
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.
#define GEN_VEXT_AMO_NOATOMIC_OP(NAME, ESZ, MSZ, H, DO_OP, SUF)
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 */
#define DO_MAXU(N, M) DO_MAX((UMTYPE)N, (UMTYPE)M)
#define DO_MINU(N, M) DO_MIN((UMTYPE)N, (UMTYPE)M)
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)
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
vector_helper.c.
I'll think about this some more.
In the short-term, I think non-atomic is the best we can do.