Fix atomic operations on RWBuffer (shader-slang#593)

* Fix atomic operations on RWBuffer

An earlier change added support for passing true pointers to `__ref` parameters to fix the global `Interlocked*()` functions when applied to `groupshared` variables or `RWStructureBuffer<T>` elements.
That change didn't apply to `RWBuffer<T>` or `RWTexture2D<T>`, etc. because those types had so far only declared `get` and `set` accessors, but not any `ref` accessors (which return a pointer).

The main fixes here are:

* Add `ref` accessors to the subscript oeprations on the `RW*` resource types

* Adjust the logic for emitting calls to subscript accessors so that we don't get quite as eager about invoking a `ref` accessor, and instead try to invoke just a `get` or `set` accessor when these will suffice. This is important for Vulkan cross-compilation, where we don't yet support the semantics of our `ref` accessors.

* Add a test case for atomics on a `RWBuffer`

* Fix up `render-test` so that we can specify a format for a buffer resource, which allows us to use things other than `*StructuredBuffer` and `*ByteAddressBuffer`. The work there is probably not complete; I just did what I could to get the test working.

* A bunch of files got whitespace edits thanks to the fact that I'm using editorconfig and others on the project seemingly arent...

* fixup: remove ifdefed-out code

Author: Tim Foley

source/slang/core.meta.slang

@@ -326,7 +326,7 @@ for( int C = 2; C <= 4; ++C )
 sb << "__magic_type(SamplerState," << int(SamplerStateFlavor::SamplerState) << ")\n";
 sb << "__intrinsic_type(" << kIROp_SamplerStateType << ")\n";
 sb << "struct SamplerState {};";
-        
+
 sb << "__magic_type(SamplerState," << int(SamplerStateFlavor::SamplerComparisonState) << ")\n";
 sb << "__intrinsic_type(" << kIROp_SamplerComparisonStateType << ")\n";
 sb << "struct SamplerComparisonState {};";
@@ -688,6 +688,11 @@ for (int tt = 0; tt < kBaseTextureTypeCount; ++tt)
 
                 default:
                     sb << "__target_intrinsic(glsl, \"imageStore($0, " << ivecN << "($1), $2)\") set;\n";
+
+                    // TODO: really need a way to map access through `ref` accessor (e.g., when
+                    // used with an atomic operation) over to GLSL equivalent.
+                    //
+                    sb << "ref;\n";
                     break;
                 }
 
@@ -911,7 +916,7 @@ for (int tt = 0; tt < kBaseTextureTypeCount; ++tt)
                     auto componentName = kk.componentName;
 
                     EMIT_LINE_DIRECTIVE();
-                            
+
                     sb << "__target_intrinsic(glsl, \"textureGather($p, $2, " << componentIndex << ")\")\n";
                     sb << "vector<T, 4> Gather" << componentName << "(SamplerState s, ";
                     sb << "float" << kBaseTextureTypes[tt].coordCount << " location);\n";

source/slang/core.meta.slang.h

@@ -341,7 +341,7 @@ for( int C = 2; C <= 4; ++C )
 sb << "__magic_type(SamplerState," << int(SamplerStateFlavor::SamplerState) << ")\n";
 sb << "__intrinsic_type(" << kIROp_SamplerStateType << ")\n";
 sb << "struct SamplerState {};";
-        
+
 sb << "__magic_type(SamplerState," << int(SamplerStateFlavor::SamplerComparisonState) << ")\n";
 sb << "__intrinsic_type(" << kIROp_SamplerComparisonStateType << ")\n";
 sb << "struct SamplerComparisonState {};";
@@ -703,6 +703,11 @@ for (int tt = 0; tt < kBaseTextureTypeCount; ++tt)
 
                 default:
                     sb << "__target_intrinsic(glsl, \"imageStore($0, " << ivecN << "($1), $2)\") set;\n";
+
+                    // TODO: really need a way to map access through `ref` accessor (e.g., when
+                    // used with an atomic operation) over to GLSL equivalent.
+                    //
+                    sb << "ref;\n";
                     break;
                 }
 
@@ -926,7 +931,7 @@ for (int tt = 0; tt < kBaseTextureTypeCount; ++tt)
                     auto componentName = kk.componentName;
 
                     EMIT_LINE_DIRECTIVE();
