Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ugly invalidation workaround for SGS7s #8769

Merged
merged 2 commits into from
May 24, 2016

Conversation

unknownbrackets
Copy link
Collaborator

See the journey:
http://forums.ppsspp.org/showthread.php?tid=18456&pid=117898#pid117898

First, this improves CPU name detection so we get something useful on the Galaxy S7. This code was not actually tested on that device, but it works on other devices.

Then, based on the Exynos 8890 chipset specifically, it enables over-invalidation and padding between basic blocks (it crashes if you stop doing either one.) Apparently, padding can be traded for aligning basic blocks, but: (a) it required aligning to 256, which probably wasted more than padding 128 bytes, and (b) it may have only been working since it was effectively padding the larger majority of blocks.

Very open to other ideas. Hoping that this is a bug that newer implementations won't have.

Still concerned we might be doing something wrong, since presumably JavaScript jits and such work, but I have no idea what that might be.

-[Unknown]

Some devices don't provide any useful information at all, such as the
Galaxy S7.
Otherwise they just crash, and crash often.  Special thanks to Jaaan for
numerous trials to try to find the best way to solve the crashes.
@hrydgard
Copy link
Owner

If it is a multi core interaction, maybe setting thread affinity to specific cores would help? Anyway, that's a separate experiment...

@hrydgard hrydgard merged commit 9b63a44 into hrydgard:master May 24, 2016
@unknownbrackets unknownbrackets deleted the cpuinfo branch May 24, 2016 14:13
@unknownbrackets
Copy link
Collaborator Author

Just double checked - this compiles fine with r11, so it's just a problem with r10.

Maybe we can do this:
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20150720/289185.html

It's less ugly than what Chromium did...

It seems like they did add it back in r11, but not sure if it's permanent. All I find searching for it are people hacking around it not being publicly exported in r10.

-[Unknown]

@unknownbrackets
Copy link
Collaborator Author

Also tried using exec, we can't get it that way AFAICT.

Build.MODEL, Build.PRODUCT, and Build.BOARD are all available in Java, so I guess we should pass them in from there....

-[Unknown]

@hrydgard
Copy link
Owner

hrydgard commented Sep 9, 2016

@unknownbrackets So apparently .. dolphin-emu/dolphin#4204

Finally an explanation that makes sense!

@hrydgard
Copy link
Owner

hrydgard commented Sep 9, 2016

Oh yeah, this may be less of an issue since we switched to LLVM, because it checks the cacheline size every time:
https://github.com/llvm-mirror/compiler-rt/blob/ff75f2a0260b1940436a483413091c5770427c04/lib/builtins/clear_cache.c#L146

Surely that's a race condition though? What if your thread gets migrated from a CPU with one cacheline size to a CPU with another right at the point between where you compute your cacheline size and where you actually clear?

@degasus
Copy link

degasus commented Sep 10, 2016

@hrydgard Do you really care about clearing the icache after moving the CPU? It's very likely not cached anyways.

@hrydgard
Copy link
Owner

@degasus errr.... you are obviously right. For anything to work it must wipe the cache when moving cores anyway, so it'll be safe. I blame that on late night posting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants