Add validation for destination of atomic operations #6093

aleino-nv · 2025-01-15T11:59:29Z

Rewrite GLSL atomic operations in terms of the intrinsic IR atomic operations
Add validation for destination of atomic operations
Add some tests that the validation is working as expected

This closes issue #5989.

aleino-nv · 2025-01-15T13:32:18Z

@csyonghe This validation currently breaks the following tests:

// tests/spirv/address-space-specialize.slang
groupshared int gArray1[2];
void atomicOp(inout int array[2])
{
    InterlockedAdd(array[0], 1); 
}
[numthreads(1,1,1)]
void main()
{
    atomicOp(gArray1);
}

How can I detect that array is groupshared in the above case?
The test tests/spirv/ptr-vector-member.slang seems to be doing atomics on a push constant. Is that supposed to be allowed?

csyonghe · 2025-01-15T23:47:27Z

you need to call getRootAddr(inst) to get the root of any addressing instruction (e.g. GetElementAddr, FieldAddress), then check the type of the root inst.

If the root addr is kIROp_RWStructuredBufferGetElementPtr, then the addr is in a hlsl structured buffer so atomic operations should be allowed.

If the root addr is ImageSubscript, then the addr is in an image so atomic operations should be allowed.

If the root addr is a pointer where the address space is one of UserPointer, Global or StorageBuffer, then atomic operations should be allowed.

In tests/spirv/ptr-vector-member.slang, the operand of the atomic operation is a pointer (whose address space is UserPointer), the pointer itself is a push constant, but that is irrelevant to the issue: the address where we are performing the atomic operation on is in PhyiscalStorageBuffer address space since we are doing atomics on a user defined pointer.

aleino-nv · 2025-01-16T09:02:17Z

you need to call getRootAddr(inst) to get the root of any addressing instruction (e.g. GetElementAddr, FieldAddress), then check the type of the root inst.

Yes, I'm following addressing instructions to get to the root.
(I will switch to use getRootAddr where I can once I get all the tests passing. At the moment I believe I need to chase through a few more indirections than getRootAddr currently does, namely Load, GetElementPtr and GetOffsetPtr.)

If the root addr is a pointer where the address space is one of UserPointer, Global or StorageBuffer, then atomic operations should be allowed.

Thanks, I hadn't added the ptr case yet and that's why ptr-vector-member was failing.

In tests/spirv/ptr-vector-member.slang, the operand of the atomic operation is a pointer (whose address space is UserPointer), the pointer itself is a push constant, but that is irrelevant to the issue: the address where we are performing the atomic operation on is in PhyiscalStorageBuffer address space since we are doing atomics on a user defined pointer.

Ok, the syntax mislead me. I think would have expected the syntax InterlockedAdd(v, 1) for what you're describing, but instead the test code reads:

uint * v = &push2.value[0].x;
InterlockedAdd(*v, 1);

aleino-nv · 2025-01-16T09:14:19Z

I'm still having an issue with this case:

// tests/spirv/address-space-specialize.slang
groupshared int gArray1[2];
void atomicOp(inout int array[2])
{
    InterlockedAdd(array[0], 1); 
}
[numthreads(1,1,1)]
void main()
{
    atomicOp(gArray1);
}

@csyonghe When I chase array[0] to get to a root addr, I just end up at IRParam.
This is not really surprising, but I wonder what the solution should be.

I see a few options:

atomicOp must be inlined before running this validation.
The parameter of atomicOp must be marked up somehow so that it only accepts parameters that lead back to valid atomic operation destinations.
The validation logic should be elaborated such that in this case we find all calls to this function and then run the validation on the function call argument corresponding to the parameter.

csyonghe · 2025-01-16T16:47:23Z

I do t think we should be chasing OpLoad, because InterlockedAdd(*ptr, …) does not translate to AtomicAdd(Load(ptr)) and is just AtomicAdd(ptr).

*ptr returns a reference, and at IR level a reference is still a pointer.

