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.

Sign up for GitHub

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 validation for destination of atomic operations #6093

Merged
merged 10 commits into from
Jan 22, 2025
Merged

Add validation for destination of atomic operations #6093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9ecdc1e
Reimplement the GLSL atomic* functions in terms of __intrinsic_op
aleino-nv Jan 14, 2025
b33fc07
Add validation for atomic operations
aleino-nv Jan 14, 2025
2d16a1b
Add tests for GLSL atomics destination validation
aleino-nv Jan 15, 2025
3c1e4f8
Validate atomic operations after address space specialization
aleino-nv Jan 17, 2025
d2f0eb1
Allow unchecked in/inout parameters for non-SPIRV targets
aleino-nv Jan 20, 2025
c32ab5f
Clean up callees left without uses during address space specialziation
aleino-nv Jan 20, 2025
240b905
format code
slangbot Jan 22, 2025
fc57786
Merge pull request #40 from slangbot/format-6093-aleino/atomics
aleino-nv Jan 22, 2025
f4b4041
Merge branch 'master' into aleino/atomics
aleino-nv Jan 22, 2025
efb9dbb
Merge branch 'master' into aleino/atomics
csyonghe Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 51 additions & 75 deletions source/slang/glsl.meta.slang
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8467,24 +8467,27 @@ for (const auto& item : atomics)
}}}}


__glsl_version(430)
[ForceInline]
[require(glsl_spirv, atomic_glsl)]
__intrinsic_op($(kIROp_AtomicAdd))
$(item.name) atomicAddWithOrder(inout $(item.name) mem, $(item.name) data, MemoryOrder order);

__glsl_version(430)
[ForceInline]
[require(glsl_spirv, atomic_glsl)]
public $(item.name) atomicAdd(inout $(item.name) mem, $(item.name) data)
{
typeRequireChecks_atomic_using_float1_tier<$(item.name)>();
typeRequireChecks_atomic_using_add<$(item.name)>();
__target_switch
{
case glsl: __intrinsic_asm "atomicAdd($0, $1)";
case spirv:
return spirv_asm
{
OpAtomic$(item.classType)Add$(item.suffix) $$$(item.name) result &mem Device UniformMemory $data
};
}
return atomicAddWithOrder(mem, data, MemoryOrder::Relaxed);
}

__glsl_version(430)
[ForceInline]
[require(glsl_spirv, atomic_glsl)]
__intrinsic_op($(kIROp_AtomicMin))
$(item.name) atomicMinWithOrder(inout $(item.name) mem, $(item.name) data, MemoryOrder order);

__glsl_version(430)
[ForceInline]
Expand All @@ -8493,17 +8496,14 @@ public $(item.name) atomicMin(inout $(item.name) mem, $(item.name) data)
{
typeRequireChecks_atomic_using_float2_tier<$(item.name)>();
typeRequireChecks_atomic_using_MinMax<$(item.name)>();
__target_switch
{
case glsl: __intrinsic_asm "atomicMin($0, $1)";
case spirv:
return spirv_asm
{
OpAtomic$(item.subclassType)Min$(item.suffix) $$$(item.name) result &mem Device UniformMemory $data
};
}
return atomicMinWithOrder(mem, data, MemoryOrder::Relaxed);
}

__glsl_version(430)
[ForceInline]
[require(glsl_spirv, atomic_glsl)]
__intrinsic_op($(kIROp_AtomicMax))
$(item.name) atomicMaxWithOrder(inout $(item.name) mem, $(item.name) data, MemoryOrder order);

__glsl_version(430)
[ForceInline]
Expand All @@ -8512,33 +8512,22 @@ public $(item.name) atomicMax(inout $(item.name) mem, $(item.name) data)
{
typeRequireChecks_atomic_using_float2_tier<$(item.name)>();
typeRequireChecks_atomic_using_MinMax<$(item.name)>();
__target_switch
{
case glsl: __intrinsic_asm "atomicMax($0, $1)";
case spirv:
return spirv_asm
{
OpAtomic$(item.subclassType)Max$(item.suffix) $$$(item.name) result &mem Device UniformMemory $data
};
}
return atomicMaxWithOrder(mem, data, MemoryOrder::Relaxed);
}

__glsl_version(430)
[ForceInline]
[require(glsl_spirv, atomic_glsl)]
__intrinsic_op($(kIROp_AtomicExchange))
$(item.name) atomicExchangeWithOrder(inout $(item.name) mem, $(item.name) data, MemoryOrder order);

