[Mono-dev] ppc64 port [was: Interpreter?]

[Andreas Färber]

Hey,

>> First, it includes changes to eglib/configure.in to make it compile
>> on OS X and adds VPATH support to facilitate compiling for ppc and
>> ppc64.
>
> I suggest sending the eglib changes separately (note that the vararg
> changes need to be inside a #ifdef conditional).

This is not a patch that anyone was supposed to apply. Also, I don't  
see an easy way of separating changes within one file that can go in  
from those that are not yet ready?

I *did* file eglib issues separately (#81456), unfortunately Jonathan  
soon after started to commit his own duplicating/conflicting Windows  
eglib patches that made this more difficult (see list).

The vararg stuff is an uncommited patch supplied by Jonathan  
available from my #81520; it made eglib compile again at all. You  
opposed his patch and demanded that *I* add tests first to have it  
reverted, in a scenario where exactly those tests were not running  
for me due to the ppc-ppc64 cross-compilation - I made that clear but  
to date nobody has stepped up with a solution (you will see that in  
my ppc64 patch eglib currently tests itself against itself so the  
tests probably are effectively useless).
Although not yet having used varargs in C myself, to me it looks  
wrong that the mandatory format argument is being dropped... So maybe  
there is a better way to patch where we don't need further ifdefs?

>> Next, it reorganizes the ppc-codegen.h (header still needs some
>> changes Miguel requested) - instructions are now listed
>
> I'm sorry you have been led to do this kind of change: it is not
> acceptable to do extensive cleanups together with code changes:
> please send us only the code changes so we can actually review them
> (this will also remove one third from the patch length).

I don't see the point in reverting those changes. Mine are trivial:  
64-bit instructions were added and are distinguishable by  
corresponding #ifdefs, for existing instructions only #ifndefs were  
added. Nothing was changed on the old instructions, and few of them  
are actually used anywhere at all! The new ordering is the only way  
the instructions can be checked sensibly because that's the order  
they are in the various manuals. I specifically asked Miguel about  
that file and he had only minor objections regarding the use of (c)  
vs. Copyright. So, might it be acceptable for you to try to split  
this up into one reordering patch and an addition/change patch?

>> Then, the mini and arch ppc files were updated with #ifdefs for ppc64
>> doing all kinds of necessary changes. I have some FIXMEs/CHECKMEs in
>> there, especially some numeric offsets and alignments are unclear to
>> me. This is where I suspect the most issues.
>> Finally, all occurrences of ppc in the sources were checked and  
>> adapted.
>
> I gave a very quick read of the changes, here are a few suggestions:
> *) you shold minimize the number of inserted #ifdefs. A patch that  
> does:

Again this was not submitted for application. Were it clear in all  
places that this were actually intended I could have done so, yes.  
Unfortunately I am not sure everywhere that four equals sizeof 
(gpointer). Having it in one file during development simplifies  
comparison of before/after for me, avoiding the use of non-graphical  
svn diff (svnX's use of Apple's FileMerge tool rarely works for me).

> +#ifdef __ppc64__
> +               offset += 8;
> +#else
>                 offset += 4;
> +#endif
>
> is not acceptable, while if it did:
>
> -               offset += 4;
> +               offset += sizeof (gpointer);
>
> it would be applied right away.
> Similar issues are with changes like:
> +#ifdef __ppc64__
> +       __asm__ volatile("ld    %0,0(r1)" : "=r" (sframe));
> +#else
>         __asm__ volatile("lwz   %0,0(r1)" : "=r" (sframe));
> +#endif
>
> Just add to the header something like:
> #ifdef __ppc64__
> #define PPC_LOAD_REG_INS "ld"
> #else
> #define PPC_LOAD_REG_INS "lwz"
> #endif
>
> and replace the asm statements with:
>         __asm__ volatile (PPC_LOAD_REG_INS "   %0,0(r1)" :  
> "=r" (sframe));
>
> Something similar should be done also for the native code emitting
> macros. Instead of:
>
> +#ifdef __ppc64__
> +               ppc_ld  (buf, ppc_r0, (guint16)G_STRUCT_OFFSET 
> (MonoLMF, previous_lmf) >> 2, ppc_r3);
> +               ppc_std (buf, ppc_r0, (guint16)G_STRUCT_OFFSET 
> (MonoLMF, previous_lmf) >> 2, ppc_r11);
> +#else
>                 ppc_lwz (buf, ppc_r0, G_STRUCT_OFFSET(MonoLMF,  
> previous_lmf), ppc_r3);
>                 ppc_stw (buf, ppc_r0, G_STRUCT_OFFSET(MonoLMF,  
> previous_lmf), ppc_r11);
> +#endif
>
> define in ppc_codegen:
>
> #ifdef __ppc64__
> #define ppc_load_reg(a,b,c,d) ppc_ld(a,b,(((c) >> 2) & 0xffff),d)
> #define ppc_store_reg(a,b,c,d) ppc_std(a,b,(((c) >> 2) & 0xffff),d)
> #else
> #define ppc_load_reg(a,b,c,d) ppc_lwz(a,b,c,d)
> #define ppc_store_reg(a,b,c,d) ppc_stw(a,b,c,d)
> #endif
>
> This will make the changes so much cleaner that it will be possible to
> more easily review them and find the issues (and it will also allow
> us to commit them incrementally to svn).

These requested changes make sense but will take some more weeks and  
months which I can unlikely spend at this time...

> *) cpu-g5.md should become cpu-ppc64.md

