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 <cwallez@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Commit-Queue: Kai Ninomiya <kainino@chromium.org>
diff --git a/src/dawn/native/ShaderModule.cpp b/src/dawn/native/ShaderModule.cpp
index 73aa23e..31b7331 100644
--- a/src/dawn/native/ShaderModule.cpp
+++ b/src/dawn/native/ShaderModule.cpp
@@ -697,7 +697,12 @@
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 @@
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 @@
}
// 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();
diff --git a/src/dawn/tests/end2end/MaxLimitTests.cpp b/src/dawn/tests/end2end/MaxLimitTests.cpp
index efcb178..01de04d 100644
--- a/src/dawn/tests/end2end/MaxLimitTests.cpp
+++ b/src/dawn/tests/end2end/MaxLimitTests.cpp
@@ -886,8 +886,7 @@
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 @@
++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 @@
}
}
+// 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 @@
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) {
diff --git a/src/dawn/tests/unittests/validation/RenderPipelineValidationTests.cpp b/src/dawn/tests/unittests/validation/RenderPipelineValidationTests.cpp
index 5fdf41a..67b37f0 100644
--- a/src/dawn/tests/unittests/validation/RenderPipelineValidationTests.cpp
+++ b/src/dawn/tests/unittests/validation/RenderPipelineValidationTests.cpp
@@ -3409,5 +3409,61 @@
}
}
+// 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
