Re: [Qemu-devel] [PATCH 5/x] ppc: Convert op_load_gpr_{T0, T1, T2} to TCG

[Andreas Färber]

Am 03.09.2008 um 01:20 schrieb Aurelien Jarno:

> On Wed, Sep 03, 2008 at 12:22:48AM +0200, Andreas Färber wrote:
> > +    static const char* const gprnames[32] = {
> > +        "r0",
> > +        "r1",
> > +        "r2",
> > +        "r3",
> > +        "r4",
> > +        "r5",
> > +        "r6",
> > +        "r7",
> > +        "r8",
> > +        "r9",
> > +        "r10",
> > +        "r11",
> > +        "r12",
> > +        "r13",
> > +        "r14",
> > +        "r15",
> > +        "r16",
> > +        "r17",
> > +        "r18",
> > +        "r19",
> > +        "r20",
> > +        "r21",
> > +        "r22",
> > +        "r23",
> > +        "r24",
> > +        "r25",
> > +        "r26",
> > +        "r27",
> > +        "r28",
> > +        "r29",
> > +        "r30",
> > +        "r31"
> > +    };
> > +
>
> You may want to use a sprintf() function instead (see other targets).

I based this on SH4. Will check the others.


> > +    for (i = 0; i < 32; i++) {
> > +        cpu_gpr[i] = tcg_global_mem_new(TCG_TYPE_TL, TCG_AREG0,
> > +                                        offsetof(CPUState, gpr[i]),
> > +                                        gprnames[i]);
>
> This is most probably wrong given the definition of ppc_gpr_t in  
> cpu.h:
> - 64 bits on 64-bit targets
> - 32 bits on 32-bit targets and 32-bit hosts
> - *64 bits* on 32-bit targets and 64-bit hosts
>
> I think it is a bit weird, and I think the best is to modify cpu.h in
> order to have ppc_gpr_t matching the target bitness.

This might be related to the 64-on-32 issue I just posted about. I  
don't remember seeing code that makes use of this sophisticated  
definition though, for non-ppc64 it apparently uses a set of 32-bit  
gpr and gprh variables. Let's not change ppc_gpr_t for now.

> Care to also replace the ops in the same
> patch?

The related dyngen ops can't be removed yet due to SPE, in case that's  
what you meant?


> It's actually a general request, would nice to have a few more
> instructions switched in a patch, to avoid spending to much time in
> handling patches.

The op_moven before was only separate because it was the first to  
translate a ~ operation to TCG.

This one is actually the largest patch in the series so far... Since  
it's my first stab at (this side of) TCG and since translate.c has  
several thousand lines of code, I appreciate early feedback on whether  
I have to revert my branch and redo a commit/patch. I don't really  
care if you apply it right away, the review is the more important part  
and reviewing small chunks is easier, no?

I'll rebase tomorrow and start pushing to repo.or.cz, that should make  
testing and applying patches easier.

Andreas