using atomic operation on an inout array is technically unsound and we may want to disallow this in the future, but for now, make sure your check is after address space specialization pass to get rid of the issue. After address space specialization, the IRParam’s type will be a IROutTypeBase with a specialized address space, and you can apply the same rules to check that address space.

aleino-nv · 2025-01-17T10:49:54Z

I do t think we should be chasing OpLoad, because InterlockedAdd(*ptr, …) does not translate to AtomicAdd(Load(ptr)) and is just AtomicAdd(ptr).

Done -- removed.
(This was not specifically for AtomicAdd. I think I don't need to chase through OpLoad anymore after I added detection for the ptr cases.)

using atomic operation on an inout array is technically unsound and we may want to disallow this in the future, but for now, make sure your check is after address space specialization pass to get rid of the issue. After address space specialization, the IRParam’s type will be a IROutTypeBase with a specialized address space, and you can apply the same rules to check that address space.

Thanks, that made the test pass.

aleino-nv · 2025-01-17T10:51:18Z

/format

slangbot · 2025-01-17T10:52:05Z

🌈 Formatted, please merge the changes from this PR

aleino-nv · 2025-01-17T12:24:31Z

/format

slangbot · 2025-01-17T12:25:17Z

🌈 Formatted, please merge the changes from this PR

Format code for PR shader-slang#6093

csyonghe · 2025-01-17T18:39:17Z

source/slang/slang-emit.cpp

@@ -1322,6 +1322,11 @@ Result linkAndOptimizeIR(
            byteAddressBufferOptions);
    }

+    // For SPIR-V, this function is called elsewhere, so that it can happen after address space


This could mean we will still be generating errors when emitting hlsl or other non-spirv target code.

I think we should amend validateAtomicOperations such that for any non-spirv target code, we silently treat inout/out IRParam as valid target for atomic operations for now.

I checked that it works for Metal, at least. (Address space specialization is run in Metal IR legalization, which happens before this point in the code.)

I think we should amend validateAtomicOperations such that for any non-spirv target code, we silently treat inout/out IRParam as valid target for atomic operations for now.

Sure, then I guess I don't need two separate call sites for this function. (Edit: well, not if we only make the exception for non-SPIR-V targets... I'll keep the separate call site then.)

Done: I added a parameter that lets the caller specify whether to skip full IRParam checks in this way.
The caller can set this parameter to false for SPIR-V and true for other targets.

csyonghe · 2025-01-17T18:40:55Z

source/slang/slang-ir-validate.cpp

