Implement readonly storage buffer - validation at shader side
This patch adds validation code for shader side for readonly storage
buffer. It also adds unit tests for validation.
BUG=dawn:180
Change-Id: Ib5789381d41d77867dd6e6aa1f1ccbc8fa43a382
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/12941
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Yunchao He <yunchao.he@intel.com>
diff --git a/src/dawn_native/ShaderModule.cpp b/src/dawn_native/ShaderModule.cpp
index acb591f..dbe2372 100644
--- a/src/dawn_native/ShaderModule.cpp
+++ b/src/dawn_native/ShaderModule.cpp
@@ -154,12 +154,27 @@
info.used = true;
info.id = resource.id;
info.base_type_id = resource.base_type_id;
- info.type = bindingType;
- if (info.type == wgpu::BindingType::SampledTexture) {
- spirv_cross::SPIRType::BaseType textureComponentType =
- compiler.get_type(compiler.get_type(info.base_type_id).image.type).basetype;
- info.textureComponentType =
- SpirvCrossBaseTypeToFormatType(textureComponentType);
+ switch (bindingType) {
+ case wgpu::BindingType::SampledTexture: {
+ spirv_cross::SPIRType::BaseType textureComponentType =
+ compiler.get_type(compiler.get_type(info.base_type_id).image.type)
+ .basetype;
+ info.textureComponentType =
+ SpirvCrossBaseTypeToFormatType(textureComponentType);
+ info.type = bindingType;
+ } break;
+ case wgpu::BindingType::StorageBuffer: {
+ // Differentiate between readonly storage bindings and writable ones based
+ // on the NonWritable decoration
+ spirv_cross::Bitset flags = compiler.get_buffer_block_flags(resource.id);
+ if (flags.get(spv::DecorationNonWritable)) {
+ info.type = wgpu::BindingType::ReadonlyStorageBuffer;
+ } else {
+ info.type = wgpu::BindingType::StorageBuffer;
+ }
+ } break;
+ default:
+ info.type = bindingType;
}
}
};
@@ -285,7 +300,16 @@
}
if (layoutBindingType != moduleInfo.type) {
- return false;
+ // Binding mismatch between shader and bind group is invalid. For example, a
+ // writable binding in the shader with a readonly storage buffer in the bind group
+ // layout is invalid. However, a readonly binding in the shader with a writable
+ // storage buffer in the bind group layout is valid.
+ bool validBindingConversion =
+ layoutBindingType == wgpu::BindingType::StorageBuffer &&
+ moduleInfo.type == wgpu::BindingType::ReadonlyStorageBuffer;
+ if (!validBindingConversion) {
+ return false;
+ }
}
if ((layoutInfo.visibilities[i] & StageBit(mExecutionModel)) == 0) {
diff --git a/src/tests/unittests/validation/BindGroupValidationTests.cpp b/src/tests/unittests/validation/BindGroupValidationTests.cpp
index 32b342d..1e7ce5e 100644
--- a/src/tests/unittests/validation/BindGroupValidationTests.cpp
+++ b/src/tests/unittests/validation/BindGroupValidationTests.cpp
@@ -671,7 +671,7 @@
}
}
-constexpr uint64_t kBufferSize = 2 * kMinDynamicBufferOffsetAlignment + 8;
+constexpr uint64_t kBufferSize = 3 * kMinDynamicBufferOffsetAlignment + 8;
constexpr uint32_t kBindingSize = 9;
class SetBindGroupValidationTest : public ValidationTest {
@@ -681,7 +681,9 @@
device, {{0, wgpu::ShaderStage::Compute | wgpu::ShaderStage::Fragment,
wgpu::BindingType::UniformBuffer, true},
{1, wgpu::ShaderStage::Compute | wgpu::ShaderStage::Fragment,
- wgpu::BindingType::StorageBuffer, true}});
+ wgpu::BindingType::StorageBuffer, true},
+ {2, wgpu::ShaderStage::Compute | wgpu::ShaderStage::Fragment,
+ wgpu::BindingType::ReadonlyStorageBuffer, true}});
}
wgpu::Buffer CreateBuffer(uint64_t bufferSize, wgpu::BufferUsage usage) {
@@ -710,6 +712,9 @@
layout(std140, set = 0, binding = 1) buffer SBuffer {
vec2 value2;
} sBuffer;
+ layout(std140, set = 0, binding = 2) readonly buffer RBuffer {
+ vec2 value3;
+ } rBuffer;
layout(location = 0) out vec4 fragColor;
void main() {
})");
@@ -737,9 +742,11 @@
layout(std140, set = 0, binding = 1) buffer SBuffer {
float value2;
} dst;
-
- void main() {
- })");
+ layout(std140, set = 0, binding = 2) readonly buffer RBuffer {
+ readonly float value3;
+ } rdst;
+ void main() {
+ })");
wgpu::PipelineLayout pipelineLayout =
utils::MakeBasicPipelineLayout(device, &mBindGroupLayout);
@@ -797,15 +804,17 @@
// Set up the bind group.
wgpu::Buffer uniformBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Uniform);
wgpu::Buffer storageBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Storage);
- wgpu::BindGroup bindGroup = utils::MakeBindGroup(
- device, mBindGroupLayout,
- {{0, uniformBuffer, 0, kBindingSize}, {1, storageBuffer, 0, kBindingSize}});
+ wgpu::Buffer readonlyStorageBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Storage);
+ wgpu::BindGroup bindGroup = utils::MakeBindGroup(device, mBindGroupLayout,
+ {{0, uniformBuffer, 0, kBindingSize},
+ {1, storageBuffer, 0, kBindingSize},
+ {2, readonlyStorageBuffer, 0, kBindingSize}});
- std::array<uint32_t, 2> offsets = {256, 0};
+ std::array<uint32_t, 3> offsets = {512, 256, 0};
- TestRenderPassBindGroup(bindGroup, offsets.data(), 2, true);
+ TestRenderPassBindGroup(bindGroup, offsets.data(), 3, true);
- TestComputePassBindGroup(bindGroup, offsets.data(), 2, true);
+ TestComputePassBindGroup(bindGroup, offsets.data(), 3, true);
}
// Test cases that test dynamic offsets count mismatch with bind group layout.
@@ -813,16 +822,22 @@
// Set up bind group.
wgpu::Buffer uniformBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Uniform);
wgpu::Buffer storageBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Storage);
- wgpu::BindGroup bindGroup = utils::MakeBindGroup(
- device, mBindGroupLayout,
- {{0, uniformBuffer, 0, kBindingSize}, {1, storageBuffer, 0, kBindingSize}});
+ wgpu::Buffer readonlyStorageBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Storage);
+ wgpu::BindGroup bindGroup = utils::MakeBindGroup(device, mBindGroupLayout,
+ {{0, uniformBuffer, 0, kBindingSize},
+ {1, storageBuffer, 0, kBindingSize},
+ {2, readonlyStorageBuffer, 0, kBindingSize}});
// Number of offsets mismatch.
- std::array<uint32_t, 1> mismatchOffsets = {0};
+ std::array<uint32_t, 4> mismatchOffsets = {768, 512, 256, 0};
TestRenderPassBindGroup(bindGroup, mismatchOffsets.data(), 1, false);
+ TestRenderPassBindGroup(bindGroup, mismatchOffsets.data(), 2, false);
+ TestRenderPassBindGroup(bindGroup, mismatchOffsets.data(), 4, false);
TestComputePassBindGroup(bindGroup, mismatchOffsets.data(), 1, false);
+ TestComputePassBindGroup(bindGroup, mismatchOffsets.data(), 2, false);
+ TestComputePassBindGroup(bindGroup, mismatchOffsets.data(), 4, false);
}
// Test cases that test dynamic offsets not aligned
@@ -830,16 +845,18 @@
// Set up bind group.
wgpu::Buffer uniformBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Uniform);
wgpu::Buffer storageBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Storage);
- wgpu::BindGroup bindGroup = utils::MakeBindGroup(
- device, mBindGroupLayout,
- {{0, uniformBuffer, 0, kBindingSize}, {1, storageBuffer, 0, kBindingSize}});
+ wgpu::Buffer readonlyStorageBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Storage);
+ wgpu::BindGroup bindGroup = utils::MakeBindGroup(device, mBindGroupLayout,
+ {{0, uniformBuffer, 0, kBindingSize},
+ {1, storageBuffer, 0, kBindingSize},
+ {2, readonlyStorageBuffer, 0, kBindingSize}});
// Dynamic offsets are not aligned.
- std::array<uint32_t, 2> notAlignedOffsets = {1, 2};
+ std::array<uint32_t, 3> notAlignedOffsets = {512, 128, 0};
- TestRenderPassBindGroup(bindGroup, notAlignedOffsets.data(), 2, false);
+ TestRenderPassBindGroup(bindGroup, notAlignedOffsets.data(), 3, false);
- TestComputePassBindGroup(bindGroup, notAlignedOffsets.data(), 2, false);
+ TestComputePassBindGroup(bindGroup, notAlignedOffsets.data(), 3, false);
}
// Test cases that test dynamic uniform buffer out of bound situation.
@@ -847,16 +864,18 @@
// Set up bind group.
wgpu::Buffer uniformBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Uniform);
wgpu::Buffer storageBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Storage);
- wgpu::BindGroup bindGroup = utils::MakeBindGroup(
- device, mBindGroupLayout,
- {{0, uniformBuffer, 0, kBindingSize}, {1, storageBuffer, 0, kBindingSize}});
+ wgpu::Buffer readonlyStorageBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Storage);
+ wgpu::BindGroup bindGroup = utils::MakeBindGroup(device, mBindGroupLayout,
+ {{0, uniformBuffer, 0, kBindingSize},
+ {1, storageBuffer, 0, kBindingSize},
+ {2, readonlyStorageBuffer, 0, kBindingSize}});
// Dynamic offset + offset is larger than buffer size.
- std::array<uint32_t, 2> overFlowOffsets = {1024, 0};
+ std::array<uint32_t, 3> overFlowOffsets = {1024, 256, 0};
- TestRenderPassBindGroup(bindGroup, overFlowOffsets.data(), 2, false);
+ TestRenderPassBindGroup(bindGroup, overFlowOffsets.data(), 3, false);
- TestComputePassBindGroup(bindGroup, overFlowOffsets.data(), 2, false);
+ TestComputePassBindGroup(bindGroup, overFlowOffsets.data(), 3, false);
}
// Test cases that test dynamic storage buffer out of bound situation.
@@ -864,16 +883,18 @@
// Set up bind group.
wgpu::Buffer uniformBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Uniform);
wgpu::Buffer storageBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Storage);
- wgpu::BindGroup bindGroup = utils::MakeBindGroup(
- device, mBindGroupLayout,
- {{0, uniformBuffer, 0, kBindingSize}, {1, storageBuffer, 0, kBindingSize}});
+ wgpu::Buffer readonlyStorageBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Storage);
+ wgpu::BindGroup bindGroup = utils::MakeBindGroup(device, mBindGroupLayout,
+ {{0, uniformBuffer, 0, kBindingSize},
+ {1, storageBuffer, 0, kBindingSize},
+ {2, readonlyStorageBuffer, 0, kBindingSize}});
// Dynamic offset + offset is larger than buffer size.
- std::array<uint32_t, 2> overFlowOffsets = {0, 1024};
+ std::array<uint32_t, 3> overFlowOffsets = {0, 256, 1024};
- TestRenderPassBindGroup(bindGroup, overFlowOffsets.data(), 2, false);
+ TestRenderPassBindGroup(bindGroup, overFlowOffsets.data(), 3, false);
- TestComputePassBindGroup(bindGroup, overFlowOffsets.data(), 2, false);
+ TestComputePassBindGroup(bindGroup, overFlowOffsets.data(), 3, false);
}
// Test cases that test dynamic uniform buffer out of bound situation because of binding size.
@@ -881,33 +902,37 @@
// Set up bind group, but binding size is larger than
wgpu::Buffer uniformBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Uniform);
wgpu::Buffer storageBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Storage);
- wgpu::BindGroup bindGroup = utils::MakeBindGroup(
- device, mBindGroupLayout,
- {{0, uniformBuffer, 0, kBindingSize}, {1, storageBuffer, 0, kBindingSize}});
+ wgpu::Buffer readonlyStorageBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Storage);
+ wgpu::BindGroup bindGroup = utils::MakeBindGroup(device, mBindGroupLayout,
+ {{0, uniformBuffer, 0, kBindingSize},
+ {1, storageBuffer, 0, kBindingSize},
+ {2, readonlyStorageBuffer, 0, kBindingSize}});
// Dynamic offset + offset isn't larger than buffer size.
// But with binding size, it will trigger OOB error.
- std::array<uint32_t, 2> offsets = {512, 0};
+ std::array<uint32_t, 3> offsets = {768, 256, 0};
- TestRenderPassBindGroup(bindGroup, offsets.data(), 2, false);
+ TestRenderPassBindGroup(bindGroup, offsets.data(), 3, false);
- TestComputePassBindGroup(bindGroup, offsets.data(), 2, false);
+ TestComputePassBindGroup(bindGroup, offsets.data(), 3, false);
}
// Test cases that test dynamic storage buffer out of bound situation because of binding size.
TEST_F(SetBindGroupValidationTest, BindingSizeOutOfBoundDynamicStorageBuffer) {
wgpu::Buffer uniformBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Uniform);
wgpu::Buffer storageBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Storage);
- wgpu::BindGroup bindGroup = utils::MakeBindGroup(
- device, mBindGroupLayout,
- {{0, uniformBuffer, 0, kBindingSize}, {1, storageBuffer, 0, kBindingSize}});
+ wgpu::Buffer readonlyStorageBuffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::Storage);
+ wgpu::BindGroup bindGroup = utils::MakeBindGroup(device, mBindGroupLayout,
+ {{0, uniformBuffer, 0, kBindingSize},
+ {1, storageBuffer, 0, kBindingSize},
+ {2, readonlyStorageBuffer, 0, kBindingSize}});
// Dynamic offset + offset isn't larger than buffer size.
// But with binding size, it will trigger OOB error.
- std::array<uint32_t, 2> offsets = {0, 512};
+ std::array<uint32_t, 3> offsets = {0, 256, 768};
- TestRenderPassBindGroup(bindGroup, offsets.data(), 2, false);
+ TestRenderPassBindGroup(bindGroup, offsets.data(), 3, false);
- TestComputePassBindGroup(bindGroup, offsets.data(), 2, false);
+ TestComputePassBindGroup(bindGroup, offsets.data(), 3, false);
}
// Test that an error is produced (and no ASSERTs fired) when using an error bindgroup in
@@ -1115,3 +1140,103 @@
renderPassEncoder.EndPass();
commandEncoder.Finish();
}
+
+class BindGroupLayoutCompatibilityTest : public ValidationTest {
+ public:
+ wgpu::Buffer CreateBuffer(uint64_t bufferSize, wgpu::BufferUsage usage) {
+ wgpu::BufferDescriptor bufferDescriptor;
+ bufferDescriptor.size = bufferSize;
+ bufferDescriptor.usage = usage;
+
+ return device.CreateBuffer(&bufferDescriptor);
+ }
+
+ wgpu::RenderPipeline CreateRenderPipeline(wgpu::BindGroupLayout* bindGroupLayout) {
+ wgpu::ShaderModule vsModule =
+ utils::CreateShaderModule(device, utils::SingleShaderStage::Vertex, R"(
+ #version 450
+ void main() {
+ })");
+
+ wgpu::ShaderModule fsModule =
+ utils::CreateShaderModule(device, utils::SingleShaderStage::Fragment, R"(
+ #version 450
+ layout(std140, set = 0, binding = 0) buffer SBuffer {
+ vec2 value2;
+ } sBuffer;
+ layout(std140, set = 0, binding = 1) readonly buffer RBuffer {
+ vec2 value3;
+ } rBuffer;
+ layout(location = 0) out vec4 fragColor;
+ void main() {
+ })");
+
+ utils::ComboRenderPipelineDescriptor pipelineDescriptor(device);
+ pipelineDescriptor.vertexStage.module = vsModule;
+ pipelineDescriptor.cFragmentStage.module = fsModule;
+ wgpu::PipelineLayout pipelineLayout =
+ utils::MakeBasicPipelineLayout(device, bindGroupLayout);
+ pipelineDescriptor.layout = pipelineLayout;
+ return device.CreateRenderPipeline(&pipelineDescriptor);
+ }
+
+ wgpu::ComputePipeline CreateComputePipeline(wgpu::BindGroupLayout* bindGroupLayout) {
+ wgpu::ShaderModule csModule =
+ utils::CreateShaderModule(device, utils::SingleShaderStage::Compute, R"(
+ #version 450
+ const uint kTileSize = 4;
+ const uint kInstances = 11;
+
+ layout(local_size_x = kTileSize, local_size_y = kTileSize, local_size_z = 1) in;
+ layout(std140, set = 0, binding = 0) buffer SBuffer {
+ float value2;
+ } dst;
+ layout(std140, set = 0, binding = 1) readonly buffer RBuffer {
+ readonly float value3;
+ } rdst;
+ void main() {
+ })");
+
+ wgpu::PipelineLayout pipelineLayout =
+ utils::MakeBasicPipelineLayout(device, bindGroupLayout);
+
+ wgpu::ComputePipelineDescriptor csDesc;
+ csDesc.layout = pipelineLayout;
+ csDesc.computeStage.module = csModule;
+ csDesc.computeStage.entryPoint = "main";
+
+ return device.CreateComputePipeline(&csDesc);
+ }
+};
+
+// Test cases that test bind group layout mismatch with shader. The second item in bind group layout
+// is a writable storage buffer, but the second item in shader is a readonly storage buffer. It is
+// valid.
+TEST_F(BindGroupLayoutCompatibilityTest, RWStorageInBGLWithROStorageInShader) {
+ // Set up the bind group layout.
+ wgpu::BindGroupLayout bindGroupLayout = utils::MakeBindGroupLayout(
+ device, {{0, wgpu::ShaderStage::Compute | wgpu::ShaderStage::Fragment,
+ wgpu::BindingType::StorageBuffer, true},
+ {1, wgpu::ShaderStage::Compute | wgpu::ShaderStage::Fragment,
+ wgpu::BindingType::StorageBuffer, true}});
+
+ CreateRenderPipeline(&bindGroupLayout);
+
+ CreateComputePipeline(&bindGroupLayout);
+}
+
+// Test cases that test bind group layout mismatch with shader. The first item in bind group layout
+// is a readonly storage buffer, but the first item in shader is a writable storage buffer. It is
+// invalid.
+TEST_F(BindGroupLayoutCompatibilityTest, ROStorageInBGLWithRWStorageInShader) {
+ // Set up the bind group layout.
+ wgpu::BindGroupLayout bindGroupLayout = utils::MakeBindGroupLayout(
+ device, {{0, wgpu::ShaderStage::Compute | wgpu::ShaderStage::Fragment,
+ wgpu::BindingType::ReadonlyStorageBuffer, true},
+ {1, wgpu::ShaderStage::Compute | wgpu::ShaderStage::Fragment,
+ wgpu::BindingType::ReadonlyStorageBuffer, true}});
+
+ ASSERT_DEVICE_ERROR(CreateRenderPipeline(&bindGroupLayout));
+
+ ASSERT_DEVICE_ERROR(CreateComputePipeline(&bindGroupLayout));
+}
