[dawn][native] Add Compatibility between BGLEArraySize and binding_array
This adds reflection of the array size of the WGSL bindings in
EntryPointMetadata. The compatibility check with the BGL is updated to
require the arraySize to be big enough.
The pipeline defaulting is also updated to make a BGLEArraySize when
needed.
Tests are added.
Bug: 393558555
Change-Id: I77e1ad908cd0cc48a9e54a94e2beb004f40079c8
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/236734
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Loko Kung <lokokung@google.com>
Reviewed-by: Brandon Jones <bajones@chromium.org>
diff --git a/src/dawn/native/PipelineLayout.cpp b/src/dawn/native/PipelineLayout.cpp
index 16e057a..ed24d8d 100644
--- a/src/dawn/native/PipelineLayout.cpp
+++ b/src/dawn/native/PipelineLayout.cpp
@@ -210,11 +210,16 @@
DeviceBase* device,
std::vector<StageAndDescriptor> stages,
bool allowInternalBinding) {
- using EntryMap = absl::flat_hash_map<BindingNumber, BindGroupLayoutEntry>;
+ // A structure containing all the BGLEntry and all of its (non-empty) chains so that they can
+ // be moved around together in maps / vectors etc. They are linked together just before calling
+ // CreateBGL.
+ struct EntryData : public BindGroupLayoutEntry {
+ BindGroupLayoutEntryArraySize arraySize;
+ };
+ using EntryMap = absl::flat_hash_map<BindingNumber, EntryData>;
// Merges two entries at the same location, if they are allowed to be merged.
- auto MergeEntries = [](BindGroupLayoutEntry* modifiedEntry,
- const BindGroupLayoutEntry& mergedEntry) -> MaybeError {
+ auto MergeEntries = [](EntryData* modifiedEntry, const EntryData& mergedEntry) -> MaybeError {
// Visibility is excluded because we take the OR across stages.
bool compatible =
modifiedEntry->binding == mergedEntry.binding &&
@@ -273,15 +278,19 @@
// Use the OR of all the stages at which we find this binding.
modifiedEntry->visibility |= mergedEntry.visibility;
+ // Size binding_arrays to be the maximum of the required array sizes.
+ modifiedEntry->arraySize.arraySize =
+ std::max(modifiedEntry->arraySize.arraySize, mergedEntry.arraySize.arraySize);
+
return {};
};
// Does the trivial conversions from a ShaderBindingInfo to a BindGroupLayoutEntry
auto ConvertMetadataToEntry =
[](const ShaderBindingInfo& shaderBinding,
- const ExternalTextureBindingLayout* externalTextureBindingEntry)
- -> BindGroupLayoutEntry {
- BindGroupLayoutEntry entry = {};
+ const ExternalTextureBindingLayout* externalTextureBindingEntry) -> EntryData {
+ EntryData entry = {};
+ entry.arraySize.arraySize = uint32_t(shaderBinding.arraySize);
MatchVariant(
shaderBinding.bindingInfo,
@@ -318,15 +327,25 @@
};
// Creates the BGL from the entries for a stage, checking it is valid.
- auto CreateBGL = [](DeviceBase* device, const EntryMap& entries,
+ auto CreateBGL = [](DeviceBase* device, EntryMap entries,
PipelineCompatibilityToken pipelineCompatibilityToken,
bool allowInternalBinding) -> ResultOrError<Ref<BindGroupLayoutBase>> {
+ // Put all the values from the map in a vector and link the chains
std::vector<BindGroupLayoutEntry> entryVec;
entryVec.reserve(entries.size());
for (auto& [_, entry] : entries) {
+ // Link the entries in the map so that the entry copied in the vector still keeps
+ // pointers to the chain in the map. This is valid to do because elements in the map
+ // won't move as the map isn't modified, only elements.
+ if (entry.arraySize.arraySize != 1) {
+ entry.arraySize.nextInChain = entry.nextInChain;
+ entry.nextInChain = &entry.arraySize;
+ }
+
entryVec.push_back(entry);
}
+ // Create and validate the BGL
BindGroupLayoutDescriptor desc = {};
desc.entries = entryVec.data();
desc.entryCount = entryVec.size();
@@ -367,9 +386,9 @@
for (auto [group, groupBindings] : Enumerate(metadata.bindings)) {
for (const auto& [bindingNumber, shaderBinding] : groupBindings) {
// Create the BindGroupLayoutEntry
- BindGroupLayoutEntry entry =
+ EntryData entry =
ConvertMetadataToEntry(shaderBinding, &externalTextureBindingLayout);
- entry.binding = static_cast<uint32_t>(bindingNumber);
+ entry.binding = uint32_t(bindingNumber);
entry.visibility = StageBit(stage.shaderStage);
// Add it to our map of all entries, if there is an existing entry, then we
@@ -379,7 +398,7 @@
if (!inserted) {
DAWN_TRY_CONTEXT(MergeEntries(&existingEntry->second, entry),
"merging implicit bindings for @group(%u) @binding(%u).",
- uint32_t(group), uint32_t(bindingNumber));
+ group, bindingNumber);
}
}
}
@@ -408,9 +427,9 @@
// be created with `pipelineCompatibilityToken` whether they are empty or not.
PerBindGroup<Ref<BindGroupLayoutBase>> bindGroupLayouts = {};
for (auto group : Range(kMaxBindGroupsTyped)) {
- DAWN_TRY_ASSIGN(
- bindGroupLayouts[group],
- CreateBGL(device, entryData[group], pipelineCompatibilityToken, allowInternalBinding));
+ DAWN_TRY_ASSIGN(bindGroupLayouts[group],
+ CreateBGL(device, std::move(entryData[group]), pipelineCompatibilityToken,
+ allowInternalBinding));
}
// Create the deduced pipeline layout, validating if it is valid.
diff --git a/src/dawn/native/ShaderModule.cpp b/src/dawn/native/ShaderModule.cpp
index a61d41a..b30a6739 100644
--- a/src/dawn/native/ShaderModule.cpp
+++ b/src/dawn/native/ShaderModule.cpp
@@ -521,6 +521,16 @@
"Entry point's stage (%s) is not in the binding visibility in the layout (%s).",
StageBit(entryPointStage), layoutInfo.visibility);
+ DAWN_INVALID_IF(layoutInfo.arraySize < shaderInfo.arraySize,
+ "Binding type in the shader is a binding_array with %u elements but the "
+ "layout only provides %u elements",
+ shaderInfo.arraySize, layoutInfo.arraySize);
+ DAWN_INVALID_IF(layoutInfo.indexInArray != BindingIndex(0),
+ "@binding(%u) in the shader is element %u of the layout's binding which is an "
+ "array starting at binding %u.",
+ shaderInfo.binding, layoutInfo.indexInArray,
+ uint32_t(layoutInfo.binding) - uint32_t(layoutInfo.indexInArray));
+
return MatchVariant(
shaderInfo.bindingInfo,
[&](const TextureBindingInfo& bindingInfo) -> MaybeError {
@@ -973,6 +983,21 @@
info.name = resource.variable_name;
+ info.arraySize = BindingIndex(resource.array_size.value_or(1));
+ DAWN_INVALID_IF(resource.array_size.has_value() &&
+ device->IsToggleEnabled(Toggle::DisableBindGroupLayoutEntryArraySize),
+ "Use of binding_array is disabled.");
+ DAWN_INVALID_IF(
+ resource.array_size.has_value() && !device->IsToggleEnabled(Toggle::AllowUnsafeAPIs),
+ "Use of binding_array is disabled as an unsafe API.");
+ DAWN_INVALID_IF(info.arraySize == BindingIndex(0), "binding_array size is 0.");
+ if (DelayedInvalidIf(
+ info.arraySize >= BindingIndex(kMaxBindingsPerBindGroup),
+ "binding_array size (%u) exceeds the maxBindingsPerBindGroup (%u) - 1.",
+ info.arraySize, kMaxBindingsPerBindGroup)) {
+ continue;
+ }
+
switch (TintResourceTypeToBindingInfoType(resource.resource_type)) {
case BindingInfoType::Buffer: {
BufferBindingInfo bindingInfo = {};
@@ -1049,16 +1074,19 @@
return DAWN_VALIDATION_ERROR("Unknown binding type in Shader");
}
- BindingNumber bindingNumber(resource.binding);
BindGroupIndex bindGroupIndex(resource.bind_group);
-
if (DelayedInvalidIf(bindGroupIndex >= kMaxBindGroupsTyped,
"The entry-point uses a binding with a group decoration (%u) "
"that exceeds maxBindGroups (%u) - 1.",
- resource.bind_group, kMaxBindGroups) ||
- DelayedInvalidIf(bindingNumber >= kMaxBindingsPerBindGroupTyped,
- "Binding number (%u) exceeds the maxBindingsPerBindGroup limit (%u).",
- uint32_t(bindingNumber), kMaxBindingsPerBindGroup)) {
+ resource.bind_group, kMaxBindGroups)) {
+ continue;
+ }
+
+ BindingNumber bindingNumber(resource.binding);
+ if (DelayedInvalidIf(
+ bindingNumber >= kMaxBindingsPerBindGroupTyped,
+ "Binding number (%u) exceeds the maxBindingsPerBindGroup limit (%u) - 1.",
+ uint32_t(bindingNumber), kMaxBindingsPerBindGroup)) {
continue;
}
diff --git a/src/dawn/native/ShaderModule.h b/src/dawn/native/ShaderModule.h
index 992a1e7..868d212 100644
--- a/src/dawn/native/ShaderModule.h
+++ b/src/dawn/native/ShaderModule.h
@@ -168,6 +168,7 @@
// Shader metadata for a binding, very similar to information contained in a pipeline layout.
struct ShaderBindingInfo {
BindingNumber binding;
+ BindingIndex arraySize;
// The variable name of the binding resource.
std::string name;
diff --git a/src/dawn/tests/unittests/validation/BindGroupValidationTests.cpp b/src/dawn/tests/unittests/validation/BindGroupValidationTests.cpp
index 6adc3a6..d09f1ae 100644
--- a/src/dawn/tests/unittests/validation/BindGroupValidationTests.cpp
+++ b/src/dawn/tests/unittests/validation/BindGroupValidationTests.cpp
@@ -1879,6 +1879,26 @@
ASSERT_DEVICE_ERROR(device.CreateBindGroupLayout(&desc));
}
+// Check that using a binding_array statically in an entry point is disabled.
+TEST_F(BindGroupLayoutArraySizeDisabledValidationTest, BindingArrayDisabled) {
+ ASSERT_DEVICE_ERROR(utils::CreateShaderModule(device, R"(
+ @group(0) @binding(0) var textures : binding_array<texture_2d<f32>, 3>;
+ @fragment fn fs() -> @location(0) u32 {
+ let _ = textures[0];
+ return 0;
+ }
+ )"));
+
+ // Even an array of size 1 is an error.
+ ASSERT_DEVICE_ERROR(utils::CreateShaderModule(device, R"(
+ @group(0) @binding(0) var textures : binding_array<texture_2d<f32>, 1>;
+ @fragment fn fs() -> @location(0) u32 {
+ let _ = textures[0];
+ return 0;
+ }
+ )"));
+}
+
// Check that using arraySize != 1 is only allowed for sampled textures.
TEST_F(BindGroupLayoutValidationTest, ArraySizeAllowedBindingTypes) {
wgpu::BindGroupLayoutEntryArraySize arraySize1;
@@ -3574,6 +3594,82 @@
{bgl}));
}
+// Test that a BGL is compatible with a pipeline if a binding's array size is at least as big as the
+// shader's binding_array's size.
+TEST_F(BindGroupLayoutCompatibilityTest, ArraySizeCompatibility) {
+ wgpu::BindGroupLayoutEntry entry;
+ entry.binding = 0;
+ entry.visibility = wgpu::ShaderStage::Fragment;
+ entry.texture.sampleType = wgpu::TextureSampleType::Float;
+
+ wgpu::BindGroupLayoutDescriptor bglDesc;
+ bglDesc.entryCount = 1;
+ bglDesc.entries = &entry;
+ wgpu::BindGroupLayout bglNoArray = device.CreateBindGroupLayout(&bglDesc);
+
+ wgpu::BindGroupLayoutEntryArraySize arraySize;
+ arraySize.arraySize = 1;
+ entry.nextInChain = &arraySize;
+ wgpu::BindGroupLayout bglArray1 = device.CreateBindGroupLayout(&bglDesc);
+
+ arraySize.arraySize = 2;
+ wgpu::BindGroupLayout bglArray2 = device.CreateBindGroupLayout(&bglDesc);
+
+ arraySize.arraySize = 3;
+ wgpu::BindGroupLayout bglArray3 = device.CreateBindGroupLayout(&bglDesc);
+
+ // Test that a BGL with arraySize 2 is valid for binding_array<T, 2>
+ CreateFSRenderPipeline(R"(
+ @group(0) @binding(0) var t : binding_array<texture_2d<f32>, 2>;
+ @fragment fn main() {
+ _ = t[0];
+ })",
+ {bglArray2});
+ // Test that a BGL with a bigger arraySize is valid.
+ CreateFSRenderPipeline(R"(
+ @group(0) @binding(0) var t : binding_array<texture_2d<f32>, 2>;
+ @fragment fn main() {
+ _ = t[0];
+ })",
+ {bglArray3});
+ // Test that a BGL with a smaller arraySize is an error.
+ ASSERT_DEVICE_ERROR(CreateFSRenderPipeline(R"(
+ @group(0) @binding(0) var t : binding_array<texture_2d<f32>, 2>;
+ @fragment fn main() {
+ _ = t[0];
+ })",
+ {bglArray1}));
+
+ // Test that an array of size 1 BGL is valid for a single binding.
+ CreateFSRenderPipeline(R"(
+ @group(0) @binding(0) var t : texture_2d<f32>;
+ @fragment fn main() {
+ _ = t;
+ })",
+ {bglArray1});
+ // Test that an array of size 2 BGL is valid for a single binding at the start of the array.
+ CreateFSRenderPipeline(R"(
+ @group(0) @binding(0) var t : texture_2d<f32>;
+ @fragment fn main() {
+ _ = t;
+ })",
+ {bglArray2});
+ // Test that the single binding cannot be an element from the arrayed BGL except the first
+ ASSERT_DEVICE_ERROR(CreateFSRenderPipeline(R"(
+ @group(0) @binding(1) var t : texture_2d<f32>;
+ @fragment fn main() {
+ _ = t;
+ })",
+ {bglArray2}));
+ // Test that a binding_array<T, 1> bindings can be fulfilled by a non-arrayed BGL.
+ CreateFSRenderPipeline(R"(
+ @group(0) @binding(0) var t : binding_array<texture_2d<f32>, 1>;
+ @fragment fn main() {
+ _ = t[0];
+ })",
+ {bglNoArray});
+}
+
class BindingsValidationTest : public BindGroupLayoutCompatibilityTest {
public:
void SetUp() override {
diff --git a/src/dawn/tests/unittests/validation/GetBindGroupLayoutValidationTests.cpp b/src/dawn/tests/unittests/validation/GetBindGroupLayoutValidationTests.cpp
index 8fbc56d..00adc9f 100644
--- a/src/dawn/tests/unittests/validation/GetBindGroupLayoutValidationTests.cpp
+++ b/src/dawn/tests/unittests/validation/GetBindGroupLayoutValidationTests.cpp
@@ -1014,6 +1014,132 @@
}
}
+// Test that a binding_array is reflected into a BGLEntry with an arraySize.
+TEST_F(GetBindGroupLayoutTests, ArraySizeReflected) {
+ DAWN_SKIP_TEST_IF(UsesWire());
+
+ wgpu::BindGroupLayoutEntryArraySize arraySize;
+
+ wgpu::BindGroupLayoutEntry entry;
+ entry.binding = 0;
+ entry.visibility = wgpu::ShaderStage::Fragment;
+ entry.texture.sampleType = wgpu::TextureSampleType::UnfilterableFloat;
+ entry.nextInChain = &arraySize;
+
+ wgpu::BindGroupLayoutDescriptor bglDesc;
+ bglDesc.entryCount = 1;
+ bglDesc.entries = &entry;
+
+ // The pipeline using binding_array
+ wgpu::RenderPipeline pipeline = RenderPipelineFromFragmentShader(R"(
+ @group(0) @binding(0) var t: binding_array<texture_2d<f32>, 3>;
+ @fragment fn main() {
+ _ = t[0];
+ })");
+
+ arraySize.arraySize = 3;
+ EXPECT_THAT(device.CreateBindGroupLayout(&bglDesc),
+ BindGroupLayoutCacheEq(pipeline.GetBindGroupLayout(0)));
+ arraySize.arraySize = 2;
+ EXPECT_THAT(device.CreateBindGroupLayout(&bglDesc),
+ Not(BindGroupLayoutCacheEq(pipeline.GetBindGroupLayout(0))));
+ arraySize.arraySize = 1;
+ EXPECT_THAT(device.CreateBindGroupLayout(&bglDesc),
+ Not(BindGroupLayoutCacheEq(pipeline.GetBindGroupLayout(0))));
+}
+
+// Test that a binding_array is reflected into an entry with the max of both sizes
+TEST_F(GetBindGroupLayoutTests, ArraySizeTwoStages) {
+ DAWN_SKIP_TEST_IF(UsesWire());
+
+ // A BGL with arraySize = 3
+ wgpu::BindGroupLayoutEntryArraySize arraySize;
+ arraySize.arraySize = 3;
+
+ wgpu::BindGroupLayoutEntry entry;
+ entry.binding = 0;
+ entry.visibility = wgpu::ShaderStage::Vertex | wgpu::ShaderStage::Fragment;
+ entry.texture.sampleType = wgpu::TextureSampleType::UnfilterableFloat;
+ entry.nextInChain = &arraySize;
+
+ wgpu::BindGroupLayoutDescriptor bglDesc;
+ bglDesc.entryCount = 1;
+ bglDesc.entries = &entry;
+
+ // The pipeline using binding_array, with differing binding_array sizes. VS = 3, FS = 2
+ {
+ wgpu::ShaderModule module = utils::CreateShaderModule(device, R"(
+ @group(0) @binding(0) var vs_t: binding_array<texture_2d<f32>, 3>;
+ @vertex fn vs() -> @builtin(position) vec4f {
+ _ = vs_t[0];
+ return vec4(0);
+ }
+
+ @group(0) @binding(0) var fs_t: binding_array<texture_2d<f32>, 2>;
+ @fragment fn fs() {
+ _ = fs_t[0];
+ })");
+
+ utils::ComboRenderPipelineDescriptor descriptor;
+ descriptor.layout = nullptr;
+ descriptor.vertex.module = module;
+ descriptor.cFragment.module = module;
+ descriptor.cTargets[0].writeMask = wgpu::ColorWriteMask::None;
+
+ wgpu::RenderPipeline pipeline = device.CreateRenderPipeline(&descriptor);
+ EXPECT_THAT(device.CreateBindGroupLayout(&bglDesc),
+ BindGroupLayoutCacheEq(pipeline.GetBindGroupLayout(0)));
+ }
+ // The pipeline using binding_array, with differing binding_array sizes. VS = 2, FS = 3
+ {
+ wgpu::ShaderModule module = utils::CreateShaderModule(device, R"(
+ @group(0) @binding(0) var vs_t: binding_array<texture_2d<f32>, 2>;
+ @vertex fn vs() -> @builtin(position) vec4f {
+ _ = vs_t[0];
+ return vec4(0);
+ }
+
+ @group(0) @binding(0) var fs_t: binding_array<texture_2d<f32>, 3>;
+ @fragment fn fs() {
+ _ = fs_t[0];
+ })");
+
+ utils::ComboRenderPipelineDescriptor descriptor;
+ descriptor.layout = nullptr;
+ descriptor.vertex.module = module;
+ descriptor.cFragment.module = module;
+ descriptor.cTargets[0].writeMask = wgpu::ColorWriteMask::None;
+
+ wgpu::RenderPipeline pipeline = device.CreateRenderPipeline(&descriptor);
+ EXPECT_THAT(device.CreateBindGroupLayout(&bglDesc),
+ BindGroupLayoutCacheEq(pipeline.GetBindGroupLayout(0)));
+ }
+ // The pipeline using binding_array, with differing binding_array sizes. VS = 3, FS = NotArrayed
+ {
+ wgpu::ShaderModule module = utils::CreateShaderModule(device, R"(
+ @group(0) @binding(0) var vs_t: binding_array<texture_2d<f32>, 3>;
+ @vertex fn vs() -> @builtin(position) vec4f {
+ _ = vs_t[0];
+ return vec4(0);
+ }
+
+ @group(0) @binding(0) var fs_t: texture_2d<f32>;
+ @fragment fn fs() {
+ _ = fs_t;
+ })");
+
+ utils::ComboRenderPipelineDescriptor descriptor;
+ descriptor.layout = nullptr;
+ descriptor.vertex.module = module;
+ descriptor.cFragment.module = module;
+ descriptor.cTargets[0].writeMask = wgpu::ColorWriteMask::None;
+
+ wgpu::RenderPipeline pipeline = device.CreateRenderPipeline(&descriptor);
+ EXPECT_THAT(device.CreateBindGroupLayout(&bglDesc),
+ BindGroupLayoutCacheEq(pipeline.GetBindGroupLayout(0)));
+ }
+}
+
// Test it is invalid to have conflicting binding types in the shaders.
TEST_F(GetBindGroupLayoutTests, ConflictingBindingType) {
wgpu::ShaderModule vsModule = utils::CreateShaderModule(device, R"(
diff --git a/src/dawn/tests/unittests/validation/UnsafeAPIValidationTests.cpp b/src/dawn/tests/unittests/validation/UnsafeAPIValidationTests.cpp
index b42ad63..85db34a 100644
--- a/src/dawn/tests/unittests/validation/UnsafeAPIValidationTests.cpp
+++ b/src/dawn/tests/unittests/validation/UnsafeAPIValidationTests.cpp
@@ -81,6 +81,26 @@
ASSERT_DEVICE_ERROR(device.CreateBindGroupLayout(&desc));
}
+// Check that using a binding_array statically in an entry point is an unsafe API.
+TEST_F(UnsafeAPIValidationTest, BindingArrayInWGSL) {
+ ASSERT_DEVICE_ERROR(utils::CreateShaderModule(device, R"(
+ @group(0) @binding(0) var textures : binding_array<texture_2d<f32>, 3>;
+ @fragment fn fs() -> @location(0) u32 {
+ let _ = textures[0];
+ return 0;
+ }
+ )"));
+
+ // Even an array of size 1 is an error.
+ ASSERT_DEVICE_ERROR(utils::CreateShaderModule(device, R"(
+ @group(0) @binding(0) var textures : binding_array<texture_2d<f32>, 1>;
+ @fragment fn fs() -> @location(0) u32 {
+ let _ = textures[0];
+ return 0;
+ }
+ )"));
+}
+
class TimestampQueryUnsafeAPIValidationTest : public ValidationTest {
protected:
std::vector<const char*> GetDisabledToggles() override { return {"allow_unsafe_apis"}; }
