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

HW/GPFifo: Refactor to class, move to Core::System. #11407

Merged
merged 2 commits into from Jan 9, 2023

Conversation

AdmiralCurtiss
Copy link
Contributor

@AdmiralCurtiss AdmiralCurtiss commented Jan 5, 2023

Do not merge this yet, this appears to noticeably impact performance... e: On second try I'm actually getting slighty better performance post-refactor so what the heck do I know, modern CPUs are a mystery.

Someone should also check the ARM JIT change.

@AdmiralCurtiss AdmiralCurtiss marked this pull request as ready for review January 5, 2023 04:50
@AdmiralCurtiss
Copy link
Contributor Author

Someone should still test the ARM JIT change before merging.

@JosJuice
Copy link
Member

JosJuice commented Jan 5, 2023

JitArm64 change tested and reviewed. Looks good.

Actually, the other day I was looking into if it would be possible to call a member function directly from the JIT. I couldn't figure out a way to get C++ to give me a plain pointer for the function, so maybe creating a wrapper function like you've done here is the way to go?

@dvessel
Copy link
Contributor

dvessel commented Jan 5, 2023

Ran about 10 different games briefly and found no issues. Performance is practically the same. M1 ARM

Running F-Zero, Sand Ocean Intro

Metal GFX:
master 5.0-18167-7cd3839-7cd3839
11436 frames 4.735ms, v-blanks 4.572ms - metal pass 1
11436 frames 4.737ms, v-blanks 4.573ms - metal pass 2
11436 frames 5.176ms, v-blanks 5.007ms - metal pass 3
34308 frames 4.883ms, v-blanks 4.717ms complete

globals-gpfifo 5.0-18168-6caafbb-7cd3839
11436 frames 4.728ms, v-blanks 4.565ms - metal pass 1
11436 frames 4.711ms, v-blanks 4.548ms - metal pass 2
11436 frames 4.725ms, v-blanks 4.560ms - metal pass 3
34308 frames 4.721ms, v-blanks 4.558ms complete

Null GFX:
master 5.0-18167-7cd3839-7cd3839
11436 frames 2.997ms, v-blanks 2.906ms - null pass 1
11436 frames 2.977ms, v-blanks 2.886ms - null pass 2
11436 frames 2.977ms, v-blanks 2.887ms - null pass 3
34308 frames 2.984ms, v-blanks 2.893ms complete

globals-gpfifo 5.0-18168-6caafbb-7cd3839
11436 frames 2.990ms, v-blanks 2.899ms - null pass 1
11436 frames 2.999ms, v-blanks 2.908ms - null pass 2
11436 frames 2.978ms, v-blanks 2.888ms - null pass 3
34308 frames 2.989ms, v-blanks 2.898ms complete

Source/Core/Core/HW/GPFifo.h Outdated Show resolved Hide resolved
Source/Core/Core/HW/GPFifo.h Outdated Show resolved Hide resolved
@AdmiralCurtiss
Copy link
Contributor Author

Actually, the other day I was looking into if it would be possible to call a member function directly from the JIT. I couldn't figure out a way to get C++ to give me a plain pointer for the function, so maybe creating a wrapper function like you've done here is the way to go?

You can actually do that, but the syntax is a bit arcane. https://godbolt.org/z/EPjf74EM6

#include <cstdio>
#include <cstdint>

struct A {
    int func(int64_t v0, double v1) {
        printf("called on instance %llxd with %lld %g\n", (uint64_t)this, v0, v1);
        return 0;
    }
}; 

int main() {
    int (A::* ptr)(int64_t, double) = &A::func;
    
    A a;
    a.func(100, 1.0);
    (a.*ptr)(200, 2.0);
    ((&a)->*ptr)(300, 3.0);

    return 0;
}

You also have to make sure to match your target platform's calling convention for member functions. But yeah, it's just simpler to use a freestanding function and adapt...

@dvessel
Copy link
Contributor

dvessel commented Jan 6, 2023

After that last commit, it crashes about 1 in 5 times when starting a game.

Process 52177 stopped
* thread #18, name = 'Emuthread - Starting', stop reason = EXC_BAD_ACCESS (code=1, address=0x1810ff4a3120ff4a)
    frame #0: 0x000000018e652bc0 libsystem_platform.dylib`__bzero + 32
