Clang for the PlayStation 2

[Dan_Ravensloft]

Hello,

I’m part of the (sadly fairly small) community of PS2 hackers. The current cross-toolchain for the PS2 is based on GCC 3.2.3, an outdated and buggy compiler, which I have personally gotten tired of working with, so I would like to port Clang as a newer cross-compiler for the PS2.

However, the PS2 has some notable quirks which make this a non-trivial task for the current compiler. It has two main CPUs, the “Emotion Engine” (EE), which controls the main operating system, and the “I/O Processor” (IOP), which is used for PS1 compatibility and for I/O.

The EE is based on a custom chip called the R5900, which implements most of MIPS III (except the ll and sc instructions, which make little sense on a single-core CPU), as well as some instructions from MIPS IV (pref, movz/movn, rsqrt.s), and a set of SIMD instructions known as Multimedia Instructions (MMI). It also contains a non-IEEE 754 single-precision FPU (which has provided a lot of headaches). It was later re-used by Toshiba as the TX79, along with a proper FPU.

The IOP is based on the MIPS I R3051A, and was also used as the PS1 CPU.

I’d like to go through some of the issues I’ve had so far.

I’ve tried to use mips64el as a target, but it would invoke the system GCC with obvious bad results, so I have used the mips64el-img-linux target to generate ELF files which the PS2 BIOS can load (is there a better solution to generate bare-metal ELF?).

I then tried to compile newlib 3.0.0-20180802, as newlib is used as the standard C library for the PS2, and hit multiple asserts, which I have attached below.

I don’t mind trying to fix these myself, but I would appreciate some pointers.

llvm-bugs.tgz (440 KB)

[TNorthover]

Hi Dan,

I then tried to compile newlib 3.0.0-20180802, as newlib is used as the standard C library for the PS2, and hit multiple asserts, which I have attached below.

After a quick look, three of the issues seem to be related to "long
double", which is translated to fp128:

vfprintf: invalid bitcast from f128 to f64. Part of some call, maybe?
ldtoa: obviously a function taking "long double".
vfscanf: Mishandling return of f128 from a libcall.

Now, mapping long double to fp128 is actually a choice you have as an
ABI creator. It's possible you feel constrained to follow GCC here and
so need that, but if not the simplest fix might well be to just
declare long double is the same as double. PS2 doesn't strike me as a
system where you'd often want a 128-bit float. You'd do that by
modifying tools/clang/lib/Basic/Targets/Mips.h (the setN32N64ABITypes
function, notice FreeBSD has already done this).

If you do have to support f128, you'd probably start by focusing on
MipsISelLowering.cpp. Specifically the functions named things like
LowerFormalArguments, LowerCall, LowerReturn. I'm not sure what the
ABI is likely to be there, but I suspect you'll end up assigning an
fp128 to 2 64-bit integer registers. If needed I'm sure me or a Mips
expert could provide more details.

The other problem (lrintfp) seems to involve an invalid truncation
from f32 to f64, possibly inserted by code unaware of the single-float
option. Nothing looks obviously wrong, so it's the usual debugging
procedure:

1. Try to produce a reduced test-case
2. gdb/lldb it, since not many places produce the Mips-specific
TruncIntFP node that's causing the problem.

For 1, I've actually already got one for you:

void foo(float *in, long long *out) {
  *out = *in;
}

In brief, how I got it was:
1. Run lldb on the crashing Clang command. Go up the backtrace until I
got usable C++ code. DAG.viewGraph() produces a very nice pictorial
representation of the code being selected, it had load(f32) ->
MipsISD::TruncIntFP (f64) -> store. That middle trunc is obviously
really dodgy.
2. From there you could try to write IR that did the same thing.
Instead I printed "BlockName" that was available in one of the frames.
Then I dumped the IR from Clang (-emit-llvm) and just looked at that
IR snippet:

  %23 = load float, float* %x.addr, align 4
  %conv33 = fptosi float %23 to i64
  store i64 %conv33, i64* %retval, align 8
  br label %return

From there it's a lot easier to write down the C function I gave,

which is what I did.

So the next step is to debug where Mips is producing those TruncIntFP
nodes. There'll be some constraint it's not checking or an unexpected
node type, probably related to -msingle-float. I'm afraid I'm not sure
what yet.

