On 12 December 2016 at 06:00, David Gibson <da...@gibson.dropbear.id.au> wrote:
> On Fri, Dec 09, 2016 at 05:47:24PM +0530, Nikunj A Dadhania wrote:
>> xxextractuw: VSX Vector Extract Unsigned Word
>>
>> Signed-off-by: Nikunj A Dadhania <nik...@linux.vnet.ibm.com>
>> ---
>> target-ppc/helper.h | 1 +
>> target-ppc/int_helper.c | 21 +++++++++++++++++++++
>> target-ppc/translate/vsx-impl.inc.c | 27 +++++++++++++++++++++++++++
>> target-ppc/translate/vsx-ops.inc.c | 5 +++++
>> 4 files changed, 54 insertions(+)
>>
>> diff --git a/target-ppc/helper.h b/target-ppc/helper.h
>> index 4707db4..8b30420 100644
>> --- a/target-ppc/helper.h
>> +++ b/target-ppc/helper.h
>> @@ -540,6 +540,7 @@ DEF_HELPER_2(xvrspip, void, env, i32)
>> DEF_HELPER_2(xvrspiz, void, env, i32)
>> DEF_HELPER_2(xxperm, void, env, i32)
>> DEF_HELPER_2(xxpermr, void, env, i32)
>> +DEF_HELPER_4(xxextractuw, void, env, tl, tl, i32)
>>
>> DEF_HELPER_2(efscfsi, i32, env, i32)
>> DEF_HELPER_2(efscfui, i32, env, i32)
>> diff --git a/target-ppc/int_helper.c b/target-ppc/int_helper.c
>> index 7989b1f..e3f66ac 100644
>> --- a/target-ppc/int_helper.c
>> +++ b/target-ppc/int_helper.c
>> @@ -2033,6 +2033,27 @@ VEXTRACT(uw, u32)
>> VEXTRACT(d, u64)
>> #undef VEXTRACT
>>
>> +void helper_xxextractuw(CPUPPCState *env, target_ulong xtn,
>> + target_ulong xbn, uint32_t index)
>> +{
>> + ppc_vsr_t xt, xb;
>> + size_t es = sizeof(uint32_t);
>> + uint32_t ext_index;
>> +
>> + getVSR(xbn, &xb, env);
>> + memset(&xt, 0, sizeof(xt));
>> +
>> +#if defined(HOST_WORDS_BIGENDIAN)
>> + ext_index = index;
>> + memcpy(&xt.u8[8 - es], &xb.u8[ext_index], es);
>> +#else
>> + ext_index = (16 - index) - es;
>> + memcpy(&xt.u8[8], &xb.u8[ext_index], es);
>
> Hm. So, IIUC, ext_index is the byte element - in IBM numbering - to
> start copying from. But I thought that when we have an LE host, the
> IBM byte element ordering is reversed from the actual order in host
> memory, so we'd need &xb.u8[16 - ext_index - es]
I am not getting you, I am getting index from user. So in case of BE host:
ext_index = index;
LE Host:
ext_index = (16 - index) - es;
I am already doing that. Am I missing something.
>
>> +#endif
>> +
>> + putVSR(xtn, &xt, env);
>> +}
>> +
>> #define VEXT_SIGNED(name, element, mask, cast, recast) \
>> void helper_##name(ppc_avr_t *r, ppc_avr_t *b) \
>> { \
>> diff --git a/target-ppc/translate/vsx-impl.inc.c
>> b/target-ppc/translate/vsx-impl.inc.c
>> index 2a17c35..1c40a35 100644
>> --- a/target-ppc/translate/vsx-impl.inc.c
>> +++ b/target-ppc/translate/vsx-impl.inc.c
>> @@ -1180,6 +1180,33 @@ static void gen_xxsldwi(DisasContext *ctx)
>> tcg_temp_free_i64(xtl);
>> }
>>
>> +#define VSX_EXTRACT(name) \
>> +static void gen_##name(DisasContext *ctx) \
>> +{ \
>> + TCGv xt, xb; \
>> + TCGv_i32 t0 = tcg_temp_new_i32(); \
>> + uint8_t uimm = UIMM4(ctx->opcode); \
>> + \
>> + if (unlikely(!ctx->vsx_enabled)) { \
>> + gen_exception(ctx, POWERPC_EXCP_VSXU); \
>> + return; \
>> + } \
>> + if (uimm > 12) { \
>
> Throughout the helper you use es == sizeof(uint32_t), but here you
> hardcode the assumption of 4 bytes, seems a bit inconsistent.
>
>> + tcg_gen_movi_i64(cpu_vsrh(xT(ctx->opcode)), 0); \
>> + tcg_gen_movi_i64(cpu_vsrl(xT(ctx->opcode)), 0); \
>> + return; \
>
> So, I know the architecture says it is undefined. But since you're
> testing for the bogus case anyway, why not turn this into an
> exception. That seems like it would be more helpful for debugging the
> guest than just setting the result to zero. Or is this done to match
> actual hardware behaviour?
I havent had a change to run on the real hardware, but on the system
simulator, it happily
returns extracted content even if UIMM > 12.
Regards
Nikunj
