Skip to content

Commit 04353fb

Browse files
aleino-nvslangbotcsyonghe
authored
Add validation for destination of atomic operations (#6093)
* Reimplement the GLSL atomic* functions in terms of __intrinsic_op Many of these functions map directly to atomic IR instructions. The functions taking atomic_uint are left as they are. This helps to address #5989, since the destination pointer type validation can then be written only for the atomic IR instructions. * Add validation for atomic operations 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 #5989. * Add tests for GLSL atomics destination validation 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. * Validate atomic operations after address space specialization 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 * Clean up callees left without uses during address space specialziation * format code --------- Co-authored-by: slangbot <186143334+slangbot@users.noreply.github.com> Co-authored-by: Yong He <yonghe@outlook.com>
1 parent 18f12ad commit 04353fb

9 files changed

+225
-75
lines changed

‎source/slang/glsl.meta.slang

+51-75
Original file line numberDiff line numberDiff line change
@@ -8467,24 +8467,27 @@ for (const auto& item : atomics)
84678467
}}}}
84688468

84698469

8470+
__glsl_version(430)
8471+
[ForceInline]
8472+
[require(glsl_spirv, atomic_glsl)]
8473+
__intrinsic_op($(kIROp_AtomicAdd))
8474+
$(item.name) atomicAddWithOrder(inout $(item.name) mem, $(item.name) data, MemoryOrder order);
8475+
84708476
__glsl_version(430)
84718477
[ForceInline]
84728478
[require(glsl_spirv, atomic_glsl)]
84738479
public $(item.name) atomicAdd(inout $(item.name) mem, $(item.name) data)
84748480
{
84758481
typeRequireChecks_atomic_using_float1_tier<$(item.name)>();
84768482
typeRequireChecks_atomic_using_add<$(item.name)>();
8477-
__target_switch
8478-
{
8479-
case glsl: __intrinsic_asm "atomicAdd($0, $1)";
8480-
case spirv:
8481-
return spirv_asm
8482-
{
8483-
OpAtomic$(item.classType)Add$(item.suffix) $$$(item.name) result &mem Device UniformMemory $data
8484-
};
8485-
}
8483+
return atomicAddWithOrder(mem, data, MemoryOrder::Relaxed);
84868484
}
84878485

8486+
__glsl_version(430)
8487+
[ForceInline]
8488+
[require(glsl_spirv, atomic_glsl)]
8489+
__intrinsic_op($(kIROp_AtomicMin))
8490+
$(item.name) atomicMinWithOrder(inout $(item.name) mem, $(item.name) data, MemoryOrder order);
84888491

84898492
__glsl_version(430)
84908493
[ForceInline]
@@ -8493,17 +8496,14 @@ public $(item.name) atomicMin(inout $(item.name) mem, $(item.name) data)
84938496
{
84948497
typeRequireChecks_atomic_using_float2_tier<$(item.name)>();
84958498
typeRequireChecks_atomic_using_MinMax<$(item.name)>();
8496-
__target_switch
8497-
{
8498-
case glsl: __intrinsic_asm "atomicMin($0, $1)";
8499-
case spirv:
8500-
return spirv_asm
8501-
{
8502-
OpAtomic$(item.subclassType)Min$(item.suffix) $$$(item.name) result &mem Device UniformMemory $data
8503-
};
8504-
}
8499+
return atomicMinWithOrder(mem, data, MemoryOrder::Relaxed);
85058500
}
85068501

8502+
__glsl_version(430)
8503+
[ForceInline]
8504+
[require(glsl_spirv, atomic_glsl)]
8505+
__intrinsic_op($(kIROp_AtomicMax))
8506+
$(item.name) atomicMaxWithOrder(inout $(item.name) mem, $(item.name) data, MemoryOrder order);
85078507

