Diagnostic improvements for direct spirv (#3053)

* Fail on an unhandled spv operand

* Diagnose on emitting a function with no definition or intrinsic

* clearer error message

* Add assert

* Add assert

* remove unused assert

* Disagnostic on snippet parsing failure

* Mention unimplemented instruction in error message

* mention unhandled local instruction for spirv in error message

* Allow specifying dump options in dumpIRToString

---------

Co-authored-by: Yong He <yonghe@outlook.com>

7 files changed, 49 insertions, 30 deletions

diff --git a/source/slang/slang-diagnostic-defs.h b/source/slang/slang-diagnostic-defs.h
index 286551716..14a03884b 100644
--- a/source/slang/slang-diagnostic-defs.h
+++ b/source/slang/slang-diagnostic-defs.h
@@ -238,6 +238,10 @@ DIAGNOSTIC(20014, Error, classIsReservedKeyword, "'class' is a reserved keyword
 
 DIAGNOSTIC(20101, Warning, unintendedEmptyStatement, "potentially unintended empty statement at this location; use {} instead.")
 
+// 29xxx - Snippet parsing
+DIAGNOSTIC(29000, Error, snippetParsingFailed, "unable to parse target intrinsic snippet: $0")
+
+
 //
 // 3xxxx - Semantic analysis
 //
@@ -723,4 +727,6 @@ DIAGNOSTIC(99999, Error, compilationAborted, "Slang compilation aborted due to i
 DIAGNOSTIC(99999, Error, compilationAbortedDueToException, "Slang compilation aborted due to an exception of $0: $1")
 DIAGNOSTIC(99999, Internal, serialDebugVerificationFailed, "Verification of serial debug information failed.")
 
+DIAGNOSTIC(99999, Internal, noBlocksOrIntrinsic, "no blocks found for function definition, is there a '$0' intrinsic missing?")
+
 #undef DIAGNOSTIC
diff --git a/source/slang/slang-emit-spirv.cpp b/source/slang/slang-emit-spirv.cpp
index f7d8a4ec6..30d1b4ecb 100644
--- a/source/slang/slang-emit-spirv.cpp
+++ b/source/slang/slang-emit-spirv.cpp
@@ -3,6 +3,7 @@
 #include "slang-compiler.h"
 #include "slang-emit-base.h"
 
+#include "slang-ir-util.h"
 #include "slang-ir.h"
 #include "slang-ir-insts.h"
 #include "slang-ir-layout.h"
@@ -287,8 +288,6 @@ struct SPIRVEmitContext
         /// The Slang IR module being translated
     IRModule* m_irModule;
 
-    DiagnosticSink* m_sink;
-
     // [2.2: Terms]
     //
     // > <id>: A numerical name; the name used to refer to an object, a type,
@@ -1066,6 +1065,7 @@ struct SPIRVEmitContext
             {
                 SpvStorageClass storageClass = SpvStorageClassFunction;
                 auto ptrType = as<IRPtrTypeBase>(inst);
+                SLANG_ASSERT(ptrType);
                 if (ptrType->hasAddressSpace())
                     storageClass = (SpvStorageClass)ptrType->getAddressSpace();
                 if (storageClass == SpvStorageClassStorageBuffer)
@@ -1206,8 +1206,9 @@ struct SPIRVEmitContext
         // ...
 
         default:
-            SLANG_UNIMPLEMENTED_X("unhandled instruction opcode for global instruction");
-            UNREACHABLE_RETURN(nullptr);
+            String e = "Unhandled global inst in spirv-emit: "
+                + dumpIRToString(inst, {IRDumpOptions::Mode::Detailed, 0});
+            SLANG_UNIMPLEMENTED_X(e.begin());
         }
     }
 
@@ -1342,6 +1343,7 @@ struct SPIRVEmitContext
     SpvInst* emitGlobalVar(IRGlobalVar* globalVar)
     {
         auto layout = getVarLayout(globalVar);
+        SLANG_ASSERT(layout);
         auto storageClass = SpvStorageClassUniform;
         if (auto ptrType = as<IRPtrTypeBase>(globalVar->getDataType()))
         {
@@ -1398,6 +1400,9 @@ struct SPIRVEmitContext
         /// Emit a SPIR-V function definition for the Slang IR function `irFunc`.
     SpvInst* emitFuncDefinition(IRFunc* irFunc)
     {
+        if(!irFunc->getFirstBlock())
+            m_sink->diagnose(irFunc, Diagnostics::noBlocksOrIntrinsic, "spirv");
+
         // [2.4: Logical Layout of a Module]
         //
         // > All function definitions (functions with a body).
@@ -1588,8 +1593,11 @@ struct SPIRVEmitContext
         switch( inst->getOp() )
         {
         default:
-            SLANG_UNIMPLEMENTED_X("unhandled instruction opcode for local instruction");
-            break;
+            {
+                String e = "Unhandled local inst in spirv-emit: "
+                    + dumpIRToString(inst, {IRDumpOptions::Mode::Detailed, 0});
+                SLANG_UNIMPLEMENTED_X(e.getBuffer());
+            }
         case kIROp_Specialize:
             return nullptr;
         case kIROp_Var:
@@ -2228,6 +2236,7 @@ struct SPIRVEmitContext
         IRTargetIntrinsicDecoration* intrinsic)
     {
         SpvSnippet* snippet = getParsedSpvSnippet(intrinsic);
+        SLANG_ASSERT(snippet);
         SpvSnippetEmitContext context;
         context.irResultType = inst->getDataType();
         context.resultType = ensureInst(inst->getFullType());
@@ -2894,9 +2903,8 @@ struct SPIRVEmitContext
     }
 
     SPIRVEmitContext(IRModule* module, TargetRequest* target, DiagnosticSink* sink)
-        : SPIRVEmitSharedContext(module, target)
+        : SPIRVEmitSharedContext(module, target, sink)
         , m_irModule(module)
-        , m_sink(sink)
         , m_memoryArena(2048)
     {
     }
diff --git a/source/slang/slang-ir-spirv-legalize.cpp b/source/slang/slang-ir-spirv-legalize.cpp
index a0183d93f..daea4582a 100644
--- a/source/slang/slang-ir-spirv-legalize.cpp
+++ b/source/slang/slang-ir-spirv-legalize.cpp
@@ -301,6 +301,23 @@ struct SPIRVLegalizationContext : public SourceEmitterBase
     }
 };
 
+SpvSnippet* SPIRVEmitSharedContext::getParsedSpvSnippet(IRTargetIntrinsicDecoration* intrinsic)
+{
+    RefPtr<SpvSnippet> snippet;
+    if (m_parsedSpvSnippets.tryGetValue(intrinsic, snippet))
+    {
+        return snippet.Ptr();
+    }
+    snippet = SpvSnippet::parse(intrinsic->getDefinition());
+    if(!snippet)
+    {
+        m_sink->diagnose(intrinsic, Diagnostics::snippetParsingFailed, intrinsic->getDefinition());
+        return nullptr;
+    }
+    m_parsedSpvSnippets[intrinsic] = snippet;
+    return snippet;
+}
+
 void legalizeSPIRV(SPIRVEmitSharedContext* sharedContext, IRModule* module)
 {
     SPIRVLegalizationContext context(sharedContext, module);
diff --git a/source/slang/slang-ir-spirv-legalize.h b/source/slang/slang-ir-spirv-legalize.h
index 5d5a7d3ab..797745f88 100644
--- a/source/slang/slang-ir-spirv-legalize.h
+++ b/source/slang/slang-ir-spirv-legalize.h
@@ -18,21 +18,11 @@ struct SPIRVEmitSharedContext
     IRModule* m_irModule;
     TargetRequest* m_targetRequest;
     Dictionary<IRTargetIntrinsicDecoration*, RefPtr<SpvSnippet>> m_parsedSpvSnippets;
-    SPIRVEmitSharedContext(IRModule* module, TargetRequest* target)
-        : m_irModule(module), m_targetRequest(target)
+    DiagnosticSink* m_sink;
+    SPIRVEmitSharedContext(IRModule* module, TargetRequest* target, DiagnosticSink* sink)
+        : m_irModule(module), m_targetRequest(target), m_sink(sink)
     {}
-
-    SpvSnippet* getParsedSpvSnippet(IRTargetIntrinsicDecoration* intrinsic)
-    {
-        RefPtr<SpvSnippet> snippet;
-        if (m_parsedSpvSnippets.tryGetValue(intrinsic, snippet))
-        {
-            return snippet.Ptr();
-        }
-        snippet = SpvSnippet::parse(intrinsic->getDefinition());
-        m_parsedSpvSnippets[intrinsic] = snippet;
-        return snippet;
-    }
+    SpvSnippet* getParsedSpvSnippet(IRTargetIntrinsicDecoration* intrinsic);
 };
 
 void legalizeIRForSPIRV(
diff --git a/source/slang/slang-ir-spirv-snippet.cpp b/source/slang/slang-ir-spirv-snippet.cpp
index 81e4d0bff..a263ad7f7 100644
--- a/source/slang/slang-ir-spirv-snippet.cpp
+++ b/source/slang/slang-ir-spirv-snippet.cpp
@@ -288,7 +288,7 @@ RefPtr<SpvSnippet> SpvSnippet::parse(UnownedStringSlice definition)
                         }
                         else
                         {
-                            SLANG_ASSERT(!"Invalid SPV ASM operand.");
+                            SLANG_UNEXPECTED(("Invalid SPV ASM operand: \"" + identifier + "\"").getBuffer());
                         }
                     }
                     break;
@@ -302,7 +302,7 @@ RefPtr<SpvSnippet> SpvSnippet::parse(UnownedStringSlice definition)
     }
     catch (const Slang::Misc::TextFormatException&)
     {
-        SLANG_ASSERT(!"Invalid ASM format.");
+        return nullptr;
     }
     return snippet;
 }