-                            
+
                     sb << "__target_intrinsic(glsl, \"textureGather($p, $2, " << componentIndex << ")\")\n";
                     sb << "vector<T, 4> Gather" << componentName << "(SamplerState s, ";
                     sb << "float" << kBaseTextureTypes[tt].coordCount << " location);\n";

source/slang/emit.cpp

@@ -2363,15 +2363,18 @@ struct EmitVisitor
         // for temporary variables.
         auto type = inst->getDataType();
 
-        // First we unwrap any layers of pointer-ness and array-ness
-        // from the types to get at the underlying data type.
-        while (auto ptrType = as<IRPtrTypeBase>(type))
+        // Unwrap any layers of array-ness from the type, so that
+        // we can look at the underlying data type, in case we
+        // should *never* expose a value of that type
+        while (auto arrayType = as<IRArrayTypeBase>(type))
         {
-            type = ptrType->getValueType();
+            type = arrayType->getElementType();
         }
-        while (auto ptrType = as<IRArrayTypeBase>(type))
+
+        // Don't allow temporaries of pointer types to be created.
+        if(as<IRPtrTypeBase>(type))
         {
-            type = ptrType->getElementType();
+            return true;
         }
 
         // First we check for uniform parameter groups,

source/slang/hlsl.meta.slang

@@ -1144,7 +1144,7 @@ for (int aa = 0; aa < kBaseBufferAccessLevelCount; ++aa)
 
     if (kBaseBufferAccessLevels[aa].access != SLANG_RESOURCE_ACCESS_READ)
     {
-        sb << "set;\n";
+        sb << "ref;\n";
     }
 
     sb << "}\n";

source/slang/hlsl.meta.slang.h

@@ -1177,7 +1177,7 @@ for (int aa = 0; aa < kBaseBufferAccessLevelCount; ++aa)
 
     if (kBaseBufferAccessLevels[aa].access != SLANG_RESOURCE_ACCESS_READ)
     {
-        sb << "set;\n";
+        sb << "ref;\n";
     }
 
     sb << "}\n";

source/slang/lower-to-ir.cpp

