Improvements to NVRTC diagnostic parsing (#2504)

* #include an absolute path didn't work - because paths were taken to always be relative.

* Float16 support for C++/CPU based targets with f16tof32 and f32tof16.

* Small correction around INF/NAN handling for f32tof16

* Small improvement to f16tof32

* Disable CUDA test for now.

* Improvements to NVRTC diagnostic parsing.
Handle compilerSpecificArgs.
Fix issue with terminating nul ending up in diagnostic string.

* Improved NVRTC error parsing.
f32tof16 and f16tof32 work in principal on CUDA.

* Small update to test, although they remain disabled.

* Work around SLANG_E_NOT_AVAILABLE being turned into ignored, when a legitimate error is found

* A more tightly constrained fallback NVRTC diagnostic parsing.

* Remove CharUtil, as not neeed.

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

5 files changed, 139 insertions, 16 deletions

diff --git a/source/compiler-core/slang-nvrtc-compiler.cpp b/source/compiler-core/slang-nvrtc-compiler.cpp
index 1a112d67f..f650a11f5 100644
--- a/source/compiler-core/slang-nvrtc-compiler.cpp
+++ b/source/compiler-core/slang-nvrtc-compiler.cpp
@@ -14,6 +14,7 @@
 #include "../core/slang-semantic-version.h"
 
 #include "../core/slang-shared-library.h"
+#include "../core/slang-char-util.h"
 
 #include "slang-artifact-diagnostic-util.h"
 #include "slang-artifact-util.h"
@@ -228,24 +229,79 @@ static SlangResult _parseNVRTCLine(SliceAllocator& allocator, const UnownedStrin
     if (split.getCount() >= 3)
     {
         // tests/cuda/cuda-compile.cu(7): warning: variable "c" is used before its value is set
-
         const auto split1 = split[1].trim();
 
-        if (split1 == "error")
+        Severity severity = Severity::Unknown;
+
+        if (split1 == toSlice("error") || 
+            split1 == toSlice("catastrophic error"))
         {
-            outDiagnostic.severity = Severity::Error;
+            severity = Severity::Error;
         }
-        else if (split1 == "warning")
+        else if (split1 == toSlice("warning"))
         {
-            outDiagnostic.severity = Severity::Warning;
+            severity = Severity::Warning;
         }
-        outDiagnostic.text = allocator.allocate(split[2].trim());
+        else
+        {
+            // Fall back position to try and determine if this really is some kind of 
+            // error/warning without succeeding when it's due to some other property 
+            // of the output diagnostics. 
+            //
+            // Anything ending with " warning:" or " error:" in effect.
+            
+            // We can expand to include character after as this is split1, as must be followed by at a minimum 
+            // : (as the split has at least 3 parts).
+            const UnownedStringSlice expandSplit1(split1.begin(), split1.end() + 1);
 
-        SLANG_RETURN_ON_FAIL(_parseLocation(allocator, split[0], outDiagnostic));
-        return SLANG_OK;
+            if (expandSplit1.endsWith(toSlice(" error:")))
+            {
+                severity = Severity::Error;
+            }
+            else if (expandSplit1.endsWith(toSlice(" warning:")))
+            {
+                severity = Severity::Warning;
+            }
+        }
+
+        if (severity != Severity::Unknown)
+        {
+            // The text is everything following the : after the warning. 
+            UnownedStringSlice text(split[2].begin(), split.getLast().end());
+       
+            // Trim whitespace at start and end
+            text = text.trim();
+
+            // Set the diagnostic
+            outDiagnostic.severity = severity;
+            outDiagnostic.text = allocator.allocate(text);
+            SLANG_RETURN_ON_FAIL(_parseLocation(allocator, split[0], outDiagnostic));
+
+            return SLANG_OK;
+        }
+
+        // TODO(JS): Note here if it's not possible to determine a line as being the main diagnostics
+        // we fall through to it potentially being a note.
+        //
+        // That could mean a valid diagnostic (from NVRTCs point of view) is ignored/noted, because this code
+        // can't parse it. Ideally that situation would lead to an error such that we can detect
+        // and things will fail.
+        // 
+        // So we might want to revisit this determination in the future.
     }
-   
-    return SLANG_E_NOT_FOUND;
+
+    // There isn't a diagnostic on this line
+    if (line.getLength() == 0 ||
+        line.trim().getLength() == 0)
+    {
+        return SLANG_E_NOT_FOUND;
+    }
+
+    // We'll assume it's info, associated with a previous line
+    outDiagnostic.severity = Severity::Info;
+    outDiagnostic.text = allocator.allocate(line);
+
+    return SLANG_OK;
 }
 
 /* An implementation of Path::Visitor that can be used for finding NVRTC shared library installations. */
@@ -819,6 +875,18 @@ SlangResult NVRTCDownstreamCompiler::compile(const DownstreamCompileOptions& opt
         cmdLine.addArg("-DSLANG_CUDA_ENABLE_OPTIX");
     }
 
+    // Add any compiler specific options
+    // NOTE! If these clash with any previously set options (as set via other flags)
+    // compilation might fail.
+    if (options.compilerSpecificArguments.count > 0)
+    {
+        for (auto compilerSpecificArg : options.compilerSpecificArguments)
+        {
+            const char*const arg = compilerSpecificArg;
+            cmdLine.addArg(arg);
+        }
+    }
+
     SLANG_ASSERT(headers.getCount() == headerIncludeNames.getCount());
 
     ComPtr<ISlangBlob> sourceBlob;
@@ -868,6 +936,11 @@ SlangResult NVRTCDownstreamCompiler::compile(const DownstreamCompileOptions& opt
         {
             char* dst = rawDiagnostics.prepareForAppend(Index(logSize));
             SLANG_NVRTC_RETURN_ON_FAIL(m_nvrtcGetProgramLog(program, dst));
+
+            // If there is a terminating zero remove it, as the rawDiagnostics
+            // string will already contain one.
+            logSize -= size_t(logSize > 0 && dst[logSize - 1] == 0);
+
             rawDiagnostics.appendInPlace(dst, Index(logSize));
 
             diagnostics->setRaw(SliceUtil::asCharSlice(rawDiagnostics));
@@ -875,24 +948,61 @@ SlangResult NVRTCDownstreamCompiler::compile(const DownstreamCompileOptions& opt
 
         SliceAllocator allocator;
 
+        // Get all of the lines
+        List<UnownedStringSlice> lines;
+        StringUtil::calcLines(rawDiagnostics.getUnownedSlice(), lines);
+
+        // Remove any trailing empty lines
+        while (lines.getCount() && lines.getLast().getLength() == 0)
+        {
+            lines.removeLast();
+        }
+
+        // Find the index searching from last line, that is blank
+        // indicating the end of the output
+        Index lastIndex = lines.getCount();
+
+        // Look for the first blank line after this point.
+        // We'll assume any information after that blank line to the end of the diagnostic
+        // is compilation summary information.
+        for (Index i = lastIndex - 1; i >= 0; --i)
+        {
+            if (lines[i].getLength() == 0)
+            {
+                lastIndex = i;
+                break;
+            }
+        }
+
         // Parse the diagnostics here
-        for (auto line : LineParser(rawDiagnostics.getUnownedSlice()))
+        for (auto line : makeConstArrayView(lines.getBuffer(), lastIndex))
         {
             ArtifactDiagnostic diagnostic;
             SlangResult lineRes = _parseNVRTCLine(allocator, line, diagnostic);
 
             if (SLANG_SUCCEEDED(lineRes))
             {
+                // We only allow info diagnostics after a 'regular' diagnostic.
+                if (diagnostic.severity == ArtifactDiagnostic::Severity::Info && 
+                    diagnostics->getCount() == 0)
+                {
+                    continue;
+                }
+
                 diagnostics->add(diagnostic);
             }
             else if (lineRes != SLANG_E_NOT_FOUND)
             {
+                // If there is an error exit
+                // But if SLANG_E_NOT_FOUND that just means this line couldn't be parsed, so ignore.
                 return lineRes;
             }
         }
 
-        // if it has a compilation error.. set on output
-        if (diagnostics->hasOfAtLeastSeverity(ArtifactDiagnostic::Severity::Error))
+        // If it has a compilation error.. and there isn't already an error set
+        // set as failed.
+        if (SLANG_SUCCEEDED(diagnostics->getResult()) && 
+            diagnostics->hasOfAtLeastSeverity(ArtifactDiagnostic::Severity::Error))
         {
             diagnostics->setResult(SLANG_FAIL);
         }
diff --git a/source/slang/hlsl.meta.slang b/source/slang/hlsl.meta.slang
index ec2e6de95..1cff7d6f3 100644
--- a/source/slang/hlsl.meta.slang
+++ b/source/slang/hlsl.meta.slang
@@ -1742,6 +1742,8 @@ matrix<T,N,M> exp2(matrix<T,N,M> x)
 __target_intrinsic(glsl, "unpackHalf2x16($0).x")
 __glsl_version(420)
 __target_intrinsic(hlsl)
+__cuda_sm_version(6.0)
+__target_intrinsic(cuda, "__half2float(__short_as_half($0))")
 float f16tof32(uint value);
 
 __generic<let N : int>
@@ -1757,6 +1759,8 @@ vector<float, N> f16tof32(vector<uint, N> value)
 __target_intrinsic(glsl, "packHalf2x16(vec2($0,0.0))")
 __glsl_version(420)
 __target_intrinsic(hlsl)
+__cuda_sm_version(6.0)
+__target_intrinsic(cuda, "__half_as_ushort(__float2half($0))")
 uint f32tof16(float value);
 
 __generic<let N : int>
diff --git a/tests/hlsl-intrinsic/f16tof32.slang b/tests/hlsl-intrinsic/f16tof32.slang
index b73ade4cf..78c5fdae6 100644
--- a/tests/hlsl-intrinsic/f16tof32.slang
+++ b/tests/hlsl-intrinsic/f16tof32.slang
@@ -2,7 +2,7 @@
 //TEST(compute):COMPARE_COMPUTE_EX:-slang -compute
 //TEST(compute):COMPARE_COMPUTE_EX:-slang -compute -dx12
 //TEST(compute, vulkan):COMPARE_COMPUTE_EX:-vk -compute
-//DISABLE_TEST(compute):COMPARE_COMPUTE_EX:-cuda -compute
+//DISABLE_TEST(compute):COMPARE_COMPUTE_EX:-cuda -compute -render-features half
 
 //TEST_INPUT:ubuffer(data=[0 0 0 0  0 0 0 0], stride=4):out,name outputBuffer
 RWStructuredBuffer<float> outputBuffer;
diff --git a/tests/hlsl-intrinsic/f32tof16.slang b/tests/hlsl-intrinsic/f32tof16.slang
index 465b2840a..ebcb6b40a 100644
--- a/tests/hlsl-intrinsic/f32tof16.slang
+++ b/tests/hlsl-intrinsic/f32tof16.slang
@@ -2,7 +2,7 @@
 //TEST(compute):COMPARE_COMPUTE_EX:-slang -compute
 //TEST(compute):COMPARE_COMPUTE_EX:-slang -compute -dx12
 //TEST(compute, vulkan):COMPARE_COMPUTE_EX:-vk -compute
-//DISABLE_TEST(compute):COMPARE_COMPUTE_EX:-cuda -compute
+//DISABLE_TEST(compute):COMPARE_COMPUTE_EX:-cuda -compute -render-features half 
 
 //TEST_INPUT:ubuffer(data=[0 0 0 0 0 0 0 0], stride=4):out,name outputBuffer
 RWStructuredBuffer<uint> outputBuffer;
diff --git a/tools/render-test/render-test-main.cpp b/tools/render-test/render-test-main.cpp
index 805f08d10..d3c284132 100644
--- a/tools/render-test/render-test-main.cpp
+++ b/tools/render-test/render-test-main.cpp
@@ -510,7 +510,16 @@ SlangResult RenderTestApp::initialize(
     //
     ComPtr<ISlangBlob> outDiagnostics;
     auto result = device->createProgram(m_compilationOutput.output.desc, m_shaderProgram.writeRef(), outDiagnostics.writeRef());
-    SLANG_RETURN_ON_FAIL(result);
+
+    // If there was a failure creating a program, we can't continue
+    // Special case SLANG_E_NOT_AVAILABLE error code to make it a failure,
+    // as it is also used to indicate an attempt setup something failed gracefully (because it couldn't be supported)
+    // but that's not this.
+    if (SLANG_FAILED(result))
+    {
+        result = (result == SLANG_E_NOT_AVAILABLE) ? SLANG_FAIL : result;
+        return result;
+    }
 
 	m_device = device;