Add validations on clip_distances and maximum inter-stage location

This patch adds the validation on the use of `clip_distances` and
the maximum location of the inter-stage shader variables according
to the latest WebGPU SPEC.

`clip_distances` will not only consume inter-stage shader variable
slots, but also consume the maximum location of inter-stage shader
variables, which is required by Vulkan. According to the Vulkan
validation layer, the total number of inter-stage shader components
is computed by the maximum location of all the inter-stage shader
variables, so we should reserve the locations for `clip_distances`.

For example, suppose the maximum location of the vertex outputs is
30, and there is builtin position (4) and clip_distances with size
1, so the Vulkan validation layer thinks the total vertex shader
output components is 129 (31 * 4 + 4 + 1) which is over the maximum
number of the total vertex shader output components on Intel GPUs.

Bug: 358408571
Test: dawn_unittests, dawn_end2end_tests
Change-Id: If89cb10b63011dd34df71389411072b4a5cc5d18
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/206354
Reviewed-by: Corentin Wallez <[email protected]>
Reviewed-by: Kai Ninomiya <[email protected]>
Commit-Queue: Kai Ninomiya <[email protected]>

Author: Jiawei-Shao

src/dawn/native/ShaderModule.cpp

@@ -697,7 +697,12 @@ ResultOrError<std::unique_ptr<EntryPointMetadata>> ReflectEntryPointUsingTint(
             metadata->usedVertexInputs.set(location);
         }
 
-        // Vertex ouput (inter-stage variables) reflection.
+        // Vertex output (inter-stage variables) reflection.
+        uint32_t clipDistancesSlots = 0;
+        if (entryPoint.clip_distances_size.has_value()) {
+            clipDistancesSlots = RoundUp(*entryPoint.clip_distances_size, 4) / 4;
+        }
+        uint32_t minInvalidLocation = maxInterStageShaderVariables - clipDistancesSlots;
         for (const auto& outputVar : entryPoint.output_variables) {
             EntryPointMetadata::InterStageVariableInfo variable;
             variable.name = outputVar.variable_name;
@@ -712,10 +717,19 @@ ResultOrError<std::unique_ptr<EntryPointMetadata>> ReflectEntryPointUsingTint(
                                 outputVar.interpolation_sampling));
 
             uint32_t location = outputVar.attributes.location.value();
-            if (DelayedInvalidIf(location >= maxInterStageShaderVariables,
-                                 "Vertex output variable \"%s\" has a location (%u) that "
-                                 "is greater than or equal to (%u).",
-                                 outputVar.name, location, maxInterStageShaderVariables)) {
+            if (location >= minInvalidLocation) {
+                if (clipDistancesSlots > 0) {
+                    metadata->infringedLimitErrors.push_back(absl::StrFormat(
+                        "Vertex output variable \"%s\" has a location (%u) that "
+                        "is too large. It should be less than (%u = %u - %u (clip_distances)).",
+                        outputVar.name, location, minInvalidLocation, maxInterStageShaderVariables,
+                        clipDistancesSlots));
+                } else {
+                    metadata->infringedLimitErrors.push_back(
+                        absl::StrFormat("Vertex output variable \"%s\" has a location (%u) that "
+                                        "is too large. It should be less than (%u).",
+                                        outputVar.name, location, minInvalidLocation));
+                }
                 continue;
             }
 
@@ -724,12 +738,8 @@ ResultOrError<std::unique_ptr<EntryPointMetadata>> ReflectEntryPointUsingTint(
         }
 
         // Other vertex metadata.
-        metadata->totalInterStageShaderVariables = entryPoint.output_variables.size();
-        if (entryPoint.clip_distances_size.has_value()) {
-            metadata->totalInterStageShaderVariables +=
-                RoundUp(*entryPoint.clip_distances_size, 4) / 4;
-        }
-
+        metadata->totalInterStageShaderVariables =
+            entryPoint.output_variables.size() + clipDistancesSlots;
         if (metadata->totalInterStageShaderVariables > maxInterStageShaderVariables) {
             size_t userDefinedOutputVariables = entryPoint.output_variables.size();
 

src/dawn/tests/end2end/MaxLimitTests.cpp

@@ -886,8 +886,7 @@ class MaxInterStageShaderVariablesLimitTests : public MaxLimitTests {
         DAWN_TEST_UNSUPPORTED_IF(IsCompatibilityMode() &&
                                  (spec.hasSampleIndex || spec.hasSampleMask));
 
-        wgpu::RenderPipeline pipeline = CreateRenderPipeline(spec);
-        EXPECT_NE(nullptr, pipeline.Get());
+        CreateRenderPipeline(spec);
     }
 
   protected:
@@ -916,7 +915,7 @@ class MaxInterStageShaderVariablesLimitTests : public MaxLimitTests {
             ++builtinVariableCount;
         }
         if (spec.clipDistancesSize.has_value()) {
-            builtinVariableCount += dawn::RoundUp(*spec.clipDistancesSize, 4) / 4;
+            builtinVariableCount += RoundUp(*spec.clipDistancesSize, 4) / 4;
         }
         return baseLimits.maxInterStageShaderVariables - builtinVariableCount;
     }
@@ -1137,6 +1136,59 @@ TEST_P(MaxInterStageShaderVariablesLimitTests, RenderPointList_ClipDistances) {
     }
 }
 
