Re: [Qemu-devel] [RFC v1 13/13] target-ppc: introduce opc4 for Expanded Opcode

[David Gibson]

On Fri, Jul 22, 2016 at 11:05:54AM +0530, Nikunj A Dadhania wrote:
> David Gibson <address@hidden> writes:
> 
> > [ Unknown signature status ]
> > On Mon, Jul 18, 2016 at 10:35:17PM +0530, Nikunj A Dadhania wrote:
> >> ISA 3.0 has introduced EO - Expanded Opcode. Introduce third level
> >> indirect opcode table and corresponding parsing routines.
> >> 
> >> EO (11:12) Expanded opcode field
> >> Formats: XX1
> >> 
> >> EO (11:15) Expanded opcode field
> >> Formats: VX, X, XX2
> >> 
> >> Signed-off-by: Nikunj A Dadhania <address@hidden>
> >> ---
> >>  target-ppc/translate.c      |  73 +++++++++++++++++++++++++------
> >>  target-ppc/translate_init.c | 103 
> >> ++++++++++++++++++++++++++++++++------------
> >>  2 files changed, 136 insertions(+), 40 deletions(-)
> >> 
> >> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> >> index 6c5a4a6..733d68d 100644
> >> --- a/target-ppc/translate.c
> >> +++ b/target-ppc/translate.c
> >> @@ -40,6 +40,7 @@
> >>  /* Include definitions for instructions classes and implementations flags 
> >> */
> >>  //#define PPC_DEBUG_DISAS
> >>  //#define DO_PPC_STATISTICS
> >> +//#define PPC_DUMP_CPU
> >>  
> >>  #ifdef PPC_DEBUG_DISAS
> >>  #  define LOG_DISAS(...) qemu_log_mask(CPU_LOG_TB_IN_ASM, ## __VA_ARGS__)
> >> @@ -367,12 +368,15 @@ GEN_OPCODE2(name, onam, opc1, opc2, opc3, inval, 
> >> type, PPC_NONE)
> >>  #define GEN_HANDLER2_E(name, onam, opc1, opc2, opc3, inval, type, type2)  
> >>     \
> >>  GEN_OPCODE2(name, onam, opc1, opc2, opc3, inval, type, type2)
> >>  
> >> +#define GEN_HANDLER_E_2(name, opc1, opc2, opc3, opc4, inval, type, type2) 
> >>     \
> >> +GEN_OPCODE3(name, opc1, opc2, opc3, opc4, inval, type, type2)
> >> +
> >>  typedef struct opcode_t {
> >> -    unsigned char opc1, opc2, opc3;
> >> +    unsigned char opc1, opc2, opc3, opc4;
> >>  #if HOST_LONG_BITS == 64 /* Explicitly align to 64 bits */
> >> -    unsigned char pad[5];
> >> +    unsigned char pad[4];
> >>  #else
> >> -    unsigned char pad[1];
> >> +    unsigned char pad[4]; /* 4-byte pad to maintain pad in opcode table */
> >
> > IIUC the point here is to align entries to the wordsize.  If the
> > worsize is 32-bit you shouldn't need any extra padding here.
> 
> You are right, the reason I had added this here is to keep the code
> clean in the GEN_OPCODEx
> 
> #define GEN_OPCODE(name, op1, op2, op3, op4, invl, _typ, _typ2)   \
> {                                                                  \
>     .opc1 = op1,                                                   \
>     .opc2 = op2,                                                   \
>     .opc3 = op3,                                                   \
>     .opc4 = 0xff,                                                  \
> #if HOST_LONG_BITS == 64                                           \
>     .pad  = { 0, },                                                \
> #endif                                                             \

Hrm.. you're using C99 designated initializers, which means I'm pretty
sure you can just leave out the pad field, since you don't care about
it's value.  That should avoid the need for an ifdef.

>     .handler = {                                                   \
>         .inval1  = invl,                                           \
>         .type = _typ,                                              \
>         .type2 = _typ2,                                            \
>         .handler = &gen_##name,                                    \
>         .oname = stringify(name),                                  \
>     },                                                             \
>     .oname = stringify(name),                                      \
> }
> 
> I am fine with both the approach, but thought of the current one as
> cleaner, we would waste 4byte per opcode in 32-bit case.
> 
> Regards
> Nikunj
> 

-- 
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