Big Endian fixes. #13399

aliaspider · 2020-09-07T20:47:05Z

this PR focuses mostly on the simple / functional big endian fixes to get those out of the way first.
gfx and audio problems are a bit more complex and were deliberately left out. they can be addressed better later on with separate PRs.

unknownbrackets · 2020-09-07T22:03:35Z

How is performance? Do games at least generally run well, or are we sacrificing the complexity of to_le<> and LEndian<> in lots of places for mostly not great performance?

I liked the previous u32_le etc. because there was almost no impact on code readability and you could just do = 0 etc. Now I see that u64_le has to be treated as a struct.

-[Unknown]

aliaspider · 2020-09-07T22:37:53Z

LEndian<T> is just a typedef for T on little endian targets so that one is harmless.

I had to remove the constructor to make swap_t<> a pod struct, and much like the PSPPointer<> struct, an explicit constructor was needed in some cases which is what the to_le<> function does. I would be surprised if it affected code generation on little endian targets since it is a no-op there.

while the non-pod struct does have its benefits, it did hide a lot of bad uses which were mostly bugs or performace loss on big endian hosts. swap_t<> should mainly be used a view into memory so requiring an explicit constructor is rarely needed.

https://github.com/hrydgard/ppsspp/blob/master/Core/HLE/HLE.cpp#L79-L93
this union was the main reason why I went with this change since a non-pod swap_t<> would require naming both structs in that union. there is also the problem of packed structs that don't like having non-pod types as member.

I liked the previous u32_le etc. because there was almost no impact on code readability and you could just do = 0 etc. Now I see that u64_le has to be treated as a struct.

= 0 can still be used, just not during variable declaration since an assignment there counts as a cast. u64_le var; var = 0; would work for example but not u64_le var = 0;

aliaspider · 2020-09-07T23:18:16Z

if to_le<> is too annoying from a code readability point of view, I can remove it and try to find a less intrusive way to get past the build errors.

unknownbrackets · 2020-09-08T21:07:34Z

Well, just trying to understand the gain is all.

Is there a reason we'd want a BE PSPPointer? Is it just to optimize the few cases in sceCcc where a PSPPointer is used not to directly reference memory? If we want to optimize those cases, it's probably best to convert to host pointers instead.

-[Unknown]

aliaspider · 2020-09-08T23:24:37Z

well I assumed a PSPPointer object was still useful in some way, even if it didn't reside in emulated little endian memory.
you think using a host pointer for those cases and reverting PSPPointer to the way it was would be better ?

I didn't want to break the original design completely that is all, and it seemed wrong to use an u32_le to store the value when an u32 would be enough.

aliaspider · 2020-09-09T13:19:21Z

this PR got a little more complex than I had originally planned, so I've decided to nuke it all and start over!
Big-endian support, while being a nice thing to have, shouldn't be getting in the way too much. and the few spared cycles here and there wouldn't have mattered in the end anyway.

the only immediate downside was that I had to name two structs in an unnamed union.

also allow using the scalar_storage_order attribute to enforce endianess (gcc only).

members.

aliaspider · 2020-10-03T01:24:04Z

Core/HLE/HLE.cpp

 		struct {
 			u32_le ra;
 			u32_le v0;
 			u32_le v1;
-		};
+		} regs;
 	};
 };



is this struct ever read by non native code ? because, as the name suggests, it is HLE, so even if the data lives in psp memory, it might not necessarily need to be little endian.
I did test it briefly with native types and nothing collapsed, so maybe that's the much simpler solution to this as opposed to naming the structs ?

Hm, I think that might be fine indeed... Although, savestate compatibility might suffer?

oh yeah savestates. even if they might not be entirely endian safe right now, let's not make the matter worse.
ok better to leave it like this then I guess...

variables to their native types when used as parameters in GenericLog calls.

unknownbrackets · 2021-02-21T18:18:21Z

Got a bunch of this merged in #14180, as well as a few extra missing ones with index/vertex decoding. Unfortunately, that means this'll need a rebase.

-[Unknown]

archanox · 2021-09-07T14:02:32Z

Hi @aliaspider, I saw this PR and I thought I'd give building upstream on my G5 Quad.
After configuring a few things in the CMakeLists file and a few other configy things to keep it happy, I was able to get so far during the build.

[ 38%] Building CXX object ext/armips/CMakeFiles/armips.dir/Util/CRC.cpp.o
In file included from /root/ppsspp/Core/MIPS/fake/FakeJit.cpp:27:
In file included from /root/ppsspp/Core/MIPS/MIPS.h:24:
/root/ppsspp/Common/Data/Random/Rng.h:52:25: error: use of overloaded operator '%' is ambiguous (with operand types 'swap_struct_t<unsigned int, swap_32_t<unsigned int>>::swapped_t' (aka 'swap_struct_t<unsigned int, swap_32_t<unsigned int>>') and 'MersenneTwister::(anonymous enum at /root/ppsspp/Common/Data/Random/Rng.h:57:2)')
                index_ = (index_ + 1) % MT_SIZE;
                         ~~~~~~~~~~~~ ^ ~~~~~~~