__glsl_version(430)
[ForceInline]
[require(glsl_spirv, atomic_glsl)]
public $(item.name) atomicExchange(inout $(item.name) mem, $(item.name) data)
{
typeRequireChecks_atomic_using_float1_tier<$(item.name)>();
__target_switch
{
case glsl: __intrinsic_asm "atomicExchange($0, $1)";
case spirv:
return spirv_asm
{
OpAtomicExchange $$$(item.name) result &mem Device UniformMemory $data
};
}
return atomicExchangeWithOrder(mem, data, MemoryOrder::Relaxed);
}

${{{{
Expand All @@ -8547,27 +8536,27 @@ if(item.isFloat)
}}}}


__glsl_version(430)
[ForceInline]
[require(glsl_spirv, atomic_glsl)]
__intrinsic_op($(kIROp_AtomicAnd))
$(item.name) atomicAndWithOrder(inout $(item.name) mem, $(item.name) data, MemoryOrder order);

__glsl_version(430)
[ForceInline]
[require(glsl_spirv, atomic_glsl)]
public $(item.name) atomicAnd(inout $(item.name) mem, $(item.name) data)
{
typeRequireChecks_atomic_using_float0_tier<$(item.name)>();
typeRequireChecks_atomic_using_Logical_CAS<$(item.name)>();
__target_switch
{
case glsl:
{
__intrinsic_asm "atomicAnd($0, $1)";
}
case spirv:
return spirv_asm
{
OpAtomicAnd $$$(item.name) result &mem Device UniformMemory $data
};
}
return atomicAndWithOrder(mem, data, MemoryOrder::Relaxed);
}

__glsl_version(430)
[ForceInline]
[require(glsl_spirv, atomic_glsl)]
__intrinsic_op($(kIROp_AtomicOr))
$(item.name) atomicOrWithOrder(inout $(item.name) mem, $(item.name) data, MemoryOrder order);

__glsl_version(430)
[ForceInline]
Expand All @@ -8576,36 +8565,31 @@ public $(item.name) atomicOr(inout $(item.name) mem, $(item.name) data)
{
typeRequireChecks_atomic_using_float0_tier<$(item.name)>();
typeRequireChecks_atomic_using_Logical_CAS<$(item.name)>();
__target_switch
{
case glsl: __intrinsic_asm "atomicOr($0, $1)";
case spirv:
return spirv_asm
{
OpAtomicOr $$$(item.name) result &mem Device UniformMemory $data
};
}
return atomicOrWithOrder(mem, data, MemoryOrder::Relaxed);
}


__glsl_version(430)
[ForceInline]
[require(glsl_spirv, atomic_glsl)]
__intrinsic_op($(kIROp_AtomicXor))
$(item.name) atomicXorWithOrder(inout $(item.name) mem, $(item.name) data, MemoryOrder order);

__glsl_version(430)
[ForceInline]
[require(glsl_spirv, atomic_glsl)]
public $(item.name) atomicXor(inout $(item.name) mem, $(item.name) data)
{
typeRequireChecks_atomic_using_float0_tier<$(item.name)>();
typeRequireChecks_atomic_using_Logical_CAS<$(item.name)>();
__target_switch
{
case glsl: __intrinsic_asm "atomicXor($0, $1)";
case spirv:
return spirv_asm
{
OpAtomicXor $$$(item.name) result &mem Device UniformMemory $data
};
}
return atomicXorWithOrder(mem, data, MemoryOrder::Relaxed);
}

__glsl_version(430)
[ForceInline]
[require(glsl_spirv, atomic_glsl)]
__intrinsic_op($(kIROp_AtomicCompareExchange))
$(item.name) atomicCompSwapWithOrder(inout $(item.name) mem, $(item.name) compare, $(item.name) data, MemoryOrder successOrder, MemoryOrder failOrder);