@@ -573,33 +573,8 @@ LoweredValInfo emitCallToDeclRef(
         bool justAGetter = true;
         for (auto accessorDeclRef : getMembersOfType<AccessorDecl>(subscriptDeclRef))
         {
-            // If the subscript declares a `ref` accessor, then we can just
-            // invoke that directly to get an l-value we can use.
-            if(auto refAccessorDeclRef = accessorDeclRef.As<RefAccessorDecl>())
-            {
-                // The `ref` accessor will return a pointer to the value, so
-                // we need to reflect that in the type of our `call` instruction.
-                IRType* ptrType = context->irBuilder->getPtrType(type);
-
-                // Rather than call `emitCallToVal` here, we make a recursive call
-                // to `emitCallToDeclRef` so that it can handle things like intrinsic-op
-                // modifiers attached to the acecssor.
-                LoweredValInfo callVal = emitCallToDeclRef(
-                    context,
-                    ptrType,
-                    refAccessorDeclRef,
-                    funcType,
-                    argCount,
-                    args);
-
-                // The result from the call needs to be implicitly dereferenced,
-                // so that it can work as an l-value of the desired result type.
-                return LoweredValInfo::ptr(getSimpleVal(context, callVal));
-            }
-
-            // If we don't find a `ref` accessor, then we want to track whether
-            // this subscript has any accessors other than `get` (assuming
-            // that everything except `get` can be used for setting...).
+            // We want to track whether this subscript has any accessors other than
+            // `get` (assuming that everything except `get` can be used for setting...).
 
             if (auto foundGetterDeclRef = accessorDeclRef.As<GetterDecl>())
             {
@@ -821,6 +796,15 @@ LoweredValInfo materialize(
         {
             auto boundSubscriptInfo = lowered.getBoundSubscriptInfo();
 
+            // We are being asked to extract a value from a subscript call
+            // (e.g., `base[index]`). We will first check if the subscript
+            // declared a getter and use that if possible, and then fall
+            // back to a `ref` accessor if one is defined.
+            //
+            // (Picking the `get` over the `ref` accessor simplifies things
+            // in case the `get` operation has a natural translation for
+            // a target, while the general `ref` case does not...)
+
             auto getters = getMembersOfType<GetterDecl>(boundSubscriptInfo->declRef);
             if (getters.Count())
             {
@@ -833,6 +817,27 @@ LoweredValInfo materialize(
                 goto top;
             }
 
+            auto refAccessors = getMembersOfType<RefAccessorDecl>(boundSubscriptInfo->declRef);
+            if(refAccessors.Count())
+            {
+                // The `ref` accessor will return a pointer to the value, so
+                // we need to reflect that in the type of our `call` instruction.
+                IRType* ptrType = context->irBuilder->getPtrType(boundSubscriptInfo->type);
+
+                LoweredValInfo refVal = emitCallToDeclRef(
+                    context,
+                    ptrType,
+                    *refAccessors.begin(),
+                    nullptr,
+                    boundSubscriptInfo->args);
+
+                // The result from the call needs to be implicitly dereferenced,
+                // so that it can work as an l-value of the desired result type.
+                lowered = LoweredValInfo::ptr(getSimpleVal(context, refVal));
+
+                goto top;
+            }
+
             SLANG_UNEXPECTED("subscript had no getter");
             UNREACHABLE_RETURN(LoweredValInfo());
         }
@@ -1308,7 +1313,7 @@ static void addNameHint(
 {
     Name* name = getNameForNameHint(context, decl);
     if(!name)
-        return;    
+        return;
     context->irBuilder->addDecoration<IRNameHintDecoration>(inst)->name = name;
 }
 
@@ -2958,18 +2963,51 @@ IRInst* getAddress(
     SourceLoc               diagnosticLocation)
 {
     LoweredValInfo val = inVal;
+
     switch(val.flavor)
     {
     case LoweredValInfo::Flavor::Ptr:
         return val.val;
 
-    // TODO: are there other cases we need to handle here (e.g.,
-    // turning a bound subscript/property into an address)
+    case LoweredValInfo::Flavor::BoundSubscript:
+        {
+            // If we are are trying to turn a subscript operation like `buffer[index]`
+            // into a pointer, then we need to find a `ref` accessor declared
+            // as part of the subscript operation being referenced.
+            //
+            auto subscriptInfo = val.getBoundSubscriptInfo();
+            auto refAccessors = getMembersOfType<RefAccessorDecl>(subscriptInfo->declRef);
+            if(refAccessors.Count())
+            {
+                // The `ref` accessor will return a pointer to the value, so
+                // we need to reflect that in the type of our `call` instruction.
+                IRType* ptrType = context->irBuilder->getPtrType(subscriptInfo->type);
+
+                LoweredValInfo refVal = emitCallToDeclRef(
+                    context,
+                    ptrType,
+                    *refAccessors.begin(),
+                    nullptr,
+                    subscriptInfo->args);
+
+                // The result from the call should be a pointer, and it
+                // is the address that we wanted in the first place.
+                return getSimpleVal(context, refVal);
+            }
+
+            // Otherwise, there was no `ref` accessor, and so it is not possible
+            // to materialize this location into a pointer for whatever purpose
+            // we have in mind (e.g., passing it to an atomic operation).
+        }
+
+    // TODO: are there other cases we need to handled here?
 
     default:
-        context->getSink()->diagnose(diagnosticLocation, Diagnostics::invalidLValueForRefParameter);
-        return nullptr;
+        break;
     }
+
+    context->getSink()->diagnose(diagnosticLocation, Diagnostics::invalidLValueForRefParameter);
+    return nullptr;
 }
 
 void assign(
@@ -3090,15 +3128,17 @@ void assign(
             // `someStructuredBuffer[index]`.
             //
             // When storing to such a value, we need to emit a call
-            // to the appropriate builtin "setter" accessor.
+            // to the appropriate builtin "setter" accessor, if there
+            // is one, and then fall back to a `ref` accessor if
+            // there is no setter.
+            //
             auto subscriptInfo = left.getBoundSubscriptInfo();
 
             // Search for an appropriate "setter" declaration
             auto setters = getMembersOfType<SetterDecl>(subscriptInfo->declRef);
             if (setters.Count())
             {
                 auto allArgs = subscriptInfo->args;
-
                 addArgs(context, &allArgs, right);
 
                 emitCallToDeclRef(
@@ -3110,6 +3150,28 @@ void assign(
                 return;
             }
 
+            auto refAccessors = getMembersOfType<RefAccessorDecl>(subscriptInfo->declRef);
+            if(refAccessors.Count())
+            {
+                // The `ref` accessor will return a pointer to the value, so
+                // we need to reflect that in the type of our `call` instruction.
+                IRType* ptrType = context->irBuilder->getPtrType(subscriptInfo->type);
+
+                LoweredValInfo refVal = emitCallToDeclRef(
+                    context,
+                    ptrType,
+                    *refAccessors.begin(),
+                    nullptr,
+                    subscriptInfo->args);
+
+                // The result from the call needs to be implicitly dereferenced,
+                // so that it can work as an l-value of the desired result type.
+                left = LoweredValInfo::ptr(getSimpleVal(context, refVal));
+
+                // Tail-recursively attempt assignment again on the new l-value.
+                goto top;
+            }
+
             // No setter found? Then we have an error!
             SLANG_UNEXPECTED("no setter found");
             break;

tests/compute/atomics-buffer.slang

@@ -0,0 +1,28 @@
+// atomics-buffer.slang
+
+//TEST(compute):COMPARE_COMPUTE_EX:-slang -compute
+
+// Note: not enabling D3D12 test yet because change
+// was developed on a machine that can run D3D12
+//
+//TEST_DISABLED(compute):COMPARE_COMPUTE_EX:-slang -compute -dx12
+
+//TEST_INPUT:ubuffer(format=R_UInt32, data=[0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0]):dxbinding(0),glbinding(0),out
+
+RWBuffer<uint> outputBuffer;
+
+void test(uint val)
+{
+    uint originalValue;
+
+	InterlockedAdd(outputBuffer[val], 		val, 		originalValue);
+	InterlockedAdd(outputBuffer[val ^ 1], 	val*16, 	originalValue);
+	InterlockedAdd(outputBuffer[val ^ 2], 	val*16*16, 	originalValue);
+}
+
+[numthreads(4, 1, 1)]
+void computeMain(uint3 dispatchThreadID : SV_DispatchThreadID)
+{
+    uint tid = dispatchThreadID.x;
+    test(tid);
+}

tests/compute/atomics-buffer.slang.expected.txt

@@ -0,0 +1,4 @@
+210
+301
+32
+123

tools/render-test/d3d-util.cpp

@@ -31,6 +31,7 @@ using namespace Slang;
         case Format::RG_Float32:            return DXGI_FORMAT_R32G32_FLOAT;
         case Format::R_Float32:             return DXGI_FORMAT_R32_FLOAT;
         case Format::RGBA_Unorm_UInt8:      return DXGI_FORMAT_R8G8B8A8_UNORM;
+        case Format::R_UInt32:              return DXGI_FORMAT_R32_UINT;
 
         case Format::D_Float32:             return DXGI_FORMAT_D32_FLOAT;
         case Format::D_Unorm24_S8:          return DXGI_FORMAT_D24_UNORM_S8_UINT;
@@ -47,7 +48,8 @@ using namespace Slang;
         switch (format)
         {
             case DXGI_FORMAT_R32_FLOAT:     /* fallthru */
-            case DXGI_FORMAT_D32_FLOAT:            
+            case DXGI_FORMAT_R32_UINT:
+            case DXGI_FORMAT_D32_FLOAT:
             {
                 return DXGI_FORMAT_R32_TYPELESS;
             }
@@ -73,7 +75,7 @@ using namespace Slang;
             switch (format)
             {
                 case DXGI_FORMAT_D32_FLOAT:     /* fallthru */
-                case DXGI_FORMAT_R32_TYPELESS:            
+                case DXGI_FORMAT_R32_TYPELESS:
                 {
                     return DXGI_FORMAT_D32_FLOAT;
                 }
@@ -88,7 +90,7 @@ using namespace Slang;
             switch (format)
             {
                 case DXGI_FORMAT_D32_FLOAT:     /* fallthru */
-                case DXGI_FORMAT_D24_UNORM_S8_UINT: 
+                case DXGI_FORMAT_D24_UNORM_S8_UINT:
                 {
                     return DXGI_FORMAT_UNKNOWN;
                 }
@@ -102,7 +104,7 @@ using namespace Slang;
             switch (format)
             {
                 case DXGI_FORMAT_D32_FLOAT:     /* fallthru */
-                case DXGI_FORMAT_R32_TYPELESS:            
+                case DXGI_FORMAT_R32_TYPELESS:
                 {
                     return DXGI_FORMAT_R32_FLOAT;
                 }