Fix translation of RWTexture subscript operations for Vulkan (#618)

Partially fixes #615

There's kind of a mess going on here, and it is difficult to be sure which of the changes here are strictly necessary.
Also, our testing isn't setup to run tests that use `RWTexture2D`, so the only testing I can really run is manual tests using Falcor.

The most basic issue here is that in an earlier change I added `ref` accessors for the subscript operation on various `RW*` types in the standard library, and that included `RWTexture2D` (and the other `RWTexture*` types). The compiler ended up favoring a `ref` accessor over a `set` accessor even when the `set` would suffice, but only the `set` accessor could be lowerd to GLSL/SPIR-V.

This change ends up implementing two different fixes for the same problem:

* Logic has been added to try and favor a `set` accessor over a `ref` accessor in the cases where either could be used (but still require a `ref` accessor to be used when it is really needed)

* The `ref` accessor for `RWTexture*` has been removed, since it turns out that the operations that might have benefited from it (atomics, and component-granularity stores) aren't actually allowed on typed UAVs anyway.

There is a deeper issue here that somebody needs to go through and rationalize our representation and handling of accessors like this, but I'm not going to be able to do that in the time I can put into this PR.

4 files changed, 111 insertions, 29 deletions

diff --git a/source/slang/core.meta.slang b/source/slang/core.meta.slang
index bd163b165..836af1a87 100644
--- a/source/slang/core.meta.slang
+++ b/source/slang/core.meta.slang
@@ -704,18 +704,34 @@ for (int tt = 0; tt < kBaseTextureTypeCount; ++tt)
                 // subscript operator
                 sb << "__subscript(" << uintN << " location) -> T {\n";
 
-                sb << "__target_intrinsic(glsl, \"texelFetch($P, " << ivecN << "($1)";
-
-                if( !isMultisample )
-                {
-                    sb << ", 0";
-                }
-                else
+                // GLSL/SPIR-V distinguished sampled vs. non-sampled images
+                switch( access )
                 {
-                    // TODO: how to handle this...
-                    sb << ", 0";
+                case SLANG_RESOURCE_ACCESS_NONE:
+                case SLANG_RESOURCE_ACCESS_READ:
+                    sb << "__target_intrinsic(glsl, \"texelFetch($P, " << ivecN << "($1)";
+                    if( !isMultisample )
+                    {
+                        sb << ", 0";
+                    }
+                    else
+                    {
+                        // TODO: how to handle passing through sample index?
+                        sb << ", 0";
+                    }
+                    break;
+
+                default:
+                    sb << "__target_intrinsic(glsl, \"imageLoad($0, " << ivecN << "($1)";
+                    if( isMultisample )
+                    {
+                        // TODO: how to handle passing through sample index?
+                        sb << ", 0";
+                    }
+                    break;
                 }
 
+
                 sb << ")$z\") get;\n";
 
                 // Depending on the access level of the texture type,
@@ -730,10 +746,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.
+                    // Note: HLSL doesn't support component-granularity access into typed UAVs,
+                    // and also doesn't support atomic operations on them. As such, there should
+                    // be no reason why a `ref` accessor is required here.
                     //
-                    sb << "ref;\n";
+                    // sb << "ref;\n";
                     break;
                 }
 
diff --git a/source/slang/core.meta.slang.h b/source/slang/core.meta.slang.h
index d3edba93d..d6c742321 100644
--- a/source/slang/core.meta.slang.h
+++ b/source/slang/core.meta.slang.h
@@ -719,18 +719,34 @@ for (int tt = 0; tt < kBaseTextureTypeCount; ++tt)
                 // subscript operator
                 sb << "__subscript(" << uintN << " location) -> T {\n";
 
-                sb << "__target_intrinsic(glsl, \"texelFetch($P, " << ivecN << "($1)";
-
-                if( !isMultisample )
-                {
-                    sb << ", 0";
-                }
-                else
+                // GLSL/SPIR-V distinguished sampled vs. non-sampled images
+                switch( access )
                 {
-                    // TODO: how to handle this...
-                    sb << ", 0";
+                case SLANG_RESOURCE_ACCESS_NONE:
+                case SLANG_RESOURCE_ACCESS_READ:
+                    sb << "__target_intrinsic(glsl, \"texelFetch($P, " << ivecN << "($1)";
+                    if( !isMultisample )
+                    {
+                        sb << ", 0";
+                    }
+                    else
+                    {
+                        // TODO: how to handle passing through sample index?
+                        sb << ", 0";
+                    }
+                    break;
+
+                default:
+                    sb << "__target_intrinsic(glsl, \"imageLoad($0, " << ivecN << "($1)";
+                    if( isMultisample )
+                    {
+                        // TODO: how to handle passing through sample index?
+                        sb << ", 0";
+                    }
+                    break;
                 }
 
+
                 sb << ")$z\") get;\n";
 
                 // Depending on the access level of the texture type,
@@ -745,10 +761,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.
+                    // Note: HLSL doesn't support component-granularity access into typed UAVs,
+                    // and also doesn't support atomic operations on them. As such, there should
+                    // be no reason why a `ref` accessor is required here.
                     //
-                    sb << "ref;\n";
+                    // sb << "ref;\n";
                     break;
                 }
 
diff --git a/source/slang/lower-to-ir.cpp b/source/slang/lower-to-ir.cpp
index 07cb508b8..280b89fb8 100644
--- a/source/slang/lower-to-ir.cpp
+++ b/source/slang/lower-to-ir.cpp
@@ -2990,10 +2990,39 @@ static LoweredValInfo moveIntoMutableTemp(
     return var;
 }
 
+// When we try to turn a `LoweredValInfo` into an address of some temporary storage,
+// we can either do it "aggressively" or not (what we'll call the "default" behavior,
+// although it isn't strictly more common).
+//
+// The case that this is mostly there to address is when somebody writes an operation
+// like:
+//
+//      foo[a] = b;
+//
+// In that case, we might as well just use the `set` accessor if there is one, rather
+// than complicate things. However, in more complex cases like:
+//
+//      foo[a].x = b;
+//
+// there is no way to satisfy the semantics of the code the user wrote (in terms of
+// only writing one vector component, and not a full vector) by using the `set`
+// accessor, and we need to be "aggressive" in turning the lvalue `foo[a]` into
+// an address.
+//
+// TODO: realistically IR lowering is too early to be binding to this choice,
+// because different accessors might be supported on different targets.
+//
+enum class TryGetAddressMode
+{
+    Default,
+    Aggressive,
+};
+
 /// Try to coerce `inVal` into a `LoweredValInfo::ptr()` with a simple address.
 LoweredValInfo tryGetAddress(
     IRGenContext*           context,
-    LoweredValInfo const&   inVal)
+    LoweredValInfo const&   inVal,
+    TryGetAddressMode       mode)
 {
     LoweredValInfo val = inVal;
 
@@ -3011,6 +3040,19 @@ LoweredValInfo tryGetAddress(
             // as part of the subscript operation being referenced.
             //
             auto subscriptInfo = val.getBoundSubscriptInfo();
+
+            // We don't want to immediately bind to a `ref` accessor if there is
+            // a `set` accessor available, unless we are in an "aggressive" mode
+            // where we really want/need a pointer to be able to make progress.
+            //
+            if(mode != TryGetAddressMode::Aggressive
+                && getMembersOfType<SetterDecl>(subscriptInfo->declRef).Count())
+            {
+                // There is a setter that we should consider using,
+                // so don't go and aggressively collapse things just yet.
+                return val;
+            }
+
             auto refAccessors = getMembersOfType<RefAccessorDecl>(subscriptInfo->declRef);
             if(refAccessors.Count())
             {
@@ -3053,7 +3095,7 @@ LoweredValInfo tryGetAddress(
             if( auto fieldDeclRef = declRef.As<StructField>() )
             {
                 auto baseVal = boundMemberInfo->base;
-                auto basePtr = tryGetAddress(context, baseVal);
+                auto basePtr = tryGetAddress(context, baseVal, TryGetAddressMode::Aggressive);
 
                 return extractField(context, boundMemberInfo->type, basePtr, fieldDeclRef);
             }
@@ -3068,7 +3110,7 @@ LoweredValInfo tryGetAddress(
 
             UInt elementCount = originalSwizzleInfo->elementCount;
 
-            auto newBase = tryGetAddress(context, originalBase);
+            auto newBase = tryGetAddress(context, originalBase, TryGetAddressMode::Aggressive);
             RefPtr<SwizzledLValueInfo> newSwizzleInfo = new SwizzledLValueInfo();
             context->shared->extValues.Add(newSwizzleInfo);
 
@@ -3098,7 +3140,7 @@ IRInst* getAddress(
     LoweredValInfo const&   inVal,
     SourceLoc               diagnosticLocation)
 {
-    LoweredValInfo val = tryGetAddress(context, inVal);
+    LoweredValInfo val = tryGetAddress(context, inVal, TryGetAddressMode::Aggressive);
 
     if( val.flavor == LoweredValInfo::Flavor::Ptr )
     {
@@ -3122,7 +3164,7 @@ void assign(
     // a simple pointer, since that would make our life a lot easier
     // when handling complex cases.
     //
-    left = tryGetAddress(context, left);
+    left = tryGetAddress(context, left, TryGetAddressMode::Default);
 
     auto builder = context->irBuilder;
 
diff --git a/source/slang/mangle.cpp b/source/slang/mangle.cpp
index d3e2f833f..dc2738d8f 100644
--- a/source/slang/mangle.cpp
+++ b/source/slang/mangle.cpp
@@ -325,6 +325,12 @@ namespace Slang
 
         emitName(context, declRef.GetName());
 
+        // Special case: accessors need some way to distinguish themselves
+        // so that a getter/setter/ref-er don't all compile to the same name.
+        if(declRef.As<GetterDecl>())        emitRaw(context, "Ag");
+        if(declRef.As<SetterDecl>())        emitRaw(context, "As");
+        if(declRef.As<RefAccessorDecl>())   emitRaw(context, "Ar");
+
         // Are we the "inner" declaration beneath a generic decl?
         if(parentGenericDeclRef && (parentGenericDeclRef.getDecl()->inner.Ptr() == declRef.getDecl()))
         {