/root/ppsspp/Common/Data/Random/Rng.h:52:25: note: built-in candidate operator%(int, int)
/root/ppsspp/Common/Data/Random/Rng.h:52:25: note: built-in candidate operator%(long, int)
/root/ppsspp/Common/Data/Random/Rng.h:52:25: note: built-in candidate operator%(long long, int)
/root/ppsspp/Common/Data/Random/Rng.h:52:25: note: built-in candidate operator%(__int128, int)
/root/ppsspp/Common/Data/Random/Rng.h:52:25: note: built-in candidate operator%(unsigned int, int)
/root/ppsspp/Common/Data/Random/Rng.h:52:25: note: built-in candidate operator%(unsigned long, int)
/root/ppsspp/Common/Data/Random/Rng.h:52:25: note: built-in candidate operator%(unsigned long long, int)
/root/ppsspp/Common/Data/Random/Rng.h:52:25: note: built-in candidate operator%(unsigned __int128, int)
1 error generated.
make[2]: *** [CMakeFiles/Common.dir/build.make:258: CMakeFiles/Common.dir/Core/MIPS/fake/FakeJit.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:501: CMakeFiles/Common.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....

What platform are you targeting for this big endian work?

archanox · 2021-09-08T12:02:42Z

I've subsequently pulled in your branch @aliaspider and applied my changes and now I get the following error

[ 67%] Building CXX object CMakeFiles/Core.dir/Core/ELF/PrxDecrypter.cpp.o
2 errors generated.
make[2]: *** [CMakeFiles/Core.dir/build.make:1280: CMakeFiles/Core.dir/Core/Dialog/SavedataParam.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [CMakeFiles/Makefile2:593: CMakeFiles/Core.dir/all] Error 2
make: *** [Makefile:136: all] Error 2

I'm not sure if this is a better error or a worse one than the one I tried on upstream. It does appear to get further in the build process mind you (67% vs. 38%).

aliaspider · 2021-09-11T14:39:36Z

Those are fixes I pulled from my Wii-U branch (32-bit PowerPC 750), but it should still build for your target platform too.
that error with the modulo operator is a bit unexpected, although it might just be because you were building against upstream and this PR is a bit outdated.
the second log is missing the actual errors, so I can't really tell what happened there.

also keep in mind that this PR isn't a complete solution, so don't expect it to actually run even if it builds.

aliaspider force-pushed the master branch from 0c206e5 to 478474e Compare September 8, 2020 16:04

aliaspider marked this pull request as draft September 8, 2020 19:41

aliaspider force-pushed the master branch from 6f4badb to cb3fda0 Compare September 9, 2020 13:19

aliaspider marked this pull request as ready for review September 9, 2020 13:24

aliaspider force-pushed the master branch from cb3fda0 to 9329947 Compare September 9, 2020 22:46

aliaspider force-pushed the master branch 2 times, most recently from 5010d10 to 3d1c20e Compare September 21, 2020 11:07

aliaspider force-pushed the master branch from 3d1c20e to 2f0739c Compare October 2, 2020 22:52

aliaspider added 4 commits October 3, 2020 00:30

simplify Common/Swap.h

cbbc963

libkirk: replace some magic offsets with struct member access. this will

e64c9b4

also allow using the scalar_storage_order attribute to enforce endianess (gcc only).

unnamed structs aren't allowed in unnamed unions when using non-pod

22df398

members.

big-endian fixes.

936f309

aliaspider force-pushed the master branch from d5760f4 to 1e139df Compare October 2, 2020 23:32

aliaspider commented Oct 3, 2020

View reviewed changes

add some parameter pack overloads to automatically cast swap_t<>

44ac4a8

variables to their native types when used as parameters in GenericLog calls.

aliaspider force-pushed the master branch from 1e139df to 44ac4a8 Compare October 3, 2020 23:59

unknownbrackets mentioned this pull request Feb 19, 2021

Correct a bunch of endian casts #14180

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Big Endian fixes. #13399

Big Endian fixes. #13399

aliaspider commented Sep 7, 2020

unknownbrackets commented Sep 7, 2020

aliaspider commented Sep 7, 2020 •

edited

aliaspider commented Sep 7, 2020

unknownbrackets commented Sep 8, 2020

aliaspider commented Sep 8, 2020

aliaspider commented Sep 9, 2020

aliaspider Oct 3, 2020

hrydgard Oct 3, 2020 •

edited

aliaspider Oct 3, 2020

unknownbrackets commented Feb 21, 2021

archanox commented Sep 7, 2021 •

edited

archanox commented Sep 8, 2021

aliaspider commented Sep 11, 2021 •

edited

Big Endian fixes. #13399

Are you sure you want to change the base?

Big Endian fixes. #13399

Conversation

aliaspider commented Sep 7, 2020

unknownbrackets commented Sep 7, 2020

aliaspider commented Sep 7, 2020 • edited

aliaspider commented Sep 7, 2020

unknownbrackets commented Sep 8, 2020

aliaspider commented Sep 8, 2020

aliaspider commented Sep 9, 2020

aliaspider Oct 3, 2020

Choose a reason for hiding this comment

hrydgard Oct 3, 2020 • edited

Choose a reason for hiding this comment

aliaspider Oct 3, 2020

Choose a reason for hiding this comment

unknownbrackets commented Feb 21, 2021

archanox commented Sep 7, 2021 • edited

archanox commented Sep 8, 2021

aliaspider commented Sep 11, 2021 • edited

aliaspider commented Sep 7, 2020 •

edited

hrydgard Oct 3, 2020 •

edited

archanox commented Sep 7, 2021 •

edited

aliaspider commented Sep 11, 2021 •

edited