85088508
__glsl_version(430)
85098509
[ForceInline]
@@ -8512,33 +8512,22 @@ public $(item.name) atomicMax(inout $(item.name) mem, $(item.name) data)
85128512
{
85138513
typeRequireChecks_atomic_using_float2_tier<$(item.name)>();
85148514
typeRequireChecks_atomic_using_MinMax<$(item.name)>();
8515-
__target_switch
8516-
{
8517-
case glsl: __intrinsic_asm "atomicMax($0, $1)";
8518-
case spirv:
8519-
return spirv_asm
8520-
{
8521-
OpAtomic$(item.subclassType)Max$(item.suffix) $$$(item.name) result &mem Device UniformMemory $data
8522-
};
8523-
}
8515+
return atomicMaxWithOrder(mem, data, MemoryOrder::Relaxed);
85248516
}
85258517

8518+
__glsl_version(430)
8519+
[ForceInline]
8520+
[require(glsl_spirv, atomic_glsl)]
8521+
__intrinsic_op($(kIROp_AtomicExchange))
8522+
$(item.name) atomicExchangeWithOrder(inout $(item.name) mem, $(item.name) data, MemoryOrder order);
85268523

85278524
__glsl_version(430)
85288525
[ForceInline]
85298526
[require(glsl_spirv, atomic_glsl)]
85308527
public $(item.name) atomicExchange(inout $(item.name) mem, $(item.name) data)
85318528
{
85328529
typeRequireChecks_atomic_using_float1_tier<$(item.name)>();
8533-
__target_switch
8534-
{
8535-
case glsl: __intrinsic_asm "atomicExchange($0, $1)";
8536-
case spirv:
8537-
return spirv_asm
8538-
{
8539-
OpAtomicExchange $$$(item.name) result &mem Device UniformMemory $data
8540-
};
8541-
}
8530+
return atomicExchangeWithOrder(mem, data, MemoryOrder::Relaxed);
85428531
}
85438532

85448533
${{{{
@@ -8547,27 +8536,27 @@ if(item.isFloat)
85478536
}}}}
85488537

85498538

8539+
__glsl_version(430)
8540+
[ForceInline]
8541+
[require(glsl_spirv, atomic_glsl)]
8542+
__intrinsic_op($(kIROp_AtomicAnd))
8543+
$(item.name) atomicAndWithOrder(inout $(item.name) mem, $(item.name) data, MemoryOrder order);
8544+
85508545
__glsl_version(430)
85518546
[ForceInline]
85528547
[require(glsl_spirv, atomic_glsl)]
85538548
public $(item.name) atomicAnd(inout $(item.name) mem, $(item.name) data)
85548549
{
85558550
typeRequireChecks_atomic_using_float0_tier<$(item.name)>();
85568551
typeRequireChecks_atomic_using_Logical_CAS<$(item.name)>();
8557-
__target_switch
8558-
{
8559-
case glsl:
8560-
{
8561-
__intrinsic_asm "atomicAnd($0, $1)";
8562-
}
8563-
case spirv:
8564-
return spirv_asm
8565-
{
8566-
OpAtomicAnd $$$(item.name) result &mem Device UniformMemory $data
8567-
};
8568-
}
8552+
return atomicAndWithOrder(mem, data, MemoryOrder::Relaxed);
85698553
}
85708554

8555+
__glsl_version(430)
8556+
[ForceInline]
8557+
[require(glsl_spirv, atomic_glsl)]
8558+
__intrinsic_op($(kIROp_AtomicOr))
8559+
$(item.name) atomicOrWithOrder(inout $(item.name) mem, $(item.name) data, MemoryOrder order);
85718560

