Skip to content

Handle DS_READ_U16, DS_WRITE_B16, DS_ADD_U64 #3007

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

Merged
merged 6 commits into from
Jun 9, 2025

Conversation

mikusp
Copy link
Contributor

@mikusp mikusp commented May 29, 2025

Hit by God of War

@@ -6,6 +6,13 @@

namespace Shader::Backend::SPIRV {

Id EmitLoadSharedU16(EmitContext& ctx, Id offset) {
const Id shift_id{ctx.ConstU32(2U)};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you sure shift by 2 (divide by 4) is still correct for a 16-bit value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea, documentation doesn't really say that, they use addr for all accesses 16/32/64bit

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well I suppose regardless of guest requirements you do need to have the right index for whatever we are using on the SPIR-V side, for example if we have a shared memory u16 array you need to shift 1 over to index by u16.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since you are using an array of 32 bit values, so dividing by 4 will be the base index, and then you need to take either the upper or lower part depending on the first bit:
result = ((offset & 1)=0) ? buf[offset/4] & 0xFFFF : buf[offset/4] & 0xFFFF0000

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, the second bit is not the first, lol
result = ((offset & 2)=0) ? buf[offset/4] & 0xFFFF : buf[offset/4] & 0xFFFF0000

Copy link
Collaborator

Choose a reason for hiding this comment

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

The approach mentioned by @red-prig here is probably correct if we don't have a way in SPIR-V to alias the same shared memory array as a different type (I don't remember if this is the case). The code as of current revision creates two separate arrays which could be problematic if a shader mixes access sizes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Creating aliases usually works fine, the problem is more likely in addressing a 16-bit value, not all GPUs support this, and those that do may work slower

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we are fine in that regard given we already may do so for buffers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm working on a better implementation with aliasing definitions for u16/32/64, as the partial access mentioned by red_prig won't work for atomic operations

@mikusp mikusp force-pushed the load_write_shared_16bit branch 2 times, most recently from 07e0417 to dc8f050 Compare June 3, 2025 19:10
@mikusp mikusp changed the title Handle DS_READ_U16 & DS_WRITE_B16 Handle DS_READ_U16, DS_WRITE_B16, DS_ADD_U64 Jun 4, 2025
@mikusp mikusp force-pushed the load_write_shared_16bit branch 2 times, most recently from 187c22d to 236aacb Compare June 8, 2025 21:06
@mikusp mikusp force-pushed the load_write_shared_16bit branch from 236aacb to 72553c4 Compare June 8, 2025 21:15
@georgemoralis georgemoralis requested review from LNDF and squidbus June 9, 2025 09:56
@georgemoralis
Copy link
Collaborator

Review issues seems to be fixed

@georgemoralis georgemoralis merged commit 217d32b into shadps4-emu:main Jun 9, 2025
12 checks passed
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.

5 participants