Cheers.

Tim.

[Dan_Ravensloft]

Thanks Tim, that’s actually a very useful guide, and debugging this is going to be much easier with that knowledge.

First step: rebuild Clang.

[Dan_Ravensloft]

I’m reasonably sure the function producing that node is lowerFP_TO_SINT_STORE in lib/Target/Mips/MipsISelLowering.cpp.

The node before that function executes has an fp_to_sint node which seems to want to convert an i64 to an f32 which seems…rather odd to me, honestly. The PS2, for what it’s worth, only has an i32 → f32 instruction, so I think there’s an impedance mismatch somewhere.

[TNorthover]

I'm reasonably sure the function producing that node is lowerFP_TO_SINT_STORE in lib/Target/Mips/MipsISelLowering.cpp.

That doesn't surprise me.

The node before that function executes has an fp_to_sint node which seems to want to convert an i64 to an f32 which seems...rather odd to me, honestly.

It's a pretty common kind of operation. C says you're allowed to cast
an int64_t to a float, and that's the IR you get when you try.

The PS2, for what it's worth, only has an i32 -> f32 instruction, so I think there's an impedance mismatch somewhere.

This is also a fairly common situation. If the operation can be
emulated with a reasonably small number of native instructions you can
often get LLVM to do that.

In this case it would probably be a libcall though because it's quite
complex. LLVM would generate a call to __floatdisf, which will be
provided by compiler-rt (there are C implementations for all kinds of
floating-point operations there).

You should see the same thing if you compile a function doing that
conversion with GCC.

Cheers.

Tim.

[dsanders]

So the next step is to debug where Mips is producing those TruncIntFP
nodes. There’ll be some constraint it’s not checking or an unexpected
node type, probably related to -msingle-float. I’m afraid I’m not sure
what yet.

FWIW, I’m not sure how well tested -msingle-float was on MIPS. I don’t think we had any bots testing it.

I’m reasonably sure the function producing that node is lowerFP_TO_SINT_STORE in lib/Target/Mips/MipsISelLowering.cpp.

The node before that function executes has an fp_to_sint node which seems to want to convert an i64 to an f32 which seems…rather odd to me, honestly. The PS2, for what it’s worth, only has an i32 → f32 instruction, so I think there’s an impedance mismatch somewhere.

Did you mean those types to be the other way around? fp_to_sint is supposed to take a floating point type and produce an integer type so if you’re seeing them backwards like this then that would definitely be a bug.

If you’re referring to the size mismatch though, that’s ok within the IR and DAG nodes. There’s no relationship between the size of the input and output.

[Dan_Ravensloft]

So the next step is to debug where Mips is producing those TruncIntFP
nodes. There’ll be some constraint it’s not checking or an unexpected
node type, probably related to -msingle-float. I’m afraid I’m not sure
what yet.

FWIW, I’m not sure how well tested -msingle-float was on MIPS. I don’t think we had any bots testing it.

Well, now we have hardware, if all of one machine, where it’s worth testing. I’m happy to try it myself if needed, though rigging it up to happen automatically would be difficult.

I’m reasonably sure the function producing that node is lowerFP_TO_SINT_STORE in lib/Target/Mips/MipsISelLowering.cpp.

The node before that function executes has an fp_to_sint node which seems to want to convert an i64 to an f32 which seems…rather odd to me, honestly. The PS2, for what it’s worth, only has an i32 → f32 instruction, so I think there’s an impedance mismatch somewhere.

Did you mean those types to be the other way around? fp_to_sint is supposed to take a floating point type and produce an integer type so if you’re seeing them backwards like this then that would definitely be a bug.

Yes, my mistake; I got confused by the direction of the arrows on the graph, they’re pointing to their parents, but I thought they were pointing to their children.

[Dan_Ravensloft]

The PS2, for what it’s worth, only has an i32 → f32 instruction, so I think there’s an impedance mismatch somewhere.

This is also a fairly common situation. If the operation can be
emulated with a reasonably small number of native instructions you can
often get LLVM to do that.

In this case it would probably be a libcall though because it’s quite
complex. LLVM would generate a call to __floatdisf, which will be
provided by compiler-rt (there are C implementations for all kinds of
floating-point operations there).

