Linux patches review

[gregory]

Just a thread to do some reviewing of my patches (or others) before breaking everythings

[gregory]

Here the first patch.

Some inline cause some failure in Gcc. When there is no symbol problem, I replace __forceinline by __inline. So gcc just inline the function wihtout complaining... In the others case I remove __forceinline because it cause some missing symbol failure (if gcc inline it by __inline)...

Note: I have the problem with both cmake and codeblock (Release mode). Am I the only one impacted ?

[rama]

For the sake of compatibility, I guess we could settle with _inline in all cases.
It's just that MSVC seemed to take _inline as a mildly funny suggestion, and ignore it.
Maybe better to wait and see what Jake has to say..

[gregory]

In fact I think inline depend on the standard but also on compiler extension which is the main reason what is a mess.

Gcc behavior is strange, may be a check with gcc-4.5 could be a good idea. Well gcc is some situation must also take it as a suggestion. The reason is inlining tend to increase binary size and thus increase cache pressure. Just do a quick test I remove Attribute always inline, the code size drop from 4.8MB to 4.1MB (inline = +17%).

In case we replace all __forceinline with _inline expect link failure everywhere in linux build. I thinks it is related to extern inline which does not work as expected.

Here an example of clamp_mix function (which is not inline in linux currently)
* Mixer.cpp. The function is defined here and used.
* Mixer.h contains the header with extern keyword
* Reverb.cpp use also the clamp_mix functions

The issue is after the compilation of mixer.cpp into object. The symbol clamp_mix does not exists (because the function is inline the function does not exist). So there is a missing symbol in Reverb.cpp.

[Air]

Well gcc is some situation must also take it as a suggestion. The reason is inlining tend to increase binary size and thus increase cache pressure. Just do a quick test I remove Attribute always inline, the code size drop from 4.8MB to 4.1MB (inline = +17%).

Yeah, and that's pretty well irrelevant on today's CPUs (anything with 1MB L2 or better). We did extensive testing on inlining, and we can't find a single scenario where inlining aggressively penalizes PCSX2 performance. Even in what seemed to me entirely brutal code cache pressure, the __forceinline performance won over. MSVC tends to be really conservative about inlining in general, which is why we started pasting it all over creation. And sure enough, the more __forceinlines we used, the faster the emu performed.

Here an example of clamp_mix function (which is not inline in linux currently)
* Mixer.cpp. The function is defined here and used.
* Mixer.h contains the header with extern keyword
* Reverb.cpp use also the clamp_mix functions

Unless I'm mistaken, we have situations like that all through PCSX2; which uses __forceinline on hundreds of functions. Why does it only cause problems in SPU2-X?

[Air]

OH, I know why, I bet. GCC gets buggy when you mix __fastcall and __forceinline. I used to do that back when I first started working on SPU2-X, because I still didn't trust MSVC to inline aggressively even with __forceinline. Turns out it almost always inlines, so the __fastcalls can be safely removed. The calling convention should never matter anyway with proper inlining.

[gregory]

I agree with CPU cache. And l3 cache are more bigger.

If I remenber correctly arcum change some inline in others place. But I will check it. Thanks for the return.

[gregory]

It fix the problem except for MulShr32 function. However the issue go away when I declare static the function !!! The function is only used on the mixer file so I think you can do it.

[Air]

Yup, that's fine.

[gregory]

Hi, here an others set.

+ inline fix: I take the hint of gigaherz. Duplicate the clamp_mix function that cause the failure. With the keyword static gcc is happy and do the inline. However I found an interesting behavior, clamp_mix function (before & after patches) are not inlined in Reverb.cpp. I try to also declare the clamp function (on reverb.cpp) static but it causes some inline error in others places... Opinion are welcome.

+ gamefixes_like_speedhach: I join a screenshot of the new interface. I move the check box at the top and change heading message. It is now more similar to speedhack panels.

+ gs log api: I update zzogl plugins to use the GSsetLogDir API. Note I also change the name of the log file to avoid some collision when you using an others gs plugins.

Feedback are welcome.