85728561
__glsl_version(430)
85738562
[ForceInline]
@@ -8576,36 +8565,31 @@ public $(item.name) atomicOr(inout $(item.name) mem, $(item.name) data)
85768565
{
85778566
typeRequireChecks_atomic_using_float0_tier<$(item.name)>();
85788567
typeRequireChecks_atomic_using_Logical_CAS<$(item.name)>();
8579-
__target_switch
8580-
{
8581-
case glsl: __intrinsic_asm "atomicOr($0, $1)";
8582-
case spirv:
8583-
return spirv_asm
8584-
{
8585-
OpAtomicOr $$$(item.name) result &mem Device UniformMemory $data
8586-
};
8587-
}
8568+
return atomicOrWithOrder(mem, data, MemoryOrder::Relaxed);
85888569
}
85898570

85908571

8572+
__glsl_version(430)
8573+
[ForceInline]
8574+
[require(glsl_spirv, atomic_glsl)]
8575+
__intrinsic_op($(kIROp_AtomicXor))
8576+
$(item.name) atomicXorWithOrder(inout $(item.name) mem, $(item.name) data, MemoryOrder order);
8577+
85918578
__glsl_version(430)
85928579
[ForceInline]
85938580
[require(glsl_spirv, atomic_glsl)]
85948581
public $(item.name) atomicXor(inout $(item.name) mem, $(item.name) data)
85958582
{
85968583
typeRequireChecks_atomic_using_float0_tier<$(item.name)>();
85978584
typeRequireChecks_atomic_using_Logical_CAS<$(item.name)>();
8598-
__target_switch
8599-
{
8600-
case glsl: __intrinsic_asm "atomicXor($0, $1)";
8601-
case spirv:
8602-
return spirv_asm
8603-
{
8604-
OpAtomicXor $$$(item.name) result &mem Device UniformMemory $data
8605-
};
8606-
}
8585+
return atomicXorWithOrder(mem, data, MemoryOrder::Relaxed);
86078586
}
86088587

8588+
__glsl_version(430)
8589+
[ForceInline]
8590+
[require(glsl_spirv, atomic_glsl)]
8591+
__intrinsic_op($(kIROp_AtomicCompareExchange))
8592+
$(item.name) atomicCompSwapWithOrder(inout $(item.name) mem, $(item.name) compare, $(item.name) data, MemoryOrder successOrder, MemoryOrder failOrder);
86098593

