Re: [Qemu-devel] [PATCH v1 1/9] target-ppc: implement lxvl instruction

2016-12-08 Thread Nikunj A Dadhania
David Gibson  writes:

> [ Unknown signature status ]
> On Wed, Dec 07, 2016 at 11:54:54PM +0530, Nikunj A Dadhania wrote:
>> lxvl: Load VSX Vector with Length
>> 
>> Little/Big-endian Storage:
>> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+--+--+
>> |“T”|“h”|“i”|“s”|“ ”|“i”|“s”|“ ”|“a”|“ ”|“T”|“E”|“S”|“T”|FF|FF|
>> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+--+--+
>> 
>> Loading 14 bytes results in:
>> 
>> Vector (8-bit elements) in BE:
>> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+--+--+
>> |“T”|“h”|“i”|“s”|“ ”|“i”|“s”|“ ”|“a”|“ ”|“T”|“E”|“S”|“T”|00|00|
>> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+--+--+
>> 
>> Vector (8-bit elements) in LE:
>> +--+--+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
>> |00|00|“T”|“S”|“E”|“T”|“ ”|“a”|“ ”|“s”|“i”|“ ”|“s”|“i”|"h"|"T"|
>> +--+--+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
>
> Took a while to wrap my head around the semantics, but I believe this
> is correct.  However, there are a couple of nits:
>
>> 
>> Signed-off-by: Nikunj A Dadhania 
>> ---
>>  target-ppc/helper.h |  1 +
>>  target-ppc/mem_helper.c | 33 +
>>  target-ppc/translate/vsx-impl.inc.c | 27 +++
>>  target-ppc/translate/vsx-ops.inc.c  |  1 +
>>  4 files changed, 62 insertions(+)
>> 
>> diff --git a/target-ppc/helper.h b/target-ppc/helper.h
>> index bc39efb..d9ccafd 100644
>> --- a/target-ppc/helper.h
>> +++ b/target-ppc/helper.h
>> @@ -317,6 +317,7 @@ DEF_HELPER_3(lvewx, void, env, avr, tl)
>>  DEF_HELPER_3(stvebx, void, env, avr, tl)
>>  DEF_HELPER_3(stvehx, void, env, avr, tl)
>>  DEF_HELPER_3(stvewx, void, env, avr, tl)
>> +DEF_HELPER_4(lxvl, void, env, tl, tl, tl)
>>  DEF_HELPER_4(vsumsws, void, env, avr, avr, avr)
>>  DEF_HELPER_4(vsum2sws, void, env, avr, avr, avr)
>>  DEF_HELPER_4(vsum4sbs, void, env, avr, avr, avr)
>> diff --git a/target-ppc/mem_helper.c b/target-ppc/mem_helper.c
>> index 1ab8a6e..54447a7 100644
>> --- a/target-ppc/mem_helper.c
>> +++ b/target-ppc/mem_helper.c
>> @@ -24,6 +24,7 @@
>>  
>>  #include "helper_regs.h"
>>  #include "exec/cpu_ldst.h"
>> +#include "internal.h"
>>  
>>  //#define DEBUG_OP
>>  
>> @@ -284,8 +285,40 @@ STVE(stvewx, cpu_stl_data_ra, bswap32, u32)
>>  #undef I
>>  #undef LVE
>>  
>> +#ifdef TARGET_PPC64
>> +#define GET_NB(rb) ((rb >> 56) & 0xFF)
>> +#else
>> +#define GET_NB(rb) ((rb >> 24) & 0xFF)
>> +#endif
>
> A 32-bit VSX implementation seems... improbable.  Simpler to just
> bracket the whole thing with ifdef TARGET_PPC64.

Sure, can be done, just that the instruction specifically got bit 0:7, i
thought that I shouldn't limit my implementation to just PPC64.


>> +
>> +void helper_lxvl(CPUPPCState *env, target_ulong addr,
>> + target_ulong xt_num, target_ulong rb)
>
> I think it would be nicer to either have two different helpers for the
> LE and BE cases, or take an endian parameter.  That should allow you
> to share the helper with the lxvll implementation.

Something like the below should work:

#define VSX_LXVL(name, lj)  \
void helper_##name(CPUPPCState *env, target_ulong addr, \
   target_ulong xt_num, target_ulong rb)\
{   \
int i;  \
ppc_vsr_t xt;   \
uint64_t nb = GET_NB(rb);   \
\
xt.s128 = int128_zero();\
if (nb) {   \
nb = (nb >= 16) ? 16 : nb;  \
if (msr_le && !lj) {\
for (i = 16; i > 16 - nb; i--) {\
xt.VsrB(i - 1) = cpu_ldub_data_ra(env, addr, GETPC());  \
addr = addr_add(env, addr, 1);  \
}   \
} else {\
for (i = 0; i < nb; i++) {  \
xt.VsrB(i) = cpu_ldub_data_ra(env, addr, GETPC());  \
addr = addr_add(env, addr, 1);  \
}   \
}   \
}   \
putVSR(xt_num, , env);   \
}

VSX_LXVL(lxvl, 0)
VSX_LXVL(lxvll, 1)

Regards
Nikunj




Re: [Qemu-devel] [PATCH v1 1/9] target-ppc: implement lxvl instruction

2016-12-08 Thread David Gibson
On Wed, Dec 07, 2016 at 11:54:54PM +0530, Nikunj A Dadhania wrote:
> lxvl: Load VSX Vector with Length
> 
> Little/Big-endian Storage:
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+--+--+
> |“T”|“h”|“i”|“s”|“ ”|“i”|“s”|“ ”|“a”|“ ”|“T”|“E”|“S”|“T”|FF|FF|
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+--+--+
> 
> Loading 14 bytes results in:
> 
> Vector (8-bit elements) in BE:
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+--+--+
> |“T”|“h”|“i”|“s”|“ ”|“i”|“s”|“ ”|“a”|“ ”|“T”|“E”|“S”|“T”|00|00|
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+--+--+
> 
> Vector (8-bit elements) in LE:
> +--+--+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> |00|00|“T”|“S”|“E”|“T”|“ ”|“a”|“ ”|“s”|“i”|“ ”|“s”|“i”|"h"|"T"|
> +--+--+---+---+---+---+---+---+---+---+---+---+---+---+---+---+

Took a while to wrap my head around the semantics, but I believe this
is correct.  However, there are a couple of nits:

> 
> Signed-off-by: Nikunj A Dadhania 
> ---
>  target-ppc/helper.h |  1 +
>  target-ppc/mem_helper.c | 33 +
>  target-ppc/translate/vsx-impl.inc.c | 27 +++
>  target-ppc/translate/vsx-ops.inc.c  |  1 +
>  4 files changed, 62 insertions(+)
> 
> diff --git a/target-ppc/helper.h b/target-ppc/helper.h
> index bc39efb..d9ccafd 100644
> --- a/target-ppc/helper.h
> +++ b/target-ppc/helper.h
> @@ -317,6 +317,7 @@ DEF_HELPER_3(lvewx, void, env, avr, tl)
>  DEF_HELPER_3(stvebx, void, env, avr, tl)
>  DEF_HELPER_3(stvehx, void, env, avr, tl)
>  DEF_HELPER_3(stvewx, void, env, avr, tl)
> +DEF_HELPER_4(lxvl, void, env, tl, tl, tl)
>  DEF_HELPER_4(vsumsws, void, env, avr, avr, avr)
>  DEF_HELPER_4(vsum2sws, void, env, avr, avr, avr)
>  DEF_HELPER_4(vsum4sbs, void, env, avr, avr, avr)
> diff --git a/target-ppc/mem_helper.c b/target-ppc/mem_helper.c
> index 1ab8a6e..54447a7 100644
> --- a/target-ppc/mem_helper.c
> +++ b/target-ppc/mem_helper.c
> @@ -24,6 +24,7 @@
>  
>  #include "helper_regs.h"
>  #include "exec/cpu_ldst.h"
> +#include "internal.h"
>  
>  //#define DEBUG_OP
>  
> @@ -284,8 +285,40 @@ STVE(stvewx, cpu_stl_data_ra, bswap32, u32)
>  #undef I
>  #undef LVE
>  
> +#ifdef TARGET_PPC64
> +#define GET_NB(rb) ((rb >> 56) & 0xFF)
> +#else
> +#define GET_NB(rb) ((rb >> 24) & 0xFF)
> +#endif

A 32-bit VSX implementation seems... improbable.  Simpler to just
bracket the whole thing with ifdef TARGET_PPC64.