In that case cpu-g4.md should also become cpu-ppc.md and the variable  
name in mini(?) should be changed, too.

> *) the ppc_load changes are not acceptable: add a ppc_load_sequence
> or something like that that will emit lis/ori on 32 and the full
> sequence on 64 bit and use that where it is required.

That I don't understand, please explain.
ppc_load is one logical operation and is ifdef'ed as the lis/ori for  
32, isn't it? All occurrences of manual lis/ori for trampolines were  
checked and updated and appear to work on ppc as is. So why would I  
need a ppc_load_sequence when there's no difference to ppc_load? The  
trampolines need 64-bit support in some places so we cannot say  
trampolines use 32-bit ppc_load and the rest 64-bit ppc_load_sequence.

> *) mono/io-layer/atomic.h: the changes here can't be skipped, they are
> used in many parts of the runtime and without the 64 bit support there
> can be memory corruption.

I will probably need more info than just "can't be skipped" to  
actually do that. :-)

> *) the emit_tls_access optimizations should likely be removed unless
> they have been tested explicitly (they look a bit bogus).

Not sure what optimizations you mean? Obviously I have to update the  
code for ppc64 and can't just remove that part of the patch. The only  
"optimization" I'm aware of is removing the G4 case as the G4 to my  
knowledge is 32-bit by nature.

>> compilation of System.dll using the new mono (or on execution of a
>> hello world assembly) it aborts with empty stacktrace:
>>
>> MONO_PATH="../../class/lib/basic:$MONO_PATH" /Users/andreas/Mono/ 
>> mono-
>> ppc64/mono/runtime/mono-wrapper  ../../class/lib/basic/mcs.exe /
>> codepage:65001   -d:NET_1_1 -d:ONLY_1_1 -d:BOOTSTRAP_WITH_OLDLIB -
>> debug /noconfig -define:XML_DEP -r:System.Xml.dll -target:library -
>> out:System.dll  @System.dll.sources
>> Stacktrace:
> [...]
>> (gdb) run ../../../testassembly.exe
>> Starting program: /Users/andreas/Mono/mono-ppc64/mono/mono/mini/
>> mono ../../../testassembly.exe
>> Reading symbols for shared libraries .++ done
>>
>> Program received signal SIGBUS, Bus error.
>> 0x0000000000000000 in ?? ()
>
> This looks like the result of a jump to NULL.
> What are the results of running make rcheck in mini/? That is the  
> first
> thing that needs checking. If it fails in the same way before starting
> the tests it is likely related to something earlier on in the
> initialization.

I was not aware of a make rcheck, I only knew the standard make  
check, which fails with a similar error at the first compilation.

> The first thing that comes to mind is: doesn't ppc64 on OSX
> use function descriptors like all the other ppc64 platforms? If it  
> does
> use them this would be the first thing to fix as any indirect call  
> would
> pretty much fail.

I had found no information on this. An IBM page described those for  
the Linux ABI, but the OS X ABI as we know differs in some points and  
did not talk about functions/descriptors. I don't have a ppc64 Linux  
running so couldn't check. My port was only targetted at OS X, for  
Linux some additional changes will need to be made and tested  
(different processor defines, offsets, the usual suspects).

> Well, I've been here and I don't remember seeing any jit-related  
> request
> for help:) Follow the above suggestions for now and we can both try to
> get the changes in svn and get the bugs fixed.

I'd be happy to share this with other users through SVN but as stated  
before the changes requested above are non-trivial and will take some  
time; I started the port for an AI competition of our university but  
didn't finish in time. ;-) I wasn't able to work on it much lately,  
just merged some eglib conflicts and checked that no conflicting JIT  
changes were introduced and played with some of the unclear offset/ 
etc. values.

I did not ask earlier about atomic changes because I am in most cases  
a fan of test-driven development and did not dare submitting untested  
JIT changes for inclusion in SVN, the only successful tests so far  
being those of the ppc-codegen.h instruction macros.

Andreas