Re: [Qemu-devel] [PATCH RFC v0 5/6] target-ppc: add vsrv instruction

[David Gibson]

On Wed, Jul 27, 2016 at 12:01:33PM +0530, Nikunj A Dadhania wrote:
> David Gibson <address@hidden> writes:
> 
> > [ Unknown signature status ]
> > On Wed, Jul 27, 2016 at 12:56:57AM +0530, Nikunj A Dadhania wrote:
> >> From: Vivek Andrew Sha <address@hidden>
> >> 
> >> Adds Vector Shift Right Variable instruction.
> >> 
> >> Signed-off-by: Vivek Andrew Sha <address@hidden>
> >> Signed-off-by: Nikunj A Dadhania <address@hidden>
> >> ---
> >>  target-ppc/helper.h     |  1 +
> >>  target-ppc/int_helper.c | 17 +++++++++++++++++
> >>  target-ppc/translate.c  |  2 ++
> >>  3 files changed, 20 insertions(+)
> >> 
> >> diff --git a/target-ppc/helper.h b/target-ppc/helper.h
> >> index 9703f85..8eada2f 100644
> >> --- a/target-ppc/helper.h
> >> +++ b/target-ppc/helper.h
> >> @@ -211,6 +211,7 @@ DEF_HELPER_3(vslw, void, avr, avr, avr)
> >>  DEF_HELPER_3(vsld, void, avr, avr, avr)
> >>  DEF_HELPER_3(vslo, void, avr, avr, avr)
> >>  DEF_HELPER_3(vsro, void, avr, avr, avr)
> >> +DEF_HELPER_3(vsrv, void, avr, avr, avr)
> >>  DEF_HELPER_3(vslv, void, avr, avr, avr)
> >>  DEF_HELPER_3(vaddcuw, void, avr, avr, avr)
> >>  DEF_HELPER_3(vsubcuw, void, avr, avr, avr)
> >> diff --git a/target-ppc/int_helper.c b/target-ppc/int_helper.c
> >> index 412398f..f4776f0 100644
> >> --- a/target-ppc/int_helper.c
> >> +++ b/target-ppc/int_helper.c
> >> @@ -1722,6 +1722,23 @@ void helper_vslv(ppc_avr_t *r, ppc_avr_t *a, 
> >> ppc_avr_t *b)
> >>      }
> >>  }
> >>  
> >> +void helper_vsrv(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b)
> >> +{
> >> +    int i;
> >> +    unsigned int shift, bytes, src[ARRAY_SIZE(r->u8) + 1];
> >> +
> >> +    src[0] = 0;
> >> +    for (i = 0; i < ARRAY_SIZE(r->u8); i++) {
> >> +        src[i + 1] = a->u8[i];
> >> +    }
> >> +
> >> +    for (i = 0; i < ARRAY_SIZE(r->u8); i++) {
> >> +        shift = b->u8[i] & 0x7;               /* extract shift value */
> >> +        bytes = (src[i] << 8) + src[i + 1];     /* extract adjacent bytes 
> >> */
> >
> > I think you should be able to construct bytes on the fly without
> > pre-generating teh whole of src, as you already did for vslv.
> 
> Had done that, but that introduces a bug like this, for eg:
> 
>     vslv vra,vra,vrb
> 
> So modified vra->u8[i] is used during subsequent operation as input.
> 
> Assuming I take care or special casing "0":
> 
> bytes = ((vra->u8[i - 1]  << 8) | (vra->u8[i])) 
> vra->u8[i] = (bytes >> shift) & 0xFF;
> 
> when i = 1, bytes will ((vra->u8[0]  << 8) | (vra->u8[1])). But vra->u8[0],
> was changed in the previous operation.

Ah, good point.

> Thats the reason src[] is needed

It's probably possible to avoid generating all of src by instead
generating the bytes one loop iteration ahead, but that sounds fiddly,
so the current approach is fine for now.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature