Implement Load/Store Local/Shared and Atomic shared using new instructions #5241

gdkchan · 2023-06-05T19:20:04Z

This is the last PR in the series of PRs to migrate memory related shader operations to the new Load and Store IR instructions that also carry a StorageKind, which I implemented a while ago. This one does the local and shared memory types, which are the only ones remaining. Those are not really buffer backed, rather they use some private memory. Shared memory is only valid for compute shaders, and is shared by all invocations on the workgroup, but is not externally visible. Meanwhile, local memory is only visible within a single invocation,

There are 2 helper functions, one for storing small integer (8-bit and 16-bit) values on shared memory, and another for doing atomic operations on signed integers. The latter is only needed on GLSL since SPIR-V has separate instructions for signed and unsigned atomic operations, regardless of the variable type. Both were moved out of the backend and are now generated at the IR level, like what I did on the previous changes.

Unlike the old implementation, this one allows multiple local memories and shared memories to exist. The old one only allowed one. This can be useful if we combine shader stages in the future for example. It is also good for symmetry: We have the first operand of all Load and Store operations reserved for the buffer binding number, which must be constant. For local and shared memory, they represent an ID that identifies the memory used.

Advantages are the same of the previous PRs: More code sharing between GLSL and SPIR-V, makes new backends easier to implement, and simpler code (this code manages to remove almost 100 lines of code which is quite nice). It also makes the IR more flexible as we can have multiple separate memory storages now if we need them for different purposes.

This also fixes a regression from #4993, but not a user facing one: The set index value was not being set when generating GLSL code, which made GLSL shaders to not work properly on Vulkan. Currently there's no way for users to enable that, it must be enabled manually by changing the code.

After that, there's still work to be done on the shader refactoring:

Move texture bindings information to ShaderProperties like buffers and memory.
Move remaining values to TranslatorContext and delete ShaderConfig.
Move instruction type to Operation, stop depending on InstructionInfo on GLSL generation and delete InstructionInfo.
Ensure all remaining texture and packing operations can use vector outputs and delete Index field from Operation.
Stop using IGpuAccessor on the backends and store all required information on ShaderProperties.

Testing is welcome.

github-actions · 2023-06-05T20:41:11Z

Download the artifacts for this pull request:

Experimental GUI (Avalonia)

GUI-less (SDL2)

Only for Developers

patrick-hovsepian

minor comments/observations

patrick-hovsepian · 2023-06-05T21:06:58Z

src/Ryujinx.Graphics.Shader/CodeGen/Glsl/Instructions/InstGen.cs

