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

[Aurelien Jarno]

On Wed, Sep 03, 2008 at 02:39:52AM +0200, Andreas Färber wrote:
>
> 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.

Well that's different, your previous post is about T_* registers, I am
speaking about the GPR registers. And either ppc_gpr_t or the init code
of cpu_gpr[i] has to be changed, otherwise you will break existing
targets.

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

Oops sorry I was tired, I forget the most important word. I was asking
for also replacing the store 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?

Well, except that until know they basically corresponds to automatic
replacement of a pattern by another. Even if the patch is big, the
changes are simple.

I agree that for more complex changes, smaller chunks are better.

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

-- 
  .''`.  Aurelien Jarno             | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   address@hidden         | address@hidden
   `-    people.debian.org/~aurel32 | www.aurel32.net