diff --git a/source/slang/slang-ir-util.cpp b/source/slang/slang-ir-util.cpp
index 07aaa127f..41ac19ee9 100644
--- a/source/slang/slang-ir-util.cpp
+++ b/source/slang/slang-ir-util.cpp
@@ -258,14 +258,10 @@ void moveInstChildren(IRInst* dest, IRInst* src)
     }
 }
 
-String dumpIRToString(IRInst* root)
+String dumpIRToString(IRInst* root, IRDumpOptions options)
 {
     StringBuilder sb;
     StringWriter writer(&sb, Slang::WriterFlag::AutoFlush);
-    IRDumpOptions options = {};
-#if 1
-    options.flags = IRDumpOptions::Flag::DumpDebugIds;
-#endif
     dumpIR(root, options, nullptr, &writer);
     return sb.toString();
 }
diff --git a/source/slang/slang-ir-util.h b/source/slang/slang-ir-util.h
index 983c39218..fc44b8d30 100644
--- a/source/slang/slang-ir-util.h
+++ b/source/slang/slang-ir-util.h
@@ -165,7 +165,9 @@ void copyNameHintDecoration(IRInst* dest, IRInst* src);
 IRInst* getRootAddr(IRInst* addrInst);
 bool canAddressesPotentiallyAlias(IRGlobalValueWithCode* func, IRInst* addr1, IRInst* addr2);
 
-String dumpIRToString(IRInst* root);
+String dumpIRToString(
+    IRInst* root,
+    IRDumpOptions options = {IRDumpOptions::Mode::Simplified, IRDumpOptions::Flag::DumpDebugIds});
 
 // Returns whether a call insts can be treated as a pure functional inst, and thus can be
 // DCE'd and deduplicated.