> +
> +void helper_lxvl(CPUPPCState *env, target_ulong addr,
> + target_ulong xt_num, target_ulong rb)

I think it would be nicer to either have two different helpers for the
LE and BE cases, or take an endian parameter.  That should allow you
to share the helper with the lxvll implementation.

> +{
> +int i;
> +ppc_vsr_t xt;
> +uint64_t nb = GET_NB(rb);
> +
> +xt.s128 = int128_zero();
> +if (nb) {
> +nb = (nb >= 16) ? 16 : nb;
> +if (msr_le) {
> +for (i = 16; i > 16 - nb; i--) {
> +xt.VsrB(i - 1) = cpu_ldub_data_ra(env, addr, GETPC());
> +addr = addr_add(env, addr, 1);
> +}
> +} else {
> +for (i = 0; i < nb; i++) {
> +xt.VsrB(i) = cpu_ldub_data_ra(env, addr, GETPC());
> +addr = addr_add(env, addr, 1);
> +}
> +}
> +}
> +putVSR(xt_num, , env);
> +}
> +
>  #undef HI_IDX
>  #undef LO_IDX
> +#undef GET_NB
>  
>  void helper_tbegin(CPUPPCState *env)
>  {
> diff --git a/target-ppc/translate/vsx-impl.inc.c 
> b/target-ppc/translate/vsx-impl.inc.c
> index 808ee48..6bd70a0 100644
> --- a/target-ppc/translate/vsx-impl.inc.c
> +++ b/target-ppc/translate/vsx-impl.inc.c
> @@ -240,6 +240,33 @@ VSX_VECTOR_LOAD_STORE(stxv, st_i64, 0)
>  VSX_VECTOR_LOAD_STORE(lxvx, ld_i64, 1)
>  VSX_VECTOR_LOAD_STORE(stxvx, st_i64, 1)
>  
> +#define VSX_VECTOR_LOAD_STORE_LENGTH(name)  \
> +static void gen_##name(DisasContext *ctx)   \
> +{   \
> +TCGv EA, xt;\
> +\
> +if (xT(ctx->opcode) < 32) { \
> +if (unlikely(!ctx->vsx_enabled)) {  \
> +gen_exception(ctx, POWERPC_EXCP_VSXU);  \
> +return; \
> +}   \
> +} else {\
> +if (unlikely(!ctx->altivec_enabled)) {  \
> +gen_exception(ctx, POWERPC_EXCP_VPU);   \
> +return; \
> +}   \
> +}   

[Qemu-devel] [PATCH v1 1/9] target-ppc: implement lxvl instruction

2016-12-07 Thread Nikunj A Dadhania
lxvl: Load VSX Vector with Length

Little/Big-endian Storage:
+---+---+---+---+---+---+---+---+---+---+---+---+---+---+--+--+
|“T”|“h”|“i”|“s”|“ ”|“i”|“s”|“ ”|“a”|“ ”|“T”|“E”|“S”|“T”|FF|FF|
+---+---+---+---+---+---+---+---+---+---+---+---+---+---+--+--+

Loading 14 bytes results in:

Vector (8-bit elements) in BE:
+---+---+---+---+---+---+---+---+---+---+---+---+---+---+--+--+
|“T”|“h”|“i”|“s”|“ ”|“i”|“s”|“ ”|“a”|“ ”|“T”|“E”|“S”|“T”|00|00|
+---+---+---+---+---+---+---+---+---+---+---+---+---+---+--+--+

Vector (8-bit elements) in LE:
+--+--+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
|00|00|“T”|“S”|“E”|“T”|“ ”|“a”|“ ”|“s”|“i”|“ ”|“s”|“i”|"h"|"T"|
+--+--+---+---+---+---+---+---+---+---+---+---+---+---+---+---+

Signed-off-by: Nikunj A Dadhania 
---
 target-ppc/helper.h |  1 +
 target-ppc/mem_helper.c | 33 +
 target-ppc/translate/vsx-impl.inc.c | 27 +++
 target-ppc/translate/vsx-ops.inc.c  |  1 +
 4 files changed, 62 insertions(+)

diff --git a/target-ppc/helper.h b/target-ppc/helper.h
index bc39efb..d9ccafd 100644
--- a/target-ppc/helper.h
+++ b/target-ppc/helper.h
@@ -317,6 +317,7 @@ DEF_HELPER_3(lvewx, void, env, avr, tl)
 DEF_HELPER_3(stvebx, void, env, avr, tl)
 DEF_HELPER_3(stvehx, void, env, avr, tl)
 DEF_HELPER_3(stvewx, void, env, avr, tl)
+DEF_HELPER_4(lxvl, void, env, tl, tl, tl)
 DEF_HELPER_4(vsumsws, void, env, avr, avr, avr)
 DEF_HELPER_4(vsum2sws, void, env, avr, avr, avr)
 DEF_HELPER_4(vsum4sbs, void, env, avr, avr, avr)
diff --git a/target-ppc/mem_helper.c b/target-ppc/mem_helper.c
index 1ab8a6e..54447a7 100644
--- a/target-ppc/mem_helper.c
+++ b/target-ppc/mem_helper.c
@@ -24,6 +24,7 @@
 
 #include "helper_regs.h"
 #include "exec/cpu_ldst.h"
+#include "internal.h"
 
 //#define DEBUG_OP
 
@@ -284,8 +285,40 @@ STVE(stvewx, cpu_stl_data_ra, bswap32, u32)
 #undef I
 #undef LVE
 
+#ifdef TARGET_PPC64
+#define GET_NB(rb) ((rb >> 56) & 0xFF)
+#else
+#define GET_NB(rb) ((rb >> 24) & 0xFF)
+#endif
+
+void helper_lxvl(CPUPPCState *env, target_ulong addr,
+ target_ulong xt_num, target_ulong rb)
+{
+int i;
+ppc_vsr_t xt;
+uint64_t nb = GET_NB(rb);
+
+xt.s128 = int128_zero();
+if (nb) {
+nb = (nb >= 16) ? 16 : nb;
+if (msr_le) {
+for (i = 16; i > 16 - nb; i--) {
+xt.VsrB(i - 1) = cpu_ldub_data_ra(env, addr, GETPC());
+addr = addr_add(env, addr, 1);
+}
+} else {
+for (i = 0; i < nb; i++) {
+xt.VsrB(i) = cpu_ldub_data_ra(env, addr, GETPC());
+addr = addr_add(env, addr, 1);
+}
+}
+}
+putVSR(xt_num, , env);
+}
+
 #undef HI_IDX
 #undef LO_IDX
+#undef GET_NB
 
 void helper_tbegin(CPUPPCState *env)
 {
diff --git a/target-ppc/translate/vsx-impl.inc.c 
b/target-ppc/translate/vsx-impl.inc.c
index 808ee48..6bd70a0 100644
--- a/target-ppc/translate/vsx-impl.inc.c
+++ b/target-ppc/translate/vsx-impl.inc.c
@@ -240,6 +240,33 @@ VSX_VECTOR_LOAD_STORE(stxv, st_i64, 0)
 VSX_VECTOR_LOAD_STORE(lxvx, ld_i64, 1)
 VSX_VECTOR_LOAD_STORE(stxvx, st_i64, 1)
 
+#define VSX_VECTOR_LOAD_STORE_LENGTH(name)  \
+static void gen_##name(DisasContext *ctx)   \
+{   \
+TCGv EA, xt;\
+\
+if (xT(ctx->opcode) < 32) { \
+if (unlikely(!ctx->vsx_enabled)) {  \
+gen_exception(ctx, POWERPC_EXCP_VSXU);  \
+return; \
+}   \
+} else {\
+if (unlikely(!ctx->altivec_enabled)) {  \
+gen_exception(ctx, POWERPC_EXCP_VPU);   \
+return; \
+}   \
+}   \
+EA = tcg_temp_new();\
+xt = tcg_const_tl(xT(ctx->opcode)); \
+gen_set_access_type(ctx, ACCESS_INT);   \
+gen_addr_register(ctx, EA); \
+gen_helper_##name(cpu_env, EA, xt, cpu_gpr[rB(ctx->opcode)]); \
+tcg_temp_free(EA);  \
+tcg_temp_free(xt);  \
+}
+
+VSX_VECTOR_LOAD_STORE_LENGTH(lxvl)
+
 #define VSX_LOAD_SCALAR_DS(name, operation)   \
 static void gen_##name(DisasContext *ctx) \
 { \
diff