You should see the same thing if you compile a function doing that
conversion with GCC.

Cheers.

Tim.

So I was rereading this; do you think the lowering function should instead emit a library call instead, then?

If so, would you mind pointing me to a function which performs this, or otherwise give a high-level description of how this is done?

[TNorthover]

So I was rereading this; do you think the lowering function should instead emit a library call instead, then?

I suspect so, though I'm not familiar enough with MIPS or the PS2 to
be 100% sure -- checking what GCC does in this case would be good
confirmation.

If so, would you mind pointing me to a function which performs this, or otherwise give a high-level description of how this is done?

I *think* you should be able to just tell the lower function to ignore
this case (maybe based on the types, maybe just because we're in
SingleFloat mode, I'll need to read more code to be sure). Then the
generic handling should get involved and expand it to a libcall if the
operation isn't natively supported.

Cheers.

Tim.

[TNorthover]

Hi again,

If so, would you mind pointing me to a function which performs this, or otherwise give a high-level description of how this is done?

I just did a very quick experiment where I made lowerFP_TO_SINT and
lowerFP_TO_SINT_STORE return SDValue() (which is the marker for "I
don't want to handle this"). It looks like someone was enthusiastic
enough to think this operation did actually deserve inlining (by
TargetLowering::expandFP_TO_SINT) so instead of a libcall it produces
a bunch of weird operations implementing that. I assume they're right,
but haven't checked.

To make it production-quality you'd want to predicate the changes I
made on Subtarget->isSingleFloat() I think (probably in combination
with the actual types, since f32 -> i32 ought to still be OK with the
existing code). The main annoyance there is that lowerFP_TO_SINT_STORE
is static rather than a member of MipsISelLowering so it doesn't have
access to Subtarget. Personally, I'd just make it a member function to
fix that.

Cheers.

Tim.

[Dan_Ravensloft]

I just did a very quick experiment where I made lowerFP_TO_SINT and
lowerFP_TO_SINT_STORE return SDValue() (which is the marker for “I
don’t want to handle this”).

I just tried this, but the compiler still crashes with the same error. Maybe our experiments were different.

To make it production-quality you’d want to predicate the changes I
made on Subtarget->isSingleFloat() I think (probably in combination
with the actual types, since f32 → i32 ought to still be OK with the
existing code). The main annoyance there is that lowerFP_TO_SINT_STORE
is static rather than a member of MipsISelLowering so it doesn’t have
access to Subtarget. Personally, I’d just make it a member function to
fix that.

lowerFP_TO_SINT_STORE is only ever called by lowerFP_TO_SINT, so I’m just passing single-floatness (we need a better name for that) as an argument to lowerFP_TO_SINT_STORE at the moment.

[Dan_Ravensloft]

Hi,

Years ago, I got to go through the magical adventure of building a working Cell compiler based on LLVM / Clang for the PS3. It was supposed to be open sourced / upstreamed to llvm, but of course the company didn’t want to pay for the extra hours that would take.

Heh, I’m not getting paid anything for this.

I’m not sure how bad the GCC compiler for PS2 is, but if it’s anything like the PS3 one I’d suggest to watch out that any Sony ABI docs probably have glaring faults somewhere.

Yup, the PS2 docs are…okay, but not great. The IOP is entirely undocumented, for example. There’s no specific Sony ABI, though.

I don’t know how much I can help with the PS2 / EE but I’ve noticed that some of that type of design stuff tends to persist so when you get to that area of it let me know if you run into any problems and they might refresh my memory on something that can help; never know.

Sure, I’m going to try to make my misadventures public for posterity so that anybody who comes along later can continue my work if they wish.

I’ve got a debug/test PS3 with a hardware Emotion Engine as well if you want any spot checks on that particular version of the platform. I’m guessing the actual debugging part is either already covered or easier via an emulator + PC at this point, and I don’t have it set up for that.

Can’t say no to free hardware, although a TOOL might be nice, providing Sony haven’t destroyed them all.

[TNorthover]

I just tried this, but the compiler still crashes with the same error. Maybe our experiments were different.

Strange. I only tested it on a simple reproducer rather than newlib:

void foo(float *in, long long *out) {
       *out = *in;
}
$ clang -target mips64el-img-linux -mcpu=mips3 -S -o- -Os tmp.c
[...]

Is it possible you were hitting a different error with roughly similar output?

lowerFP_TO_SINT_STORE is only ever called by lowerFP_TO_SINT, so I'm just passing single-floatness (we need a better name for that) as an argument to lowerFP_TO_SINT_STORE at the moment.

Are you sure? I see it being called by lowerSTORE.

Cheers.

Tim.

[Dan_Ravensloft]

I just tried this, but the compiler still crashes with the same error. Maybe our experiments were different.

Strange. I only tested it on a simple reproducer rather than newlib:

void foo(float *in, long long *out) {
*out = *in;
}
$ clang -target mips64el-img-linux -mcpu=mips3 -S -o- -Os tmp.c
[…]

Is it possible you were hitting a different error with roughly similar output?

Both your simple reproducer and newlib give the same crash for me (Unexpected illegal type), though my build of LLVM doesn’t list any direct symbols in the backtrace despite compiling in Debug mode. (maybe I missed a cmake option?)

lowerFP_TO_SINT_STORE is only ever called by lowerFP_TO_SINT, so I’m just passing single-floatness (we need a better name for that) as an argument to lowerFP_TO_SINT_STORE at the moment.

Are you sure? I see it being called by lowerSTORE.

You are correct, my apologies.

So we’re on the same metaphorical page, I’ve attached my git diffs; the build target I’m using is mips64el-scei-ps2. ps2-clang.diff applies to clang, while ps2-llvm applies to llvm, but that should be obvious.

ps2-llvm.diff (2.71 KB)

ps2-clang.diff (587 Bytes)

[Dan_Ravensloft]

Actually, I just tried your flags; you’re missing -msingle-float, which is what reproduces the crash on my end. Without it there is no problem.

[TNorthover]

Oops, yes, I was then. Fortunately not last night though, and my Clang
still works with -msingle-float.

I looked at your diffs and you've only changed one of the functions to
return SDValue(), you need to change lowerFP_TO_SINT itself too. The
one with the store is just there as an optimization; if it doesn't
trigger (because of your diff) then lowerFP_TO_SINT will still create
a bad node afterwards.

Cheers.

Tim.

[Dan_Ravensloft]

Thank you!

With that, the four bugs I found in newlib’s C code are dead. I think the lrint fix should be upstreamed right away; would you mind if I credited you in the patch?

Unfortunately, we are not out of the woods yet. If memory serves me correctly, newlib has some assembly code in used for things like _start to bring up the C runtime, which GNU binutils accepts and Clang’s assembler currently does not.

I’ll go build that so I’m not talking out of my ass.

Cheers, Tim!

[TNorthover]

With that, the four bugs I found in newlib's C code are dead.

Excellent!

I think the lrint fix should be upstreamed right away; would you mind if I credited you in the patch?

Not at all. Thanks!

Tim.

[arichardson]

I looked at your diffs and you've only changed one of the functions to
return SDValue(), you need to change lowerFP_TO_SINT itself too. The
one with the store is just there as an optimization; if it doesn't
trigger (because of your diff) then lowerFP_TO_SINT will still create
a bad node afterwards.

Thank you!

With that, the four bugs I found in newlib's C code are dead. I think the lrint fix should be upstreamed right away; would you mind if I credited you in the patch?

Unfortunately, we are not out of the woods yet. If memory serves me correctly, newlib has some assembly code in used for things like `_start` to bring up the C runtime, which GNU binutils accepts and Clang's assembler currently does not.

I'll go build that so I'm not talking out of my ass.

Hi Dan,

In case you are interested in the baremetal libgloss bits, I made some
changes to get that to compile with clang in
<https://github.com/CTSRD-CHERI/newlib&gt;\. This also contains some
unrelated changes to make it easier to use on QEMU MALTA and to build
in CHERI pure-capability mode but the changes to the MIPS crt0 bits
might be useful.
I believe <https://github.com/CTSRD-CHERI/newlib/commit/2e686345f77a1b745e504ffdbdf012ec78fbfc74&gt;
and possibly some follow-up commits should be sufficient to compile
libgloss with clang (at least for MIPS64).

Alex

[Dan_Ravensloft]

I’ve been somewhat busy with life over the past week, but I thought I’d come back to this all the same.