[sksl][wgsl] Enable WGSL validation using Tint

- Introduced the SK_ENABLE_WGSL_VALIDATION macro which is currently only
  enabled when skslc gets compiled when using the `skia_compile_sksl_tests`
  setting.
- SkSLCompiler::toWGSL now validates its output using Tint's WGSL reader
  structures based on conditionally compiled code depending on the
  SK_ENABLE_WGSL_VALIDATION flag.
- Fixed `warning: use of deprecated language feature: struct members should be separated with commas"
  warnings that were generated for HelloWorld.wgsl.

Bug: skia:13092
Change-Id: Ib894457030004966221faf82f61360e390b95e22
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/537802
Commit-Queue: Arman Uguray <armansito@google.com>
Reviewed-by: Kevin Lubick <kjlubick@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>

BUILD.gn

@@ -718,6 +718,7 @@ if (skia_compile_sksl_tests) {
       "SK_DISABLE_TRACING",
       "SK_ENABLE_SPIRV_CROSS",
       "SK_ENABLE_SPIRV_VALIDATION",
+      "SK_ENABLE_WGSL_VALIDATION",
     ]
     sources = skslc_deps
     sources += [ "tools/skslc/Main.cpp" ]
@@ -735,6 +736,7 @@ if (skia_compile_sksl_tests) {
     deps = [
       ":run_sksllex",
       ":skvm_jit",
+      "//third_party/externals/dawn/src/tint:libtint",
       "//third_party/externals/spirv-tools:spvtools",
       "//third_party/externals/spirv-tools:spvtools_val",
       "//third_party/spirv-cross:spirv_cross",

bazel/BUILD.bazel

@@ -38,6 +38,7 @@ GENERAL_DEFINES = [
         "SK_DISABLE_TRACING",
         "SK_ENABLE_SPIRV_CROSS",
         "SK_ENABLE_SPIRV_VALIDATION",
+        "SK_ENABLE_WGSL_VALIDATION",
     ],
     "//conditions:default": [],
 }) + select({

bazel/external/dawn/BUILD.bazel

@@ -1079,7 +1079,7 @@ cc_library(
         "",
         "include",
     ],
-    visibility = ["//visibility:private"],  # only used by :dawn
+    visibility = ["//visibility:public"],
     deps = [
         # Tint specifically depends on spirv-tools/BUILD.gn:spirv_opt
         "@spirv_tools//:spirv_tools_opt",

src/sksl/BUILD.bazel

@@ -324,6 +324,7 @@ generated_cc_atom(
         "//src/sksl/ir:SkSLVariableReference_hdr",
         "//src/sksl/ir:SkSLVariable_hdr",
         "//src/sksl/transform:SkSLTransform_hdr",
+        "@dawn//:tint",
         "@spirv_tools",
     ],
 )

src/sksl/SkSLCompiler.cpp

@@ -59,6 +59,10 @@
 #include "spirv-tools/libspirv.hpp"
 #endif
 
+#ifdef SK_ENABLE_WGSL_VALIDATION
+#include "tint/tint.h"
+#endif
+
 #ifdef SKSL_STANDALONE
 #define REHYDRATE 0
 #include <fstream>
@@ -821,11 +825,42 @@ bool Compiler::toMetal(Program& program, std::string* out) {
     return result;
 }
 
+#if defined(SK_ENABLE_WGSL_VALIDATION)
+static bool validate_wgsl(ErrorReporter& reporter, const std::string& wgsl) {
+    tint::Source::File srcFile("", wgsl);
+    tint::Program program(tint::reader::wgsl::Parse(&srcFile));
+    if (program.Diagnostics().count() > 0) {
+        tint::diag::Formatter diagFormatter;
+        std::string diagOutput = diagFormatter.format(program.Diagnostics());
+#if defined(SKSL_STANDALONE)
+        reporter.error(Position(), diagOutput);
+        reporter.reportPendingErrors(Position());
+#else
+        SkDEBUGFAILF("%s", diagOutput.c_str());
+#endif
+        return false;
+    }
+    return true;
+}
+#endif  // defined(SK_ENABLE_WGSL_VALIDATION)
+
 bool Compiler::toWGSL(Program& program, OutputStream& out) {
     TRACE_EVENT0("skia.shaders", "SkSL::Compiler::toWGSL");
     AutoSource as(this, *program.fSource);
+#ifdef SK_ENABLE_WGSL_VALIDATION
+    StringStream wgsl;
+    WGSLCodeGenerator cg(fContext.get(), &program, &wgsl);
+    bool result = cg.generateCode();
+    if (result) {
+        std::string wgslString = wgsl.str();
+        result = validate_wgsl(this->errorReporter(), wgslString);
+        out.writeString(wgslString);
+    }
+#else
     WGSLCodeGenerator cg(fContext.get(), &program, &out);
-    return cg.generateCode();
+    bool result = cg.generateCode();
+#endif
+    return result;
 }
 
 #endif // defined(SKSL_STANDALONE) || SK_SUPPORT_GPU || SK_GRAPHITE_ENABLED

src/sksl/codegen/SkSLWGSLCodeGenerator.cpp

@@ -199,6 +199,20 @@ std::shared_ptr<SymbolTable> top_level_symbol_table(const FunctionDefinition& f)
     return f.body()->as<Block>().symbolTable()->fParent;
 }
 
+const char* delimiter_to_str(WGSLCodeGenerator::Delimiter delimiter) {
+    using Delim = WGSLCodeGenerator::Delimiter;
+    switch (delimiter) {
+        case Delim::kComma:
+            return ",";
+        case Delim::kSemicolon:
+            return ";";
+        case Delim::kNone:
+        default:
+            break;
+    }
+    return "";
+}
+
 // FunctionDependencyResolver visits the IR tree rooted at a particular function definition and
 // computes that function's dependencies on pipeline stage IO parameters. These are later used to
 // synthesize arguments when writing out function definitions.
@@ -387,7 +401,8 @@ void WGSLCodeGenerator::writeName(std::string_view name) {
 
 void WGSLCodeGenerator::writePipelineIODeclaration(Modifiers modifiers,
                                                    const Type& type,
-                                                   std::string_view name) {
+                                                   std::string_view name,
+                                                   Delimiter delimiter) {
     // In WGSL, an entry-point IO parameter is "one of either a built-in value or
     // assigned a location". However, some SkSL declarations, specifically sk_FragColor, can
     // contain both a location and a builtin modifier. In addition, WGSL doesn't have a built-in
@@ -403,34 +418,36 @@ void WGSLCodeGenerator::writePipelineIODeclaration(Modifiers modifiers,
     // https://www.w3.org/TR/WGSL/#builtin-inputs-outputs
     int location = modifiers.fLayout.fLocation;
     if (location >= 0) {
-        this->writeUserDefinedVariableDecl(type, name, location);
+        this->writeUserDefinedVariableDecl(type, name, location, delimiter);
     } else if (modifiers.fLayout.fBuiltin >= 0) {
         auto builtin = builtin_from_sksl_name(modifiers.fLayout.fBuiltin);
         if (builtin.has_value()) {
-            this->writeBuiltinVariableDecl(type, name, *builtin);
+            this->writeBuiltinVariableDecl(type, name, *builtin, delimiter);
         }
     }
 }
 
 void WGSLCodeGenerator::writeUserDefinedVariableDecl(const Type& type,
                                                      std::string_view name,
-                                                     int location) {
+                                                     int location,
+                                                     Delimiter delimiter) {
     this->write("@location(" + std::to_string(location) + ") ");
     this->writeName(name);
     this->write(": " + to_wgsl_type(type));
-    this->writeLine(";");
+    this->writeLine(delimiter_to_str(delimiter));
 }
 
 void WGSLCodeGenerator::writeBuiltinVariableDecl(const Type& type,
                                                  std::string_view name,
-                                                 Builtin kind) {
+                                                 Builtin kind,
+                                                 Delimiter delimiter) {
     this->write("@builtin(");
     this->write(to_wgsl_builtin_name(kind));
     this->write(") ");
 
     this->writeName(name);
     this->write(": " + to_wgsl_type(type));
-    this->writeLine(";");
+    this->writeLine(delimiter_to_str(delimiter));
 }
 
 void WGSLCodeGenerator::writeFunction(const FunctionDefinition& f) {
@@ -776,7 +793,8 @@ void WGSLCodeGenerator::writeStageInputStruct() {
             const Variable& v =
                     e->as<GlobalVarDeclaration>().declaration()->as<VarDeclaration>().var();
             if (v.modifiers().fFlags & Modifiers::kIn_Flag) {
-                this->writePipelineIODeclaration(v.modifiers(), v.type(), v.name());
+                this->writePipelineIODeclaration(
+                        v.modifiers(), v.type(), v.name(), Delimiter::kComma);
                 if (v.modifiers().fLayout.fBuiltin == SK_FRAGCOORD_BUILTIN) {
                     declaredFragCoordsBuiltin = true;
                 }
@@ -790,7 +808,8 @@ void WGSLCodeGenerator::writeStageInputStruct() {
             // but with members that have individual storage qualifiers?
             if (v.modifiers().fFlags & Modifiers::kIn_Flag) {
                 for (const auto& f : v.type().fields()) {
-                    this->writePipelineIODeclaration(f.fModifiers, *f.fType, f.fName);
+                    this->writePipelineIODeclaration(
+                            f.fModifiers, *f.fType, f.fName, Delimiter::kComma);
                     if (f.fModifiers.fLayout.fBuiltin == SK_FRAGCOORD_BUILTIN) {
                         declaredFragCoordsBuiltin = true;
                     }
@@ -801,7 +820,7 @@ void WGSLCodeGenerator::writeStageInputStruct() {
 
     if (ProgramConfig::IsFragment(fProgram.fConfig->fKind) &&
         fRequirements.mainNeedsCoordsArgument && !declaredFragCoordsBuiltin) {
-        this->writeLine("@builtin(position) sk_FragCoord: vec4<f32>;");
+        this->writeLine("@builtin(position) sk_FragCoord: vec4<f32>,");
     }
 
     fIndentation--;
@@ -827,7 +846,8 @@ void WGSLCodeGenerator::writeStageOutputStruct() {
             const Variable& v =
                     e->as<GlobalVarDeclaration>().declaration()->as<VarDeclaration>().var();
             if (v.modifiers().fFlags & Modifiers::kOut_Flag) {
-                this->writePipelineIODeclaration(v.modifiers(), v.type(), v.name());
+                this->writePipelineIODeclaration(
+                        v.modifiers(), v.type(), v.name(), Delimiter::kComma);
             }
         } else if (e->is<InterfaceBlock>()) {
             const Variable& v = e->as<InterfaceBlock>().variable();
@@ -838,7 +858,8 @@ void WGSLCodeGenerator::writeStageOutputStruct() {
             // but with members that have individual storage qualifiers?
             if (v.modifiers().fFlags & Modifiers::kOut_Flag) {
                 for (const auto& f : v.type().fields()) {
-                    this->writePipelineIODeclaration(f.fModifiers, *f.fType, f.fName);
+                    this->writePipelineIODeclaration(
+                            f.fModifiers, *f.fType, f.fName, Delimiter::kComma);
                 }
             }
         }

src/sksl/codegen/SkSLWGSLCodeGenerator.h

@@ -79,6 +79,17 @@ public:
         kPipelineOutputs = 2,
     };
 
+    // Variable declarations can be terminated by:
+    //   - comma (","), e.g. in struct member declarations or function parameters
+    //   - semicolon (";"), e.g. in function scope variables
+    // A "none" option is provided to skip the delimiter when not needed, e.g. at the end of a list
+    // of declarations.
+    enum class Delimiter {
+        kComma,
+        kSemicolon,
+        kNone,
+    };
+
     struct ProgramRequirements {
         using DepsMap = SkTHashMap<const FunctionDeclaration*, FunctionDependencies>;
 
@@ -115,9 +126,18 @@ private:
     void writeName(std::string_view name);
 
     // Helpers to declare a pipeline stage IO parameter declaration.
-    void writePipelineIODeclaration(Modifiers modifiers, const Type& type, std::string_view name);
-    void writeUserDefinedVariableDecl(const Type& type, std::string_view name, int location);
-    void writeBuiltinVariableDecl(const Type& type, std::string_view name, Builtin kind);
+    void writePipelineIODeclaration(Modifiers modifiers,
+                                    const Type& type,
+                                    std::string_view name,
+                                    Delimiter delimiter);
+    void writeUserDefinedVariableDecl(const Type& type,
+                                      std::string_view name,
+                                      int location,
+                                      Delimiter delimiter);
+    void writeBuiltinVariableDecl(const Type& type,
+                                  std::string_view name,
+                                  Builtin kind,
+                                  Delimiter delimiter);
 
     // Write a function definition.
     void writeFunction(const FunctionDefinition& f);

tests/sksl/shared/HelloWorld.wgsl

@@ -1,9 +1,9 @@
 struct FSIn {
-    @builtin(front_facing) sk_Clockwise: bool;
-    @builtin(position) sk_FragCoord: vec4<f32>;
+    @builtin(front_facing) sk_Clockwise: bool,
+    @builtin(position) sk_FragCoord: vec4<f32>,
 };
 struct FSOut {
-    @location(0) sk_FragColor: vec4<f32>;
+    @location(0) sk_FragColor: vec4<f32>,
 };
 fn main(coords: vec2<f32>) -> vec4<f32> {
     return vec4<f32>(0.0, 1.0, 0.0, 1.0);

third_party/file_map_for_bazel.json

@@ -7,6 +7,7 @@
   "dawn/native/DawnNative.h": "@dawn",
   "dawn/native/OpenGLBackend.h": "@dawn",
   "dawn/native/VulkanBackend.h": "@dawn",
+  "tint/tint.h": "@dawn//:tint",
   "webgpu/webgpu.h": "@dawn",
   "webgpu/webgpu_cpp.h": "@dawn",