__glsl_version(430)
[ForceInline]
Expand All @@ -8614,15 +8598,7 @@ public $(item.name) atomicCompSwap(inout $(item.name) mem, $(item.name) compare,
{
typeRequireChecks_atomic_using_float0_tier<$(item.name)>();
typeRequireChecks_atomic_using_Logical_CAS<$(item.name)>();
__target_switch
{
case glsl: __intrinsic_asm "atomicCompSwap($0, $1, $2)";
case spirv:
return spirv_asm
{
result:$$$(item.name) = OpAtomicCompareExchange &mem Device None None $data $compare
};
}
return atomicCompSwapWithOrder(mem, compare, data, MemoryOrder::Relaxed, MemoryOrder::Relaxed);
}

${{{{
Expand Down
7 changes: 7 additions & 0 deletions source/slang/slang-diagnostic-defs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2255,6 +2255,13 @@ DIAGNOSTIC(
multiSampledTextureDoesNotAllowWrites,
"cannot write to a multisampled texture with target '$0'.")

DIAGNOSTIC(
41403,
Error,
invalidAtomicDestinationPointer,
"cannot perform atomic operation because destination is neither groupshared nor from a device "
"buffer.")

//
// 5xxxx - Target code generation.
//
Expand Down
8 changes: 8 additions & 0 deletions source/slang/slang-emit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1320,6 +1320,14 @@ Result linkAndOptimizeIR(
byteAddressBufferOptions);
}

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

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@aleino-nv aleino-nv Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

// specialization
if (target != CodeGenTarget::SPIRV && target != CodeGenTarget::SPIRVAssembly)
{
bool skipFuncParamValidation = true;
validateAtomicOperations(skipFuncParamValidation, sink, irModule->getModuleInst());
}

// For CUDA targets only, we will need to turn operations
// the implicitly reference the "active mask" into ones
// that use (and pass around) an explicit mask instead.
Expand Down
7 changes: 7 additions & 0 deletions source/slang/slang-ir-insts.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2502,6 +2502,13 @@ struct IRGetElementPtr : IRInst
IRInst* getIndex() { return getOperand(1); }
};

struct IRGetOffsetPtr : IRInst
{
IR_LEAF_ISA(GetOffsetPtr);
IRInst* getBase() { return getOperand(0); }
IRInst* getOffset() { return getOperand(1); }
};

struct IRRWStructuredBufferGetElementPtr : IRInst
{
IR_LEAF_ISA(RWStructuredBufferGetElementPtr);
Expand Down
10 changes: 10 additions & 0 deletions source/slang/slang-ir-specialize-address-space.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ struct AddressSpaceContext : public AddressSpaceSpecializationContext

Dictionary<IRInst*, AddressSpace> mapInstToAddrSpace;
InitialAddressSpaceAssigner* addrSpaceAssigner;
HashSet<IRFunc*> functionsToConsiderRemoving;

AddressSpaceContext(IRModule* inModule, InitialAddressSpaceAssigner* inAddrSpaceAssigner)
: module(inModule), addrSpaceAssigner(inAddrSpaceAssigner)
Expand Down Expand Up @@ -279,6 +280,8 @@ struct AddressSpaceContext : public AddressSpaceSpecializationContext
callInst = as<IRCall>(builder.replaceOperand(
callInst->getOperands(),
specializedCallee));
// At this point, the original callee may be left without uses.
functionsToConsiderRemoving.add(callee);
}
auto callResultAddrSpace =
getFuncResultAddrSpace(specializedCallee);
Expand Down Expand Up @@ -394,6 +397,13 @@ struct AddressSpaceContext : public AddressSpaceSpecializationContext
}

applyAddressSpaceToInstType();

for (IRFunc* func : functionsToConsiderRemoving)
{
SLANG_ASSERT(!func->findDecoration<IREntryPointDecoration>());
if (!func->hasUses())
func->removeAndDeallocate();
}
}
};

Expand Down
6 changes: 6 additions & 0 deletions source/slang/slang-ir-spirv-legalize.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "slang-ir-simplify-cfg.h"
#include "slang-ir-specialize-address-space.h"
#include "slang-ir-util.h"
#include "slang-ir-validate.h"
#include "slang-ir.h"
#include "slang-legalize-types.h"

Expand Down Expand Up @@ -1931,6 +1932,11 @@ struct SPIRVLegalizationContext : public SourceEmitterBase
// Specalize address space for all pointers.
SpirvAddressSpaceAssigner addressSpaceAssigner;
specializeAddressSpace(m_module, &addressSpaceAssigner);

// For SPIR-V, we don't skip this validation, because we might then be generating invalid
// SPIR-V.
bool skipFuncParamValidation = false;
validateAtomicOperations(skipFuncParamValidation, m_sink, m_module->getModuleInst());
}

void updateFunctionTypes()
Expand Down
Loading
Loading