@@ -68,7 +68,7 @@ private static string GetExpression(CodeGenContext context, AstOperation operati

                string args = string.Empty;

-                if (atomic && operation.StorageKind == StorageKind.StorageBuffer)
+                if (atomic && (operation.StorageKind == StorageKind.StorageBuffer || operation.StorageKind == StorageKind.SharedMemory))


super nit: might be more readable with is and or if it works in this context

I'm not a big fan of is, or, and etc expressions. It is easier to understand like this for me.

patrick-hovsepian · 2023-06-05T21:08:45Z

src/Ryujinx.Graphics.Shader/CodeGen/Glsl/OperandManager.cs

+                            if (!(operation.GetSource(0) is AstOperand bindingId) || bindingId.Type != OperandType.Constant)
+                            {
+                                throw new InvalidOperationException($"First input of {operation.Inst} with {operation.StorageKind} storage must be a constant operand.");
+                            }
+
+                            MemoryDefinition memory = operation.StorageKind == StorageKind.LocalMemory
+                                ? context.Config.Properties.LocalMemories[bindingId.Value]
+                                : context.Config.Properties.SharedMemories[bindingId.Value];


worth making a helper function that takes a source index and returns a memory definition so it can be reused here and in InstGenMemory?

I plan to delete that code soon-ish, so I didn't worry too much about de-duplicating it.

src/Ryujinx.Graphics.Shader/Instructions/InstEmitMemory.cs

patrick-hovsepian · 2023-06-05T21:14:50Z

src/Ryujinx.Graphics.Shader/StructuredIr/MemoryDefinition.cs

+        public string Name { get; }
+        public AggregateType Type { get; }
+        public int ArrayLength { get; }


worth marking these as init?

it might imply the constructor too and you could default ArrayLength to 1

What's the benefit of using init here? AFAIK with init the property can only be set on the constructor, but get-only properties also behave like this already, they can only be initialized on the constructor.

my bad, force of habit and I'm partial to object initializer syntax. I also thought by making it a record the compiler would generate the constructor for you

patrick-hovsepian · 2023-06-05T21:17:19Z

src/Ryujinx.Graphics.Shader/Translation/HelperFunctionManager.cs

+
+            Operand result = GenerateSharedAtomicCasLoop(context, wordOffset, id, (memValue) =>
+            {
+                return isMin


worth making this a local function? There's an implication with the closures but I don't recall offhand

I could extract it into a separate function. Both are fine for me.

I think it's fine. I feel like @marco-carvalho might be interested in benchmarking this vs a local function?

patrick-hovsepian · 2023-06-05T21:20:12Z

src/Ryujinx.Graphics.Shader/StructuredIr/MemoryDefinition.cs

+
+namespace Ryujinx.Graphics.Shader.StructuredIr
+{
+    readonly struct MemoryDefinition


worth making it a record to get pass by reference semantics?

Exhigh · 2023-06-12T14:24:22Z

Needs a rebase

…tions

riperiperi

Looking good, sorry for the late review.

gdkchan added enhancement New feature or request gpu Related to Ryujinx.Graphics labels Jun 5, 2023

gdkchan force-pushed the lsmem-v2 branch from 26ba0eb to 6ccfd3b Compare June 5, 2023 19:48

patrick-hovsepian reviewed Jun 5, 2023

View reviewed changes

gdkchan mentioned this pull request Jun 8, 2023

Stop identifying shader textures with handle and cbuf, use binding instead #5266

Merged

marysaka approved these changes Jun 9, 2023

View reviewed changes

gdkchan added 7 commits June 12, 2023 12:26

Implement Load/Store Local/Shared and Atomic shared using new instruc…

475cfbe

…tions

Remove now unused code

7cdf023

Fix base offset register overwrite

5b57fa0

Fix missing storage buffer set index when generating GLSL for Vulkan

a36febf

Shader cache version bump

5639cd1

Remove more unused code

1eb475f

Some PR feedback

a3c5c03

gdkchan force-pushed the lsmem-v2 branch from 0ce9fe4 to a3c5c03 Compare June 12, 2023 15:27

github-actions bot removed the gpu Related to Ryujinx.Graphics label Jun 12, 2023

riperiperi approved these changes Jun 15, 2023

View reviewed changes

gdkchan merged commit f92921a into Ryujinx:master Jun 15, 2023
9 checks passed

gdkchan deleted the lsmem-v2 branch June 15, 2023 20:31

gdkchan mentioned this pull request Sep 10, 2023

Fix shader GlobalToStorage pass when base address comes from local or shared memory #5668

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Load/Store Local/Shared and Atomic shared using new instructions #5241

Implement Load/Store Local/Shared and Atomic shared using new instructions #5241

gdkchan commented Jun 5, 2023

github-actions bot commented Jun 5, 2023 •

edited

patrick-hovsepian left a comment

patrick-hovsepian Jun 5, 2023

gdkchan Jun 5, 2023

patrick-hovsepian Jun 5, 2023

gdkchan Jun 5, 2023

patrick-hovsepian Jun 5, 2023

patrick-hovsepian Jun 5, 2023

gdkchan Jun 5, 2023

patrick-hovsepian Jun 6, 2023

patrick-hovsepian Jun 5, 2023

gdkchan Jun 5, 2023

patrick-hovsepian Jun 6, 2023

patrick-hovsepian Jun 5, 2023

Exhigh commented Jun 12, 2023

riperiperi left a comment

Implement Load/Store Local/Shared and Atomic shared using new instructions #5241

Implement Load/Store Local/Shared and Atomic shared using new instructions #5241

Conversation

gdkchan commented Jun 5, 2023

github-actions bot commented Jun 5, 2023 • edited

patrick-hovsepian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Exhigh commented Jun 12, 2023

riperiperi left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 5, 2023 •

edited