+// Tests that using @builtin(clip_distances) will decrease the maximum location of the inter-stage
+// shader variable, while the PointList primitive topology doesn't affect the maximum location of
+// the inter-stage shader variable.
+TEST_P(MaxInterStageShaderVariablesLimitTests, MaxLocation_ClipDistances) {
+    DAWN_TEST_UNSUPPORTED_IF(!mSupportsClipDistances);
+
+    wgpu::Limits baseLimits = GetAdapterLimits().limits;
+
+    constexpr std::array<wgpu::PrimitiveTopology, 2> kPrimitives = {
+        {wgpu::PrimitiveTopology::TriangleList, wgpu::PrimitiveTopology::PointList}};
+    for (wgpu::PrimitiveTopology primitive : kPrimitives) {
+        for (uint32_t clipDistanceSize = 1; clipDistanceSize <= 8; ++clipDistanceSize) {
+            uint32_t colorLocation =
+                baseLimits.maxInterStageShaderVariables - 1 - RoundUp(clipDistanceSize, 4) / 4;
+            std::stringstream stream;
+            stream << R"(
+    enable clip_distances;
+    struct VertexOut {
+        @location()"
+                   << colorLocation << ") color : vec4f,\n"
+                   << R"(
+        @builtin(clip_distances) clipDistances : array<f32, )"
+                   << clipDistanceSize << ">,\n"
+                   << R"(
+        @builtin(position) pos : vec4f,
+    }
+    struct FragmentIn {
+        @location()"
+                   << colorLocation << ") color : vec4f,\n"
+                   << R"(
+        @builtin(position) pos : vec4f,
+    }
+    @vertex fn vsMain() -> VertexOut {
+        var vout : VertexOut;
+        return vout;
+    }
+    @fragment fn fsMain(fragIn : FragmentIn) -> @location(0) vec4f {
+        return fragIn.pos;
+    })";
+
+            wgpu::ShaderModule shaderModule = utils::CreateShaderModule(device, stream.str());
+            utils::ComboRenderPipelineDescriptor descriptor;
+            descriptor.vertex.module = shaderModule;
+            descriptor.cFragment.module = shaderModule;
+            descriptor.vertex.bufferCount = 0;
+            descriptor.cBuffers[0].attributeCount = 0;
+            descriptor.cTargets[0].format = wgpu::TextureFormat::RGBA8Unorm;
+            descriptor.primitive.topology = primitive;
+            device.CreateRenderPipeline(&descriptor);
+        }
+    }
+}
+
 DAWN_INSTANTIATE_TEST(MaxInterStageShaderVariablesLimitTests,
                       D3D11Backend(),
                       D3D12Backend({}, {"use_dxc"}),
@@ -1154,10 +1206,7 @@ class MaxVertexAttributesPipelineCreationTests : public MaxLimitTests {
         bool hasInstanceIndex;
     };
 
-    void DoTest(const TestSpec& spec) {
-        wgpu::RenderPipeline pipeline = CreateRenderPipeline(spec);
-        EXPECT_NE(nullptr, pipeline.Get());
-    }
+    void DoTest(const TestSpec& spec) { CreateRenderPipeline(spec); }
 
   private:
     wgpu::RenderPipeline CreateRenderPipeline(const TestSpec& spec) {

src/dawn/tests/unittests/validation/RenderPipelineValidationTests.cpp

@@ -3409,5 +3409,61 @@ TEST_F(ClipDistancesValidationTest, ClipDistancesAgainstMaxInterStageShaderVaria
     }
 }
 
+// Tests that using @builtin(clip_distances) will decrease the maximum location of the inter-stage
+// shader variable, while the PointList primitive topology doesn't affect the maximum location of
+// the inter-stage shader variable.
+TEST_F(ClipDistancesValidationTest, ClipDistancesAgainstMaxInterStageLocation) {
+    constexpr std::array<wgpu::PrimitiveTopology, 2> kPrimitives = {
+        {wgpu::PrimitiveTopology::TriangleList, wgpu::PrimitiveTopology::PointList}};
+    for (wgpu::PrimitiveTopology primitive : kPrimitives) {
+        for (uint32_t clipDistancesSize = 1; clipDistancesSize <= 8; ++clipDistancesSize) {
+            for (uint32_t location = kMaxInterStageShaderVariables - 3u;
+                 location < kMaxInterStageShaderVariables; ++location) {
+                std::stringstream stream;
+                stream << R"(
+                    enable clip_distances;
+                    struct VertexOut {
+                        @location()"
+                       << location << ") color : vec4f,\n"
+                       << R"(
+                        @builtin(clip_distances) clipDistances : array<f32, )"
+                       << clipDistancesSize << ">,\n"
+                       << R"(
+                        @builtin(position) pos : vec4f,
+                     }
+                     struct FragmentIn {
+                         @location()"
+                       << location << ") color : vec4f,\n"
+                       << R"(
+                         @builtin(position) pos : vec4f,
+                     }
+                     @vertex
+                     fn vsMain() -> VertexOut {
+                         var vout : VertexOut;
+                         return vout;
+                     }
+                     @fragment
+                     fn fsMain(fragIn : FragmentIn) -> @location(0) vec4f {
+                         return fragIn.pos;
+                     })";
+
+                wgpu::ShaderModule shaderModule = utils::CreateShaderModule(device, stream.str());
+                utils::ComboRenderPipelineDescriptor descriptor;
+                descriptor.vertex.module = shaderModule;
+                descriptor.cFragment.module = shaderModule;
+                descriptor.primitive.topology = primitive;
+
+                uint32_t slotsForClipDistances = Align(clipDistancesSize, 4u) / 4;
+                uint32_t maxLocation = kMaxInterStageShaderVariables - 1 - slotsForClipDistances;
+                if (location > maxLocation) {
+                    ASSERT_DEVICE_ERROR(device.CreateRenderPipeline(&descriptor));
+                } else {
+                    device.CreateRenderPipeline(&descriptor);
+                }
+            }
+        }
+    }
+}
+
 }  // anonymous namespace
 }  // namespace dawn