+{
+    // There may be unused functions containing violations after address space specialization.
+    if (auto func = as<IRFunc>(inst))
+        if (!(func->hasUses() || func->findDecoration<IREntryPointDecoration>()))


I think we can remove this check because all functions that exist in the IR at this point are existing for a reason, due to DCE has already run.

I added this because of the IRParam issue: it seemed like the original function (with inout IRParam) was left in the IR with no uses after address space specialization had run.

Confirmed: I still hit this issue in the call from slang-ir-spirv-legalize.cpp (since that's SPIR-V target I don't allow skipping the validation of the IRParam, as for other targets), if I delete this check.

Some options I can see:

Do the check in address space specialization and clean up any non-entrypoint functions left without uses there.

Keep the check here.

Move the call to validateAtomicOperations to some other place where the function has been cleaned up.

Done. I attempted the first option and removed the check.

csyonghe · 2025-01-17T18:42:36Z

tests/glsl-intrinsic/atomic/invalidDestAdd.slang

@@ -0,0 +1,13 @@
+//DIAGNOSTIC_TEST:SIMPLE(filecheck=CHECK):  -allow-glsl -stage compute -entry computeMain -target glsl -DTARGET_GLSL


You can merge all these tests into one single file, and use

// CHECK: ([[# @LINE+1]]): error 41043

before all the atomic calls to ensure there are error messages for each one of them.

aleino-nv · 2025-01-20T12:06:42Z

/format

slangbot · 2025-01-20T12:07:25Z

🌈 Formatted, please merge the changes from this PR

Format code for PR shader-slang#6093

Many of these functions map directly to atomic IR instructions. The functions taking atomic_uint are left as they are. This helps to address shader-slang#5989, since the destination pointer type validation can then be written only for the atomic IR instructions.

Diagnose an error if the destination of the atomic operation is not appropriate, where appropriate means it's either: - 'groupshared' - from a device buffer This closes shader-slang#5989.

Attempting to use the GLSL atomic functions on destinations that are neither groupshared nor from a device buffer should fail with the following error: error 41403: cannot perform atomic operation because destination is neither groupshared nor from a device buffer.

Address space specialization for SPIR-V is not done as part of `linkAndOptimizeIR`, as it is for e.g. Metal, so opt out and add a separate call for SPIR-V.

aleino-nv · 2025-01-22T06:30:22Z

I changed the entrypoint check into an assert, as discussed yesterday.

aleino-nv · 2025-01-22T06:45:54Z

/format

slangbot · 2025-01-22T06:46:47Z

🌈 Formatted, please merge the changes from this PR

Format code for PR shader-slang#6093

aleino-nv force-pushed the aleino/atomics branch from ffeaabc to 070707b Compare January 15, 2025 13:23

aleino-nv added the pr: non-breaking PRs without breaking changes label Jan 15, 2025

aleino-nv changed the title ~~Aleino/atomics~~ Add validation for destination of atomic operations Jan 15, 2025

aleino-nv force-pushed the aleino/atomics branch from 070707b to 9ddc2c2 Compare January 16, 2025 08:47

aleino-nv force-pushed the aleino/atomics branch from 9ddc2c2 to 797fade Compare January 16, 2025 09:04

aleino-nv force-pushed the aleino/atomics branch from 797fade to 509fa2a Compare January 17, 2025 10:39

slangbot mentioned this pull request Jan 17, 2025

Format code for PR #6093 aleino-nv/slang#38

Merged

aleino-nv force-pushed the aleino/atomics branch 2 times, most recently from e4491f3 to 82dfacf Compare January 17, 2025 11:41

aleino-nv added a commit to aleino-nv/slang that referenced this pull request Jan 17, 2025

Merge pull request #38 from slangbot/format-6093-aleino/atomics

d67584b

Format code for PR shader-slang#6093

aleino-nv marked this pull request as ready for review January 17, 2025 12:29

aleino-nv requested a review from a team as a code owner January 17, 2025 12:29

csyonghe reviewed Jan 17, 2025

View reviewed changes

aleino-nv force-pushed the aleino/atomics branch from d67584b to 5e60b4f Compare January 20, 2025 08:29

aleino-nv requested a review from csyonghe January 20, 2025 08:31

aleino-nv force-pushed the aleino/atomics branch 2 times, most recently from fea98f0 to 0c9e9a7 Compare January 20, 2025 12:05

slangbot mentioned this pull request Jan 20, 2025

Format code for PR #6093 aleino-nv/slang#39

Merged

aleino-nv added a commit to aleino-nv/slang that referenced this pull request Jan 20, 2025

Merge pull request #39 from slangbot/format-6093-aleino/atomics

e854a8b

Format code for PR shader-slang#6093

csyonghe previously approved these changes Jan 21, 2025

View reviewed changes

aleino-nv enabled auto-merge (squash) January 22, 2025 06:19

aleino-nv disabled auto-merge January 22, 2025 06:22

aleino-nv added 6 commits January 22, 2025 08:23

Add validation for atomic operations

b33fc07

Diagnose an error if the destination of the atomic operation is not appropriate, where appropriate means it's either: - 'groupshared' - from a device buffer This closes shader-slang#5989.

Validate atomic operations after address space specialization

3c1e4f8

Address space specialization for SPIR-V is not done as part of `linkAndOptimizeIR`, as it is for e.g. Metal, so opt out and add a separate call for SPIR-V.

Allow unchecked in/inout parameters for non-SPIRV targets

d2f0eb1

Clean up callees left without uses during address space specialziation

c32ab5f

aleino-nv dismissed csyonghe’s stale review via c32ab5f January 22, 2025 06:29

aleino-nv force-pushed the aleino/atomics branch from 1c823ac to c32ab5f Compare January 22, 2025 06:29

format code

240b905

slangbot mentioned this pull request Jan 22, 2025

Format code for PR #6093 aleino-nv/slang#40

Merged

Merge pull request #40 from slangbot/format-6093-aleino/atomics

fc57786

Format code for PR shader-slang#6093

aleino-nv enabled auto-merge (squash) January 22, 2025 12:26

Merge branch 'master' into aleino/atomics

f4b4041

aleino-nv requested a review from csyonghe January 22, 2025 13:22

csyonghe approved these changes Jan 22, 2025

View reviewed changes

Merge branch 'master' into aleino/atomics

efb9dbb

csyonghe disabled auto-merge January 22, 2025 17:10

csyonghe merged commit 04353fb into shader-slang:master Jan 22, 2025
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add validation for destination of atomic operations #6093

Add validation for destination of atomic operations #6093

aleino-nv commented Jan 15, 2025 •

edited

Loading

aleino-nv commented Jan 15, 2025

csyonghe commented Jan 15, 2025 •

edited

Loading

aleino-nv commented Jan 16, 2025

aleino-nv commented Jan 16, 2025 •

edited

Loading

csyonghe commented Jan 16, 2025

aleino-nv commented Jan 17, 2025

aleino-nv commented Jan 17, 2025

slangbot commented Jan 17, 2025

aleino-nv commented Jan 17, 2025

slangbot commented Jan 17, 2025

csyonghe Jan 17, 2025

aleino-nv Jan 20, 2025 •

edited

Loading

aleino-nv Jan 20, 2025

csyonghe Jan 17, 2025

aleino-nv Jan 20, 2025 •

edited

Loading

aleino-nv Jan 20, 2025

aleino-nv Jan 20, 2025

csyonghe Jan 17, 2025

aleino-nv Jan 20, 2025

aleino-nv commented Jan 20, 2025

slangbot commented Jan 20, 2025

aleino-nv commented Jan 22, 2025

aleino-nv commented Jan 22, 2025

slangbot commented Jan 22, 2025

		@@ -0,0 +1,13 @@
		//DIAGNOSTIC_TEST:SIMPLE(filecheck=CHECK): -allow-glsl -stage compute -entry computeMain -target glsl -DTARGET_GLSL

Add validation for destination of atomic operations #6093

Add validation for destination of atomic operations #6093

Conversation

aleino-nv commented Jan 15, 2025 • edited Loading

aleino-nv commented Jan 15, 2025

csyonghe commented Jan 15, 2025 • edited Loading

aleino-nv commented Jan 16, 2025

aleino-nv commented Jan 16, 2025 • edited Loading

csyonghe commented Jan 16, 2025

aleino-nv commented Jan 17, 2025

aleino-nv commented Jan 17, 2025

slangbot commented Jan 17, 2025

aleino-nv commented Jan 17, 2025

slangbot commented Jan 17, 2025

csyonghe Jan 17, 2025

Choose a reason for hiding this comment

aleino-nv Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

aleino-nv Jan 20, 2025

Choose a reason for hiding this comment

csyonghe Jan 17, 2025

Choose a reason for hiding this comment

aleino-nv Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

aleino-nv Jan 20, 2025

Choose a reason for hiding this comment

aleino-nv Jan 20, 2025

Choose a reason for hiding this comment

csyonghe Jan 17, 2025

Choose a reason for hiding this comment

aleino-nv Jan 20, 2025

Choose a reason for hiding this comment

aleino-nv commented Jan 20, 2025

slangbot commented Jan 20, 2025

aleino-nv commented Jan 22, 2025

aleino-nv commented Jan 22, 2025

slangbot commented Jan 22, 2025

aleino-nv commented Jan 15, 2025 •

edited

Loading

csyonghe commented Jan 15, 2025 •

edited

Loading

aleino-nv commented Jan 16, 2025 •

edited

Loading

aleino-nv Jan 20, 2025 •

edited

Loading

aleino-nv Jan 20, 2025 •

edited

Loading