libsystem_platform.dylib`:
->  0x18e652bc0 <+32>: stnp   q0, q0, [x0]
    0x18e652bc4 <+36>: stnp   q0, q0, [x0, #0x20]
    0x18e652bc8 <+40>: add    x3, x0, #0x40
    0x18e652bcc <+44>: and    x3, x3, #0xffffffffffffffc0
Target 0: (Dolphin) stopped.
(lldb) bt
* thread #18, name = 'Emuthread - Starting', stop reason = EXC_BAD_ACCESS (code=1, address=0x1810ff4a3120ff4a)
  * frame #0: 0x000000018e652bc0 libsystem_platform.dylib`__bzero + 32
    frame #1: 0x000000010076e050 Dolphin`Memory::MemoryManager::Clear() + 168
    frame #2: 0x000000010076dc88 Dolphin`Memory::MemoryManager::Init() + 1136
    frame #3: 0x000000010076baec Dolphin`HW::Init(Sram const*) + 92
    frame #4: 0x00000001005d14a0 Dolphin`Core::EmuThread(std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >, WindowSystemInfo) + 520
    frame #5: 0x00000001005d246c Dolphin`decltype(static_cast<void (*>(fp)(static_cast<std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >>(fp0), static_cast<WindowSystemInfo>(fp0))) std::__1::__invoke<void (*)(std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >, WindowSystemInfo), std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >, WindowSystemInfo>(void (*&&)(std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >, WindowSystemInfo), std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >&&, WindowSystemInfo&&) + 76
    frame #6: 0x00000001005d2370 Dolphin`void std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >, WindowSystemInfo), std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >, WindowSystemInfo, 2ul, 3ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >, WindowSystemInfo), std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >, WindowSystemInfo>&, std::__1::__tuple_indices<2ul, 3ul>) + 80
    frame #7: 0x00000001005d1e18 Dolphin`void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >, WindowSystemInfo), std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >, WindowSystemInfo> >(void*) + 80
    frame #8: 0x000000018e62506c libsystem_pthread.dylib`_pthread_start + 148

another:

Process 52246 stopped
* thread #18, name = 'Emuthread - Starting', stop reason = EXC_BAD_ACCESS (code=1, address=0xff000000ff0000)
    frame #0: 0x000000018e652bc0 libsystem_platform.dylib`__bzero + 32
libsystem_platform.dylib`:
->  0x18e652bc0 <+32>: stnp   q0, q0, [x0]
    0x18e652bc4 <+36>: stnp   q0, q0, [x0, #0x20]
    0x18e652bc8 <+40>: add    x3, x0, #0x40
    0x18e652bcc <+44>: and    x3, x3, #0xffffffffffffffc0
Target 0: (Dolphin) stopped.
(lldb) bt
* thread #18, name = 'Emuthread - Starting', stop reason = EXC_BAD_ACCESS (code=1, address=0xff000000ff0000)
  * frame #0: 0x000000018e652bc0 libsystem_platform.dylib`__bzero + 32
    frame #1: 0x000000010076e050 Dolphin`Memory::MemoryManager::Clear() + 168
    frame #2: 0x000000010076dc88 Dolphin`Memory::MemoryManager::Init() + 1136
    frame #3: 0x000000010076baec Dolphin`HW::Init(Sram const*) + 92
    frame #4: 0x00000001005d14a0 Dolphin`Core::EmuThread(std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >, WindowSystemInfo) + 520
    frame #5: 0x00000001005d246c Dolphin`decltype(static_cast<void (*>(fp)(static_cast<std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >>(fp0), static_cast<WindowSystemInfo>(fp0))) std::__1::__invoke<void (*)(std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >, WindowSystemInfo), std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >, WindowSystemInfo>(void (*&&)(std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >, WindowSystemInfo), std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >&&, WindowSystemInfo&&) + 76
    frame #6: 0x00000001005d2370 Dolphin`void std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >, WindowSystemInfo), std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >, WindowSystemInfo, 2ul, 3ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >, WindowSystemInfo), std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >, WindowSystemInfo>&, std::__1::__tuple_indices<2ul, 3ul>) + 80
    frame #7: 0x00000001005d1e18 Dolphin`void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >, WindowSystemInfo), std::__1::unique_ptr<BootParameters, std::__1::default_delete<BootParameters> >, WindowSystemInfo> >(void*) + 80
    frame #8: 0x000000018e62506c libsystem_pthread.dylib`_pthread_start + 148

@AdmiralCurtiss
Copy link
Contributor Author

Hm, can't reproduce that here, and I'm not sure what part of the change would have triggered that either...

@AdmiralCurtiss
Copy link
Contributor Author

AdmiralCurtiss commented Jan 6, 2023

Does this fix it, perhaps?

Copy link
Member

@lioncash lioncash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two minor nits, but overall looks good

Source/Core/Core/HW/GPFifo.h Outdated Show resolved Hide resolved
Source/Core/Core/System.cpp Outdated Show resolved Hide resolved
@dvessel
Copy link
Contributor

dvessel commented Jan 6, 2023

Does this fix it, perhaps?

All good now. Zero crashes.

@JosJuice
Copy link
Member

JosJuice commented Jan 6, 2023

@AdmiralCurtiss Sorry, what I mean by plain pointer is actually reinterpret_cast<uintptr_t>(&A::func) (so that I can store the address in a register). The compiler says that that cast is forbidden.

dvessel added a commit to dvessel/dolphin that referenced this pull request Jan 6, 2023
@AdmiralCurtiss AdmiralCurtiss merged commit 21c29ba into dolphin-emu:master Jan 9, 2023
11 checks passed
@AdmiralCurtiss AdmiralCurtiss deleted the globals-gpfifo branch January 9, 2023 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants