Change handling of strings for HLSL/GLSL targets (#1204)

* Change handling of strings for HLSL/GLSL targets

This change switches our handling of string literals and `getStringHash` to something that is more streamlined at the cost of potentially being less general/flexible.

* `String` is now allowed as a parameter type in user-defined functions

* `getStringHash` is now allowed to apply to `String`-type values that aren't literals

* The list of strings in an IR module is now generated during IR lowering as part of lowering a string literal expression, rather than being defined by recursively walking the IR of the module looking for `getStringHash` calls. The public API still refers to these as "hashed" strings, but they are realistically now "static strings."

* When emitting code for HLSL/GLSL, the `String` type emits as `int`, and `getStringHash(x)` emits as `x`.

In terms of implementation, the choice was whether to translate `String` over to `int` in an explicit IR pass, or to lump it into the emit pass. While adding the logic to emit clutters up an already complicated step, it is ultimately much easier to make the change there than to write a clean IR pass to eliminate all `String` use.

Note that other targets that can handle a more full-featured `String` type are *not* addressed by this change and thus do not support `String` at all. It may be woth emitting `String` as `const char*` on those targets, and emitting string literals directly, but the `getStringHash` function would need to be implemented in the "prelude" then, and we probably want to pick a well-known/-documented hash algorithm before we go that far.

This change also brings along some some clean-ups to the `gpu-printing` example, since it can now take advantage of the new functionality of `String`.

* Fix up tests for new string handling

* Add global string literal list to string-literal test (since we now list *all* static string literals and not just those passed to `getStringHash`)

* Disable `getStringHash` test on CPU, since we don't have a working `String` on that platform right now (only HLSL/GLSL)

Co-authored-by: Tim Foley <tim.foley.is@gmail.com>

10 files changed, 84 insertions, 85 deletions

diff --git a/examples/gpu-printing/gpu-printing.cpp b/examples/gpu-printing/gpu-printing.cpp
index ff35fd0b3..7503c8e03 100644
--- a/examples/gpu-printing/gpu-printing.cpp
+++ b/examples/gpu-printing/gpu-printing.cpp
@@ -219,7 +219,7 @@ void GPUPrinting::processGPUPrintCommands(const void* data, size_t dataSize)
                     // likely that the application code needs to be configured
                     // to pass in the right strings.
                     //
-                    fprintf(stderr, "error: string with unknown hash %d\n", hash);
+                    fprintf(stderr, "error: string with unknown hash 0x%x\n", hash);
                     continue;
                 }
 
@@ -253,7 +253,7 @@ void GPUPrinting::processGPUPrintCommands(const void* data, size_t dataSize)
                     // likely that the application code needs to be configured
                     // to pass in the right strings.
                     //
-                    fprintf(stderr, "error: string with unknown hash %d\n", formatHash);
+                    fprintf(stderr, "error: string with unknown hash 0x%x\n", formatHash);
                     continue;
                 }
                 std::string format = iter->second;
@@ -373,7 +373,7 @@ void GPUPrinting::processGPUPrintCommands(const void* data, size_t dataSize)
                             auto iter = m_hashedStrings.find(hash);
                             if(iter == m_hashedStrings.end())
                             {
-                                fprintf(stderr, "error: string with unknown hash %d\n", hash);
+                                fprintf(stderr, "error: string with unknown hash 0x%x\n", hash);
                                 continue;
                             }
                             printf("%s", iter->second.c_str());
diff --git a/examples/gpu-printing/kernels.slang b/examples/gpu-printing/kernels.slang
index ec4533958..8693bfed1 100644
--- a/examples/gpu-printing/kernels.slang
+++ b/examples/gpu-printing/kernels.slang
@@ -29,10 +29,10 @@ void computeMain(uint3 tid : SV_DispatchThreadID)
     // in terms of their hash code, and the current Slang implementation
     // of `getStringHash` only applies to string literals.
     //
-    println(getStringHash("hello from thread number "), tid.x);
+    println("hello from thread number ", tid.x);
 
     // The second facility supported by `printing.slang` is a C-style
     // `printf()` function.
     //
-    printf(getStringHash("printf from thread 0x%x\n"), tid.x);
+    printf("printf from thread 0x%x\n", tid.x);
 }
diff --git a/examples/gpu-printing/printing.slang b/examples/gpu-printing/printing.slang
index 47d102f97..941a1518b 100644
--- a/examples/gpu-printing/printing.slang
+++ b/examples/gpu-printing/printing.slang
@@ -199,23 +199,14 @@ extension uint : IPrintable // <-- Note: we are adding a conformance to `IPrinta
     }
 }
 
-// HACK: Because we currently don't have a `String` type that we
-// can pass down into subroutines, we will be using the hash
-// code of a string to represent the string itself. These hash
-// codes currently have type `int`, so our printing library
-// will *always* assume that an `int` represents a hashed
-// string, and thus we can't print plain old `int`s right now.
-
-typedef int StringHash;
-
-extension StringHash : IPrintable
+extension String : IPrintable
 {
     uint getPrintWordCount() { return 2; }
 
     void writePrintWords(RWStructuredBuffer<uint> buffer, uint offset)
     {
         buffer[offset++] = (uint(PrintingOp.String) << 16) | 1;
-        buffer[offset++] = this;
+        buffer[offset++] = getStringHash(this);
     }
 }
 
@@ -294,7 +285,7 @@ void println<A : IPrintable, B : IPrintable, C : IPrintable>(
 // work.
 //
 
-uint _beginPrintf(int formatStrngHash, uint wordCount)
+uint _beginPrintf(String format, uint wordCount)
 {
     // A printf command will start with the usual command header word,
     // along with a word for the (hashed) format string. These
@@ -303,7 +294,7 @@ uint _beginPrintf(int formatStrngHash, uint wordCount)
     //
     uint wordOffset = _allocatePrintWords(wordCount + 2);
     gPrintBuffer[wordOffset++] = (uint(PrintingOp.PrintF) << 16) | (wordCount+1);
-    gPrintBuffer[wordOffset++] = formatStrngHash;
+    gPrintBuffer[wordOffset++] = getStringHash(format);
     return wordOffset;
 }
 
@@ -337,26 +328,26 @@ extension uint : IPrintf
     }
 }
 
-extension StringHash : IPrintf
+extension String : IPrintf
 {
     uint getPrintfWordCount() { return 1; }
 
     void writePrintfWords(RWStructuredBuffer<uint> buffer, uint offset)
     {
-        buffer[offset++] = this;
+        buffer[offset++] = getStringHash(this);
     }
 }
 
 
 // A `printf()` with no format arguments can just call back to `_beginPrintf()`
-void printf(StringHash format)
+void printf(String format)
 {
     _beginPrintf(format, 0);
 }
 
 // The `printf()` cases with one or more format arguments are all quite similar.
 
-void printf<A : IPrintf>(StringHash format, A a)
+void printf<A : IPrintf>(String format, A a)
 {
     // We need to compute the words required by each format argument
     // and sum them up.
@@ -375,7 +366,7 @@ void printf<A : IPrintf>(StringHash format, A a)
     a.writePrintfWords(gPrintBuffer, wordOffset); wordOffset += aCount;
 }
 
-void printf<A : IPrintf, B : IPrintf>(StringHash format, A a, B b)
+void printf<A : IPrintf, B : IPrintf>(String format, A a, B b)
 {
     uint wordCount = 0;
     uint aCount = a.getPrintfWordCount(); wordCount += aCount;
diff --git a/source/slang/slang-emit-glsl.cpp b/source/slang/slang-emit-glsl.cpp
index fbae21f75..1191a170c 100644
--- a/source/slang/slang-emit-glsl.cpp
+++ b/source/slang/slang-emit-glsl.cpp
@@ -1279,6 +1279,23 @@ bool GLSLSourceEmitter::tryEmitInstExprImpl(IRInst* inst, const EmitOpInfo& inOu
         // `FRem` there is no direct GLSL translation, so we will
         // leave things with the default behavior for now.
 
+        case kIROp_StringLit:
+        {
+            IRStringLit* lit = cast<IRStringLit>(inst);
+            m_writer->emit(GetHashCode(lit->getStringSlice()));
+            return true;
+        }
+        case kIROp_GetStringHash:
+        {
+            // On GLSL target, the `String` type is just an `int`
+            // that is the hash of the string, so we can emit
+            // the first operand to `getStringHash` directly.
+            //
+            EmitOpInfo outerPrec = inOuterPrec;
+            emitOperand(inst->getOperand(0), outerPrec);
+            return true;
+        }
+
         default: break;
     }
 
@@ -1480,6 +1497,7 @@ void GLSLSourceEmitter::emitSimpleTypeImpl(IRType* type)
             }
             return;
         }
+        case kIROp_StringType: m_writer->emit("int"); return;
         default: break;
     }
 
diff --git a/source/slang/slang-emit-hlsl.cpp b/source/slang/slang-emit-hlsl.cpp
index f615d41fb..4225b1bf3 100644
--- a/source/slang/slang-emit-hlsl.cpp
+++ b/source/slang/slang-emit-hlsl.cpp
@@ -458,6 +458,22 @@ bool HLSLSourceEmitter::tryEmitInstExprImpl(IRInst* inst, const EmitOpInfo& inOu
             m_writer->emit(")");
             return true;
         }
+        case kIROp_StringLit:
+        {
+            IRStringLit* lit = cast<IRStringLit>(inst);
+            m_writer->emit(GetHashCode(lit->getStringSlice()));
+            return true;
+        }
+        case kIROp_GetStringHash:
+        {
+            // On GLSL target, the `String` type is just an `int`
+            // that is the hash of the string, so we can emit
+            // the first operand to `getStringHash` directly.
+            //
+            EmitOpInfo outerPrec = inOuterPrec;
+            emitOperand(inst->getOperand(0), outerPrec);
+            return true;
+        }
         default: break;
     }
     // Not handled
@@ -584,6 +600,7 @@ void HLSLSourceEmitter::emitSimpleTypeImpl(IRType* type)
             }
             return;
         }
+        case kIROp_StringType: m_writer->emit("int"); return;
         default: break;
     }
 
diff --git a/source/slang/slang-ir-string-hash.cpp b/source/slang/slang-ir-string-hash.cpp
index 0a15ba52d..2d5d78d7e 100644
--- a/source/slang/slang-ir-string-hash.cpp
+++ b/source/slang/slang-ir-string-hash.cpp
@@ -18,40 +18,6 @@ static void _findGetStringHashRec(IRInst* inst, List<IRGetStringHash*>& outInsts
     }
 }
 
-void replaceGetStringHash(IRModule* module, SharedIRBuilder& sharedBuilder, StringSlicePool& ioPool)
-{
-    IRBuilder builder;
-    builder.sharedBuilder = &sharedBuilder;
-
-    builder.setInsertInto(module->getModuleInst());
-
-    List<IRGetStringHash*> insts;
-    _findGetStringHashRec(module->getModuleInst(), insts);
-
-    // Then we want to add the GlobalHashedString instruction in the root
-    for (auto inst : insts)
-    {
-        IRStringLit* stringLit = inst->getStringLit();
-        ioPool.add(stringLit->getStringSlice());
-        
-        // Okay work out what the hash is
-        const int hash = GetHashCode(stringLit->getStringSlice());
-
-        IRInst* intLit = builder.getIntValue(builder.getIntType(), int32_t(hash));
-
-        // Okay we want to replace all uses with the literal
-        inst->replaceUsesWith(intLit);
-        inst->removeAndDeallocate();
-    }
-}
-
-void replaceGetStringHashWithGlobal(IRModule* module, SharedIRBuilder& sharedBuilder)
-{
-    StringSlicePool pool(StringSlicePool::Style::Empty);
-    replaceGetStringHash(module, sharedBuilder, pool);
-    addGlobalHashedStringLiterals(pool, sharedBuilder);
-}
-
 void findGlobalHashedStringLiterals(IRModule* module, StringSlicePool& pool)
 {
     IRModuleInst* moduleInst = module->getModuleInst();
diff --git a/source/slang/slang-ir-string-hash.h b/source/slang/slang-ir-string-hash.h
index 13dbe12df..ccbbf4049 100644
--- a/source/slang/slang-ir-string-hash.h
+++ b/source/slang/slang-ir-string-hash.h
@@ -11,14 +11,6 @@ namespace Slang
 struct IRModule;
 struct SharedIRBuilder;
 
-// Find all of the calls to 'getStringHash' and replace them with the an int literal of the value. Place all
-// of the string literal values into the ioPool. 
-void replaceGetStringHash(IRModule* module, SharedIRBuilder& sharedBuilder, StringSlicePool& ioPool);
-
-// Does the same as replaceGetStringHash, but also adds a GlobalHashedStringLiterals instruction of any
-// string literals referenced via 'getStringHash'. If there are none, it does nothing.
-void replaceGetStringHashWithGlobal(IRModule* module, SharedIRBuilder& sharedBuilder);
-
 // Finds the global GlobalHashedStringLiterals instruction for the module if there is one, and then
 // adds all of it's strings to ioPool.
 void findGlobalHashedStringLiterals(IRModule* module, StringSlicePool& ioPool);
diff --git a/source/slang/slang-lower-to-ir.cpp b/source/slang/slang-lower-to-ir.cpp
index 14cc68b16..fdbb2cb27 100644
--- a/source/slang/slang-lower-to-ir.cpp
+++ b/source/slang/slang-lower-to-ir.cpp
@@ -343,6 +343,18 @@ struct SharedIRGenContext
     // to the appropriate basic block to jump to.
     Dictionary<Stmt*, IRBlock*> breakLabels;
     Dictionary<Stmt*, IRBlock*> continueLabels;
+
+    // List of all string literals used in user code, regardless
+    // of how they were used (i.e., whether or not they were hashed).
+    //
+    // This does *not* collect:
+    // * String literals that were only used for attributes/modifiers in
+    //   the user's code (e.g., `"compute"` in `[shader("compute")]`)
+    // * Any IR string literals constructed for the purpose of decorations,
+    //   reflection, or other meta-data that did not appear as a literal
+    //   in the source code.
+    //
+    List<IRInst*> m_stringLiterals;
 };
 
 
@@ -778,21 +790,6 @@ LoweredValInfo emitCallToDeclRef(
         }
         else
         {
-            switch (intrinsicOp)
-            {
-                case kIROp_GetStringHash:
-                {
-                    IRStringLit* stringLit = as<IRStringLit>(args[0]);
-                    if (stringLit == nullptr)
-                    {
-                        auto sink = context->getSink();
-                        sink->diagnose(funcDecl, Diagnostics::getStringHashRequiresStringLiteral);
-                        return LoweredValInfo();
-                    }
-
-                }
-            }
-
             // The intrinsic op maps to a single IR instruction,
             // so we will emit an instruction with the chosen
             // opcode, and the arguments to the call as its operands.
@@ -2515,7 +2512,9 @@ struct ExprLoweringVisitorBase : ExprVisitor<Derived, LoweredValInfo>
 
     LoweredValInfo visitStringLiteralExpr(StringLiteralExpr* expr)
     {
-        return LoweredValInfo::simple(context->irBuilder->getStringValue(expr->value.getUnownedSlice()));
+        auto irLit = context->irBuilder->getStringValue(expr->value.getUnownedSlice());
+        context->shared->m_stringLiterals.add(irLit);
+        return LoweredValInfo::simple(irLit);
     }
 
     LoweredValInfo visitAggTypeCtorExpr(AggTypeCtorExpr* /*expr*/)
@@ -6813,6 +6812,18 @@ IRModule* generateIRForTranslationUnit(
         ensureAllDeclsRec(context, decl);
     }
 
+    // Build a global instruction to hold all the string
+    // literals used in the module.
+    {
+        auto& stringLits = sharedContext->m_stringLiterals;
+        auto stringLitCount = stringLits.getCount();
+        if(stringLitCount != 0)
+        {
+            builder->setInsertInto(module->getModuleInst());
+            builder->emitIntrinsicInst(builder->getVoidType(), kIROp_GlobalHashedStringLiterals, stringLitCount, stringLits.getBuffer());
+        }
+    }
+
 #if 0
     fprintf(stderr, "### GENERATED\n");
     dumpIR(module);
@@ -6852,9 +6863,6 @@ IRModule* generateIRForTranslationUnit(
     // call graph) based on constraints imposed by different instructions.
     propagateConstExpr(module, compileRequest->getSink());
 
-    // Replace calls to getStringHash, and save all the unique string lits in a GlobalHashedStringLiterals inst
-    replaceGetStringHashWithGlobal(module, *sharedBuilder);
-    
     // TODO: give error messages if any `undefined` or
     // `unreachable` instructions remain.
 
diff --git a/tests/ir/string-literal-hash.slang b/tests/ir/string-literal-hash.slang
index b6ab8bf4e..3e71b8a32 100644
--- a/tests/ir/string-literal-hash.slang
+++ b/tests/ir/string-literal-hash.slang
@@ -1,7 +1,13 @@
-//TEST(compute):COMPARE_COMPUTE: -cpu
 //TEST(compute):COMPARE_COMPUTE:
 //TEST(compute):COMPARE_COMPUTE: -vk
 
+// Note: disabled on CPU target until we can fill
+// in a more correct/complete `String` and `getStringHash`
+// for that target.
+//
+//TEST_DISABLED(compute):COMPARE_COMPUTE: -cpu
+
+
 import string_literal_module;
 
 //TEST_INPUT:ubuffer(data=[0 0 0 0], stride=4):out,name outputBuffer
diff --git a/tests/ir/string-literal.slang.expected b/tests/ir/string-literal.slang.expected
index 44464d97a..3f6b2f0ff 100644
--- a/tests/ir/string-literal.slang.expected
+++ b/tests/ir/string-literal.slang.expected
@@ -11,6 +11,7 @@ block %1(
 		param %tid	: UInt):
 	return_void
 }
+global_hashed_string_literals("Hello \t\n\0x083 World")
 }
 standard output = {
 }