Disallow storage buffer binding in vertex shader
Writable storage buffer in vertex shader is an optional feature.
It is not supported in many devices/OSes. WebGPU doesn't support
writable storage buffer in vertex shader. This change generates an
error for storage buffer binding for vertex shader stage, in order
to disallow writable storage buffer in vertex shader.
This change also adds a validation test and revises existing
end2end tests and validation tests accordingly.
BUG=dawn:180
Change-Id: I9def918d19f65aab45a31acb985c1a0a09c97ca8
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/14521
Commit-Queue: Yunchao He <yunchao.he@intel.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>
diff --git a/src/dawn_native/BindGroupLayout.cpp b/src/dawn_native/BindGroupLayout.cpp
index 3d0ecd2..0ab3f1c 100644
--- a/src/dawn_native/BindGroupLayout.cpp
+++ b/src/dawn_native/BindGroupLayout.cpp
@@ -49,6 +49,12 @@
return DAWN_VALIDATION_ERROR("some binding index was specified more than once");
}
+ if (binding.type == wgpu::BindingType::StorageBuffer &&
+ (binding.visibility & wgpu::ShaderStage::Vertex) != 0) {
+ return DAWN_VALIDATION_ERROR(
+ "storage buffer binding is not supported in vertex shader");
+ }
+
switch (binding.type) {
case wgpu::BindingType::UniformBuffer:
if (binding.hasDynamicOffset) {
diff --git a/src/dawn_native/PipelineLayout.cpp b/src/dawn_native/PipelineLayout.cpp
index b76e553..a810f37 100644
--- a/src/dawn_native/PipelineLayout.cpp
+++ b/src/dawn_native/PipelineLayout.cpp
@@ -132,9 +132,14 @@
BindGroupLayoutBinding bindingSlot;
bindingSlot.binding = binding;
- bindingSlot.visibility = wgpu::ShaderStage::Vertex |
- wgpu::ShaderStage::Fragment |
- wgpu::ShaderStage::Compute;
+ if (bindingInfo.type == wgpu::BindingType::StorageBuffer) {
+ bindingSlot.visibility =
+ wgpu::ShaderStage::Fragment | wgpu::ShaderStage::Compute;
+ } else {
+ bindingSlot.visibility = wgpu::ShaderStage::Vertex |
+ wgpu::ShaderStage::Fragment |
+ wgpu::ShaderStage::Compute;
+ }
bindingSlot.type = bindingInfo.type;
bindingSlot.hasDynamicOffset = false;
bindingSlot.multisampled = bindingInfo.multisampled;
diff --git a/src/tests/end2end/GpuMemorySynchronizationTests.cpp b/src/tests/end2end/GpuMemorySynchronizationTests.cpp
index 2917ee8..995d272 100644
--- a/src/tests/end2end/GpuMemorySynchronizationTests.cpp
+++ b/src/tests/end2end/GpuMemorySynchronizationTests.cpp
@@ -179,7 +179,7 @@
pass0.Draw(1, 1, 0, 0);
pass0.EndPass();
- // Read that data in render pass.
+ // Read that data in compute pass.
wgpu::ComputePassEncoder pass1 = encoder.BeginComputePass();
pass1.SetPipeline(compute);
pass1.SetBindGroup(0, bindGroup1);
@@ -210,7 +210,7 @@
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
- // Write data into a storage buffer in render pass.
+ // Write data into a storage buffer in compute pass.
wgpu::ComputePassEncoder pass0 = encoder.BeginComputePass();
pass0.SetPipeline(compute);
pass0.SetBindGroup(0, bindGroup1);
diff --git a/src/tests/end2end/OpArrayLengthTests.cpp b/src/tests/end2end/OpArrayLengthTests.cpp
index 44283c6..a20f9e2 100644
--- a/src/tests/end2end/OpArrayLengthTests.cpp
+++ b/src/tests/end2end/OpArrayLengthTests.cpp
@@ -38,10 +38,10 @@
// Put them all in a bind group for tests to bind them easily.
wgpu::ShaderStage kAllStages =
wgpu::ShaderStage::Fragment | wgpu::ShaderStage::Vertex | wgpu::ShaderStage::Compute;
- mBindGroupLayout =
- utils::MakeBindGroupLayout(device, {{0, kAllStages, wgpu::BindingType::StorageBuffer},
- {1, kAllStages, wgpu::BindingType::StorageBuffer},
- {2, kAllStages, wgpu::BindingType::StorageBuffer}});
+ mBindGroupLayout = utils::MakeBindGroupLayout(
+ device, {{0, kAllStages, wgpu::BindingType::ReadonlyStorageBuffer},
+ {1, kAllStages, wgpu::BindingType::ReadonlyStorageBuffer},
+ {2, kAllStages, wgpu::BindingType::ReadonlyStorageBuffer}});
mBindGroup = utils::MakeBindGroup(device, mBindGroupLayout,
{
@@ -54,19 +54,19 @@
// 0.
mShaderInterface = R"(
// The length should be 1 because the buffer is 4-byte long.
- layout(std430, set = 0, binding = 0) buffer Buffer1 {
+ layout(std430, set = 0, binding = 0) readonly buffer Buffer1 {
float data[];
} buffer1;
// The length should be 64 because the buffer is 256 bytes long.
- layout(std430, set = 0, binding = 1) buffer Buffer2 {
+ layout(std430, set = 0, binding = 1) readonly buffer Buffer2 {
float data[];
} buffer2;
// The length should be (512 - 16*4) / 8 = 56 because the buffer is 512 bytes long
// and the structure is 8 bytes big.
struct Buffer3Data {float a; int b;};
- layout(std430, set = 0, binding = 2) buffer Buffer3 {
+ layout(std430, set = 0, binding = 2) readonly buffer Buffer3 {
mat4 garbage;
Buffer3Data data[];
} buffer3;
diff --git a/src/tests/unittests/validation/BindGroupValidationTests.cpp b/src/tests/unittests/validation/BindGroupValidationTests.cpp
index 1e7ce5e..de5df85 100644
--- a/src/tests/unittests/validation/BindGroupValidationTests.cpp
+++ b/src/tests/unittests/validation/BindGroupValidationTests.cpp
@@ -493,6 +493,22 @@
}
};
+// Tests setting storage buffer and readonly storage buffer bindings in vertex and fragment shader.
+TEST_F(BindGroupLayoutValidationTest, BindGroupLayoutStorageBindingsInVertexShader) {
+ // Checks that storage buffer binding is not supported in vertex shader.
+ ASSERT_DEVICE_ERROR(utils::MakeBindGroupLayout(
+ device, {{0, wgpu::ShaderStage::Vertex, wgpu::BindingType::StorageBuffer}}));
+
+ utils::MakeBindGroupLayout(
+ device, {{0, wgpu::ShaderStage::Vertex, wgpu::BindingType::ReadonlyStorageBuffer}});
+
+ utils::MakeBindGroupLayout(
+ device, {{0, wgpu::ShaderStage::Fragment, wgpu::BindingType::StorageBuffer}});
+
+ utils::MakeBindGroupLayout(
+ device, {{0, wgpu::ShaderStage::Fragment, wgpu::BindingType::ReadonlyStorageBuffer}});
+}
+
// Tests setting OOB checks for kMaxBindingsPerGroup in bind group layouts.
TEST_F(BindGroupLayoutValidationTest, BindGroupLayoutBindingOOB) {
// Checks that kMaxBindingsPerGroup - 1 is valid.
diff --git a/src/tests/unittests/validation/CommandBufferValidationTests.cpp b/src/tests/unittests/validation/CommandBufferValidationTests.cpp
index b5be61a..7c36afe 100644
--- a/src/tests/unittests/validation/CommandBufferValidationTests.cpp
+++ b/src/tests/unittests/validation/CommandBufferValidationTests.cpp
@@ -210,7 +210,7 @@
// Create the bind group to use the buffer as storage
wgpu::BindGroupLayout bgl = utils::MakeBindGroupLayout(
- device, {{0, wgpu::ShaderStage::Vertex, wgpu::BindingType::StorageBuffer}});
+ device, {{0, wgpu::ShaderStage::Fragment, wgpu::BindingType::StorageBuffer}});
wgpu::BindGroup bg = utils::MakeBindGroup(device, bgl, {{0, buffer, 0, 4}});
// Use the buffer as both index and storage in the same pass
@@ -258,8 +258,8 @@
// Create the bind group to use the buffer as storage
wgpu::BindGroupLayout bgl = utils::MakeBindGroupLayout(
- device, {{0, wgpu::ShaderStage::Vertex, wgpu::BindingType::StorageBuffer},
- {1, wgpu::ShaderStage::Vertex, wgpu::BindingType::ReadonlyStorageBuffer}});
+ device, {{0, wgpu::ShaderStage::Fragment, wgpu::BindingType::StorageBuffer},
+ {1, wgpu::ShaderStage::Fragment, wgpu::BindingType::ReadonlyStorageBuffer}});
wgpu::BindGroup bg =
utils::MakeBindGroup(device, bgl, {{0, buffer, 0, 4}, {1, buffer, 256, 4}});
diff --git a/src/tests/unittests/validation/GetBindGroupLayoutValidationTests.cpp b/src/tests/unittests/validation/GetBindGroupLayoutValidationTests.cpp
index 18673ea..40a63b3 100644
--- a/src/tests/unittests/validation/GetBindGroupLayoutValidationTests.cpp
+++ b/src/tests/unittests/validation/GetBindGroupLayoutValidationTests.cpp
@@ -22,14 +22,14 @@
static constexpr wgpu::ShaderStage kVisibilityAll =
wgpu::ShaderStage::Compute | wgpu::ShaderStage::Fragment | wgpu::ShaderStage::Vertex;
- wgpu::RenderPipeline RenderPipelineFromVertexShader(const char* shader) {
+ wgpu::RenderPipeline RenderPipelineFromFragmentShader(const char* shader) {
wgpu::ShaderModule vsModule =
- utils::CreateShaderModule(device, utils::SingleShaderStage::Vertex, shader);
- wgpu::ShaderModule fsModule =
- utils::CreateShaderModule(device, utils::SingleShaderStage::Fragment, R"(
+ utils::CreateShaderModule(device, utils::SingleShaderStage::Vertex, R"(
#version 450
void main() {
})");
+ wgpu::ShaderModule fsModule =
+ utils::CreateShaderModule(device, utils::SingleShaderStage::Fragment, shader);
utils::ComboRenderPipelineDescriptor descriptor(device);
descriptor.layout = nullptr;
@@ -93,7 +93,7 @@
// - shader stage visibility is All
// - dynamic offsets is false
TEST_F(GetBindGroupLayoutTests, DefaultShaderStageAndDynamicOffsets) {
- wgpu::RenderPipeline pipeline = RenderPipelineFromVertexShader(R"(
+ wgpu::RenderPipeline pipeline = RenderPipelineFromFragmentShader(R"(
#version 450
layout(set = 0, binding = 0) uniform UniformBuffer {
vec4 pos;
@@ -167,7 +167,6 @@
TEST_F(GetBindGroupLayoutTests, BindingType) {
wgpu::BindGroupLayoutBinding binding = {};
binding.binding = 0;
- binding.visibility = kVisibilityAll;
binding.hasDynamicOffset = false;
binding.multisampled = false;
@@ -176,8 +175,23 @@
desc.bindings = &binding;
{
+ // Storage buffer binding is not supported in vertex shader.
+ binding.visibility = wgpu::ShaderStage::Compute | wgpu::ShaderStage::Fragment;
+ binding.type = wgpu::BindingType::StorageBuffer;
+ wgpu::RenderPipeline pipeline = RenderPipelineFromFragmentShader(R"(
+ #version 450
+ layout(set = 0, binding = 0) buffer Storage {
+ vec4 pos;
+ };
+
+ void main() {})");
+ EXPECT_EQ(device.CreateBindGroupLayout(&desc).Get(), pipeline.GetBindGroupLayout(0).Get());
+ }
+
+ binding.visibility = kVisibilityAll;
+ {
binding.type = wgpu::BindingType::UniformBuffer;
- wgpu::RenderPipeline pipeline = RenderPipelineFromVertexShader(R"(
+ wgpu::RenderPipeline pipeline = RenderPipelineFromFragmentShader(R"(
#version 450
layout(set = 0, binding = 0) uniform Buffer {
vec4 pos;
@@ -188,20 +202,8 @@
}
{
- binding.type = wgpu::BindingType::StorageBuffer;
- wgpu::RenderPipeline pipeline = RenderPipelineFromVertexShader(R"(
- #version 450
- layout(set = 0, binding = 0) buffer Storage {
- vec4 pos;
- };
-
- void main() {})");
- EXPECT_EQ(device.CreateBindGroupLayout(&desc).Get(), pipeline.GetBindGroupLayout(0).Get());
- }
-
- {
binding.type = wgpu::BindingType::ReadonlyStorageBuffer;
- wgpu::RenderPipeline pipeline = RenderPipelineFromVertexShader(R"(
+ wgpu::RenderPipeline pipeline = RenderPipelineFromFragmentShader(R"(
#version 450
layout(set = 0, binding = 0) readonly buffer Storage {
vec4 pos;
@@ -213,7 +215,7 @@
{
binding.type = wgpu::BindingType::SampledTexture;
- wgpu::RenderPipeline pipeline = RenderPipelineFromVertexShader(R"(
+ wgpu::RenderPipeline pipeline = RenderPipelineFromFragmentShader(R"(
#version 450
layout(set = 0, binding = 0) uniform texture2D tex;
@@ -223,7 +225,7 @@
{
binding.type = wgpu::BindingType::Sampler;
- wgpu::RenderPipeline pipeline = RenderPipelineFromVertexShader(R"(
+ wgpu::RenderPipeline pipeline = RenderPipelineFromFragmentShader(R"(
#version 450
layout(set = 0, binding = 0) uniform sampler samp;
@@ -246,7 +248,7 @@
{
binding.multisampled = false;
- wgpu::RenderPipeline pipeline = RenderPipelineFromVertexShader(R"(
+ wgpu::RenderPipeline pipeline = RenderPipelineFromFragmentShader(R"(
#version 450
layout(set = 0, binding = 0) uniform texture2D tex;
@@ -258,7 +260,7 @@
GTEST_SKIP() << "Multisampling unimplemented";
{
binding.multisampled = true;
- wgpu::RenderPipeline pipeline = RenderPipelineFromVertexShader(R"(
+ wgpu::RenderPipeline pipeline = RenderPipelineFromFragmentShader(R"(
#version 450
layout(set = 0, binding = 0) uniform texture2DMS tex;
@@ -282,7 +284,7 @@
{
binding.textureDimension = wgpu::TextureViewDimension::e1D;
- wgpu::RenderPipeline pipeline = RenderPipelineFromVertexShader(R"(
+ wgpu::RenderPipeline pipeline = RenderPipelineFromFragmentShader(R"(
#version 450
layout(set = 0, binding = 0) uniform texture1D tex;
@@ -292,7 +294,7 @@
{
binding.textureDimension = wgpu::TextureViewDimension::e2D;
- wgpu::RenderPipeline pipeline = RenderPipelineFromVertexShader(R"(
+ wgpu::RenderPipeline pipeline = RenderPipelineFromFragmentShader(R"(
#version 450
layout(set = 0, binding = 0) uniform texture2D tex;
@@ -302,7 +304,7 @@
{
binding.textureDimension = wgpu::TextureViewDimension::e2DArray;
- wgpu::RenderPipeline pipeline = RenderPipelineFromVertexShader(R"(
+ wgpu::RenderPipeline pipeline = RenderPipelineFromFragmentShader(R"(
#version 450
layout(set = 0, binding = 0) uniform texture2DArray tex;
@@ -312,7 +314,7 @@
{
binding.textureDimension = wgpu::TextureViewDimension::e3D;
- wgpu::RenderPipeline pipeline = RenderPipelineFromVertexShader(R"(
+ wgpu::RenderPipeline pipeline = RenderPipelineFromFragmentShader(R"(
#version 450
layout(set = 0, binding = 0) uniform texture3D tex;
@@ -322,7 +324,7 @@
{
binding.textureDimension = wgpu::TextureViewDimension::Cube;
- wgpu::RenderPipeline pipeline = RenderPipelineFromVertexShader(R"(
+ wgpu::RenderPipeline pipeline = RenderPipelineFromFragmentShader(R"(
#version 450
layout(set = 0, binding = 0) uniform textureCube tex;
@@ -332,7 +334,7 @@
{
binding.textureDimension = wgpu::TextureViewDimension::CubeArray;
- wgpu::RenderPipeline pipeline = RenderPipelineFromVertexShader(R"(
+ wgpu::RenderPipeline pipeline = RenderPipelineFromFragmentShader(R"(
#version 450
layout(set = 0, binding = 0) uniform textureCubeArray tex;
@@ -356,7 +358,7 @@
{
binding.textureComponentType = wgpu::TextureComponentType::Float;
- wgpu::RenderPipeline pipeline = RenderPipelineFromVertexShader(R"(
+ wgpu::RenderPipeline pipeline = RenderPipelineFromFragmentShader(R"(
#version 450
layout(set = 0, binding = 0) uniform texture2D tex;
@@ -366,7 +368,7 @@
{
binding.textureComponentType = wgpu::TextureComponentType::Sint;
- wgpu::RenderPipeline pipeline = RenderPipelineFromVertexShader(R"(
+ wgpu::RenderPipeline pipeline = RenderPipelineFromFragmentShader(R"(
#version 450
layout(set = 0, binding = 0) uniform itexture2D tex;
@@ -376,7 +378,7 @@
{
binding.textureComponentType = wgpu::TextureComponentType::Uint;
- wgpu::RenderPipeline pipeline = RenderPipelineFromVertexShader(R"(
+ wgpu::RenderPipeline pipeline = RenderPipelineFromFragmentShader(R"(
#version 450
layout(set = 0, binding = 0) uniform utexture2D tex;
@@ -399,7 +401,7 @@
{
binding.binding = 0;
- wgpu::RenderPipeline pipeline = RenderPipelineFromVertexShader(R"(
+ wgpu::RenderPipeline pipeline = RenderPipelineFromFragmentShader(R"(
#version 450
layout(set = 0, binding = 0) uniform Buffer {
vec4 pos;
@@ -411,7 +413,7 @@
{
binding.binding = 1;
- wgpu::RenderPipeline pipeline = RenderPipelineFromVertexShader(R"(
+ wgpu::RenderPipeline pipeline = RenderPipelineFromFragmentShader(R"(
#version 450
layout(set = 0, binding = 1) uniform Buffer {
vec4 pos;
@@ -423,7 +425,7 @@
{
binding.binding = 2;
- wgpu::RenderPipeline pipeline = RenderPipelineFromVertexShader(R"(
+ wgpu::RenderPipeline pipeline = RenderPipelineFromFragmentShader(R"(
#version 450
layout(set = 0, binding = 1) uniform Buffer {
vec4 pos;
@@ -571,7 +573,7 @@
// Test it is an error to query an out of range bind group layout.
TEST_F(GetBindGroupLayoutTests, OutOfRangeIndex) {
- ASSERT_DEVICE_ERROR(RenderPipelineFromVertexShader(R"(
+ ASSERT_DEVICE_ERROR(RenderPipelineFromFragmentShader(R"(
#version 450
layout(set = 0, binding = 0) uniform Buffer1 {
vec4 pos1;
@@ -579,7 +581,7 @@
void main() {})")
.GetBindGroupLayout(kMaxBindGroups));
- ASSERT_DEVICE_ERROR(RenderPipelineFromVertexShader(R"(
+ ASSERT_DEVICE_ERROR(RenderPipelineFromFragmentShader(R"(
#version 450
layout(set = 0, binding = 0) uniform Buffer1 {
vec4 pos1;
@@ -590,7 +592,7 @@
// Test that unused indices return the empty bind group layout.
TEST_F(GetBindGroupLayoutTests, UnusedIndex) {
- wgpu::RenderPipeline pipeline = RenderPipelineFromVertexShader(R"(
+ wgpu::RenderPipeline pipeline = RenderPipelineFromFragmentShader(R"(
#version 450
layout(set = 0, binding = 0) uniform Buffer1 {
vec4 pos1;
