Read and write usage in compute pass is valid
Resource usage tracking in compute pass is per dispatch. So readable
and writeable usages in pass granularity may be valid.
This patch also removes ComputePassValidationTests.cpp because it
is duplicated with ResourceUsageTrackingTests.cpp. The former actually
contains resource usage tracking tests only, and the latter is also
for the same purpose and it is much more comprehensive.
Bug: dawn:358
Change-Id: I53f8906660b348eeff4f2a061e3b829d1c2ceab8
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/20122
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/CommandValidation.cpp b/src/dawn_native/CommandValidation.cpp
index 7fc8cae..818b503 100644
--- a/src/dawn_native/CommandValidation.cpp
+++ b/src/dawn_native/CommandValidation.cpp
@@ -299,7 +299,7 @@
bool readOnly = (usage & kReadOnlyBufferUsages) == usage;
bool singleUse = wgpu::HasZeroOrOneBits(usage);
- if (!readOnly && !singleUse) {
+ if (pass.passType == PassType::Render && !readOnly && !singleUse) {
return DAWN_VALIDATION_ERROR(
"Buffer used as writable usage and another usage in pass");
}
diff --git a/src/tests/BUILD.gn b/src/tests/BUILD.gn
index 7e41d40..aac3861 100644
--- a/src/tests/BUILD.gn
+++ b/src/tests/BUILD.gn
@@ -151,7 +151,6 @@
"unittests/validation/BufferValidationTests.cpp",
"unittests/validation/CommandBufferValidationTests.cpp",
"unittests/validation/ComputeIndirectValidationTests.cpp",
- "unittests/validation/ComputePassValidationTests.cpp",
"unittests/validation/ComputeValidationTests.cpp",
"unittests/validation/CopyCommandsValidationTests.cpp",
"unittests/validation/DebugMarkerValidationTests.cpp",
diff --git a/src/tests/unittests/validation/ComputePassValidationTests.cpp b/src/tests/unittests/validation/ComputePassValidationTests.cpp
deleted file mode 100644
index 851c519..0000000
--- a/src/tests/unittests/validation/ComputePassValidationTests.cpp
+++ /dev/null
@@ -1,147 +0,0 @@
-// Copyright 2019 The Dawn Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-#include "tests/unittests/validation/ValidationTest.h"
-
-#include "utils/WGPUHelpers.h"
-
-class ComputePassValidationTest : public ValidationTest {};
-
-// Test that it is invalid to use a buffer with both read and write usages in a compute pass.
-TEST_F(ComputePassValidationTest, ReadWriteUsage) {
- wgpu::BufferDescriptor bufferDesc = {};
- bufferDesc.usage = wgpu::BufferUsage::Storage | wgpu::BufferUsage::Uniform;
- bufferDesc.size = 4;
- wgpu::Buffer buffer = device.CreateBuffer(&bufferDesc);
-
- wgpu::ShaderModule module =
- utils::CreateShaderModule(device, utils::SingleShaderStage::Compute, R"(
- #version 450
- layout(std430, set = 0, binding = 0) buffer BufA { uint bufA; };
- layout(std140, set = 0, binding = 1) uniform BufB { uint bufB; };
- void main() {}
- )");
-
- wgpu::BindGroupLayout bgl = utils::MakeBindGroupLayout(
- device, {{0, wgpu::ShaderStage::Compute, wgpu::BindingType::StorageBuffer},
- {1, wgpu::ShaderStage::Compute, wgpu::BindingType::UniformBuffer}});
-
- wgpu::BindGroup bindGroup = utils::MakeBindGroup(device, bgl,
- {
- {0, buffer, 0, 4},
- {1, buffer, 0, 4},
- });
-
- wgpu::PipelineLayout layout = utils::MakeBasicPipelineLayout(device, &bgl);
-
- wgpu::ComputePipelineDescriptor pipelineDesc = {};
- pipelineDesc.layout = layout;
- pipelineDesc.computeStage.module = module;
- pipelineDesc.computeStage.entryPoint = "main";
- wgpu::ComputePipeline pipeline = device.CreateComputePipeline(&pipelineDesc);
-
- wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
- wgpu::ComputePassEncoder pass = encoder.BeginComputePass();
- pass.SetPipeline(pipeline);
- pass.SetBindGroup(0, bindGroup);
-
- pass.Dispatch(1);
- pass.Dispatch(1);
-
- pass.EndPass();
- ASSERT_DEVICE_ERROR(encoder.Finish());
-}
-
-// Test that it is valid to use a buffer with a single write usage multiple times in a compute pass.
-TEST_F(ComputePassValidationTest, MultipleWrites) {
- wgpu::BufferDescriptor bufferDesc = {};
- bufferDesc.usage = wgpu::BufferUsage::Storage;
- bufferDesc.size = 4;
- wgpu::Buffer buffer = device.CreateBuffer(&bufferDesc);
-
- wgpu::ShaderModule module =
- utils::CreateShaderModule(device, utils::SingleShaderStage::Compute, R"(
- #version 450
- layout(std430, set = 0, binding = 0) buffer Buf { uint buf; };
- void main() {}
- )");
-
- wgpu::BindGroupLayout bgl = utils::MakeBindGroupLayout(
- device, {{0, wgpu::ShaderStage::Compute, wgpu::BindingType::StorageBuffer}});
-
- wgpu::BindGroup bindGroup = utils::MakeBindGroup(device, bgl, {{0, buffer, 0, 4}});
-
- wgpu::PipelineLayout layout = utils::MakeBasicPipelineLayout(device, &bgl);
-
- wgpu::ComputePipelineDescriptor pipelineDesc = {};
- pipelineDesc.layout = layout;
- pipelineDesc.computeStage.module = module;
- pipelineDesc.computeStage.entryPoint = "main";
- wgpu::ComputePipeline pipeline = device.CreateComputePipeline(&pipelineDesc);
-
- wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
- wgpu::ComputePassEncoder pass = encoder.BeginComputePass();
- pass.SetPipeline(pipeline);
- pass.SetBindGroup(0, bindGroup);
-
- pass.Dispatch(1);
- pass.Dispatch(1);
-
- pass.EndPass();
- encoder.Finish();
-}
-
-// Test that it is valid to use a buffer with a single write usage multiple times in a compute pass,
-// even if the buffer is referenced in separate bind groups.
-TEST_F(ComputePassValidationTest, MultipleWritesSeparateBindGroups) {
- wgpu::BufferDescriptor bufferDesc = {};
- bufferDesc.usage = wgpu::BufferUsage::Storage;
- bufferDesc.size = 4;
- wgpu::Buffer buffer = device.CreateBuffer(&bufferDesc);
-
- wgpu::ShaderModule module =
- utils::CreateShaderModule(device, utils::SingleShaderStage::Compute, R"(
- #version 450
- #define kNumValues 100
- layout(std430, set = 0, binding = 0) buffer Buf { uint buf; };
- void main() {}
- )");
-
- wgpu::BindGroupLayout bgl = utils::MakeBindGroupLayout(
- device, {{0, wgpu::ShaderStage::Compute, wgpu::BindingType::StorageBuffer}});
-
- wgpu::BindGroup bindGroupA = utils::MakeBindGroup(device, bgl, {{0, buffer, 0, 4}});
- wgpu::BindGroup bindGroupB = utils::MakeBindGroup(device, bgl, {{0, buffer, 0, 4}});
-
- wgpu::PipelineLayout layout = utils::MakeBasicPipelineLayout(device, &bgl);
-
- wgpu::ComputePipelineDescriptor pipelineDesc = {};
- pipelineDesc.layout = layout;
- pipelineDesc.computeStage.module = module;
- pipelineDesc.computeStage.entryPoint = "main";
- wgpu::ComputePipeline pipeline = device.CreateComputePipeline(&pipelineDesc);
-
- wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
- wgpu::ComputePassEncoder pass = encoder.BeginComputePass();
- pass.SetPipeline(pipeline);
-
- pass.SetBindGroup(0, bindGroupA);
- pass.Dispatch(1);
-
- pass.SetBindGroup(0, bindGroupB);
- pass.Dispatch(1);
-
- pass.EndPass();
- encoder.Finish();
-}
diff --git a/src/tests/unittests/validation/ResourceUsageTrackingTests.cpp b/src/tests/unittests/validation/ResourceUsageTrackingTests.cpp
index 741bb6b..04573ac 100644
--- a/src/tests/unittests/validation/ResourceUsageTrackingTests.cpp
+++ b/src/tests/unittests/validation/ResourceUsageTrackingTests.cpp
@@ -82,7 +82,8 @@
}
}
- // Test that using the same buffer as both readable and writable in the same pass is disallowed
+ // Test that it is invalid to use the same buffer as both readable and writable in the same
+ // render pass. But it is valid in compute pass.
TEST_F(ResourceUsageTrackingTest, BufferWithReadAndWriteUsage) {
// test render pass for index buffer and storage buffer
{
@@ -94,7 +95,7 @@
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 render pass
+ // It is invalid to use the buffer as both index and storage in render pass
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
DummyRenderPass dummyRenderPass(device);
wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&dummyRenderPass);
@@ -116,12 +117,12 @@
wgpu::BindGroup bg =
utils::MakeBindGroup(device, bgl, {{0, buffer, 0, 4}, {1, buffer, 256, 4}});
- // Use the buffer as both storage and readonly storage in compute pass
+ // It is valid to use the buffer as both storage and readonly storage in compute pass.
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
wgpu::ComputePassEncoder pass = encoder.BeginComputePass();
pass.SetBindGroup(0, bg);
pass.EndPass();
- ASSERT_DEVICE_ERROR(encoder.Finish());
+ encoder.Finish();
}
}
@@ -455,12 +456,13 @@
{1, wgpu::ShaderStage::None, wgpu::BindingType::StorageBuffer}});
wgpu::BindGroup bg = utils::MakeBindGroup(device, bgl, {{0, buffer}, {1, buffer}});
- // These two bindings are invisible in compute pass. But we still track these bindings.
+ // These two bindings are invisible in compute pass. We still track these invisible
+ // bindings, but read and write usages in one compute pass is allowed.
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
wgpu::ComputePassEncoder pass = encoder.BeginComputePass();
pass.SetBindGroup(0, bg);
pass.EndPass();
- ASSERT_DEVICE_ERROR(encoder.Finish());
+ encoder.Finish();
}
}
@@ -499,13 +501,13 @@
wgpu::BindGroup bg = utils::MakeBindGroup(device, bgl, {{0, buffer}, {1, buffer}});
// Buffer usage in compute stage conflicts with buffer usage in fragment stage. And
- // binding for fragment stage is not visible in compute pass. But we still track this
- // binding.
+ // binding for fragment stage is not visible in compute pass. We still track this
+ // invisible binding, but read and write usages in one compute pass is allowed.
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
wgpu::ComputePassEncoder pass = encoder.BeginComputePass();
pass.SetBindGroup(0, bg);
pass.EndPass();
- ASSERT_DEVICE_ERROR(encoder.Finish());
+ encoder.Finish();
}
}
@@ -630,7 +632,8 @@
wgpu::ComputePipeline cp = device.CreateComputePipeline(&pipelineDescriptor);
// Resource in bg1 conflicts with resources used in bg0. However, the binding in bg1 is
- // not used in pipeline. But we still track this binding.
+ // not used in pipeline. But we still track this binding and read/write usage in one
+ // dispatch is not allowed.
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
wgpu::ComputePassEncoder pass = encoder.BeginComputePass();
pass.SetBindGroup(0, bg0);
@@ -638,7 +641,9 @@
pass.SetPipeline(cp);
pass.Dispatch(1);
pass.EndPass();
- ASSERT_DEVICE_ERROR(encoder.Finish());
+ encoder.Finish();
+ // TODO (yunchao.he@intel.com): add resource tracking per dispatch for compute pass
+ // ASSERT_DEVICE_ERROR(encoder.Finish());
}
}