86108594
__glsl_version(430)
86118595
[ForceInline]
@@ -8614,15 +8598,7 @@ public $(item.name) atomicCompSwap(inout $(item.name) mem, $(item.name) compare,
86148598
{
86158599
typeRequireChecks_atomic_using_float0_tier<$(item.name)>();
86168600
typeRequireChecks_atomic_using_Logical_CAS<$(item.name)>();
8617-
__target_switch
8618-
{
8619-
case glsl: __intrinsic_asm "atomicCompSwap($0, $1, $2)";
8620-
case spirv:
8621-
return spirv_asm
8622-
{
8623-
result:$$$(item.name) = OpAtomicCompareExchange &mem Device None None $data $compare
8624-
};
8625-
}
8601+
return atomicCompSwapWithOrder(mem, compare, data, MemoryOrder::Relaxed, MemoryOrder::Relaxed);
86268602
}
86278603

86288604
${{{{

‎source/slang/slang-diagnostic-defs.h

+7
Original file line numberDiff line numberDiff line change
@@ -2255,6 +2255,13 @@ DIAGNOSTIC(
22552255
multiSampledTextureDoesNotAllowWrites,
22562256
"cannot write to a multisampled texture with target '$0'.")
22572257

2258+
DIAGNOSTIC(
2259+
41403,
2260+
Error,
2261+
invalidAtomicDestinationPointer,
2262+
"cannot perform atomic operation because destination is neither groupshared nor from a device "
2263+
"buffer.")
2264+
22582265
//
22592266
// 5xxxx - Target code generation.
22602267
//

‎source/slang/slang-emit.cpp

+8
Original file line numberDiff line numberDiff line change
@@ -1320,6 +1320,14 @@ Result linkAndOptimizeIR(
13201320
byteAddressBufferOptions);
13211321
}
13221322

1323+
// For SPIR-V, this function is called elsewhere, so that it can happen after address space
1324+
// specialization
1325+
if (target != CodeGenTarget::SPIRV && target != CodeGenTarget::SPIRVAssembly)
1326+
{
1327+
bool skipFuncParamValidation = true;
1328+
validateAtomicOperations(skipFuncParamValidation, sink, irModule->getModuleInst());
1329+
}
1330+
13231331
// For CUDA targets only, we will need to turn operations
13241332
// the implicitly reference the "active mask" into ones
13251333
// that use (and pass around) an explicit mask instead.

‎source/slang/slang-ir-insts.h

+7
Original file line numberDiff line numberDiff line change
@@ -2502,6 +2502,13 @@ struct IRGetElementPtr : IRInst
25022502
IRInst* getIndex() { return getOperand(1); }
25032503
};
25042504

2505+
struct IRGetOffsetPtr : IRInst
2506+
{
2507+
IR_LEAF_ISA(GetOffsetPtr);
2508+
IRInst* getBase() { return getOperand(0); }
2509+
IRInst* getOffset() { return getOperand(1); }
2510+
};
2511+
25052512
struct IRRWStructuredBufferGetElementPtr : IRInst
25062513
{
25072514
IR_LEAF_ISA(RWStructuredBufferGetElementPtr);

‎source/slang/slang-ir-specialize-address-space.cpp

+10
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ struct AddressSpaceContext : public AddressSpaceSpecializationContext
1313

1414
Dictionary<IRInst*, AddressSpace> mapInstToAddrSpace;
1515
InitialAddressSpaceAssigner* addrSpaceAssigner;
16+
HashSet<IRFunc*> functionsToConsiderRemoving;
1617

1718
AddressSpaceContext(IRModule* inModule, InitialAddressSpaceAssigner* inAddrSpaceAssigner)
1819
: module(inModule), addrSpaceAssigner(inAddrSpaceAssigner)
@@ -279,6 +280,8 @@ struct AddressSpaceContext : public AddressSpaceSpecializationContext
279280
callInst = as<IRCall>(builder.replaceOperand(
280281
callInst->getOperands(),
281282
specializedCallee));
283+
// At this point, the original callee may be left without uses.
284+
functionsToConsiderRemoving.add(callee);
282285
}
283286
auto callResultAddrSpace =
284287
getFuncResultAddrSpace(specializedCallee);
@@ -394,6 +397,13 @@ struct AddressSpaceContext : public AddressSpaceSpecializationContext
394397
}
395398

396399
applyAddressSpaceToInstType();
400+
401+
for (IRFunc* func : functionsToConsiderRemoving)
402+
{
403+
SLANG_ASSERT(!func->findDecoration<IREntryPointDecoration>());
404+
if (!func->hasUses())
405+
func->removeAndDeallocate();
406+
}
397407
}
398408
};
399409

‎source/slang/slang-ir-spirv-legalize.cpp

+6
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
#include "slang-ir-simplify-cfg.h"
2323
#include "slang-ir-specialize-address-space.h"
2424
#include "slang-ir-util.h"
25+
#include "slang-ir-validate.h"
2526
#include "slang-ir.h"
2627
#include "slang-legalize-types.h"
2728

@@ -1931,6 +1932,11 @@ struct SPIRVLegalizationContext : public SourceEmitterBase
19311932
// Specalize address space for all pointers.
19321933
SpirvAddressSpaceAssigner addressSpaceAssigner;
19331934
specializeAddressSpace(m_module, &addressSpaceAssigner);
1935+
1936+
// For SPIR-V, we don't skip this validation, because we might then be generating invalid
1937+
// SPIR-V.
1938+
bool skipFuncParamValidation = false;
1939+
validateAtomicOperations(skipFuncParamValidation, m_sink, m_module->getModuleInst());
19341940
}
19351941

19361942
void updateFunctionTypes()

0 commit comments

Comments
 (0)