OpenGL: Use binding= for all bindings in generated GLSL
This changes the call to SPIRV-Cross in the GLSL backend to always set
the binding= decoration on interface variables instead of changing the
variable name and matching against it as was done previously. This
matching by name was done for macOS's OpenGL driver's old version of GL
but isn't needed now that Dawn has a Metal backend for macOS.
This also fixes opengl::ShaderModule to iterate only on bindings that
are known to the layout as the reflection between SPIRV-Cross and Tint
can be different.
Bug: tint:808
Bug: tint:386
Change-Id: I308b8da5994d8618799cf6583f1b0e5ea2ba2069
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/51520
Commit-Queue: 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 bcff274..4417610 100644
--- a/src/dawn_native/BindGroupLayout.cpp
+++ b/src/dawn_native/BindGroupLayout.cpp
@@ -542,6 +542,10 @@
return mBindingMap;
}
+ bool BindGroupLayoutBase::HasBinding(BindingNumber bindingNumber) const {
+ return mBindingMap.count(bindingNumber) != 0;
+ }
+
BindingIndex BindGroupLayoutBase::GetBindingIndex(BindingNumber bindingNumber) const {
ASSERT(!IsError());
const auto& it = mBindingMap.find(bindingNumber);
diff --git a/src/dawn_native/BindGroupLayout.h b/src/dawn_native/BindGroupLayout.h
index 113c591..641bf45 100644
--- a/src/dawn_native/BindGroupLayout.h
+++ b/src/dawn_native/BindGroupLayout.h
@@ -54,6 +54,7 @@
return mBindingInfo[bindingIndex];
}
const BindingMap& GetBindingMap() const;
+ bool HasBinding(BindingNumber bindingNumber) const;
BindingIndex GetBindingIndex(BindingNumber bindingNumber) const;
// Functions necessary for the unordered_set<BGLBase*>-based cache.
diff --git a/src/dawn_native/opengl/PipelineGL.cpp b/src/dawn_native/opengl/PipelineGL.cpp
index a0181ef..0110025 100644
--- a/src/dawn_native/opengl/PipelineGL.cpp
+++ b/src/dawn_native/opengl/PipelineGL.cpp
@@ -119,125 +119,58 @@
}
}
- // The uniforms are part of the program state so we can pre-bind buffer units, texture units
- // etc.
+ // Compute links between stages for combined samplers, then bind them to texture units
gl.UseProgram(mProgram);
const auto& indices = layout->GetBindingIndexInfo();
- for (BindGroupIndex group : IterateBitSet(layout->GetBindGroupLayoutsMask())) {
- const BindGroupLayoutBase* bgl = layout->GetBindGroupLayout(group);
-
- for (const auto& it : bgl->GetBindingMap()) {
- BindingNumber bindingNumber = it.first;
- BindingIndex bindingIndex = it.second;
-
- std::string name = GetBindingName(group, bindingNumber);
- const BindingInfo& bindingInfo = bgl->GetBindingInfo(bindingIndex);
- switch (bindingInfo.bindingType) {
- case BindingInfoType::Buffer:
- switch (bindingInfo.buffer.type) {
- case wgpu::BufferBindingType::Uniform: {
- GLint location = gl.GetUniformBlockIndex(mProgram, name.c_str());
- if (location != -1) {
- gl.UniformBlockBinding(mProgram, location,
- indices[group][bindingIndex]);
- }
- break;
- }
- case wgpu::BufferBindingType::Storage:
- case wgpu::BufferBindingType::ReadOnlyStorage: {
- // Since glShaderStorageBlockBinding doesn't exist in OpenGL ES, we
- // skip that call and handle it during shader translation by
- // modifying the location decoration. Contrary to all other binding
- // types, OpenGL ES's SSBO binding index in the SSBO table is the
- // value of the location= decoration in GLSL.
- if (gl.GetVersion().IsDesktop()) {
- GLuint location = gl.GetProgramResourceIndex(
- mProgram, GL_SHADER_STORAGE_BLOCK, name.c_str());
- if (location != GL_INVALID_INDEX) {
- gl.ShaderStorageBlockBinding(mProgram, location,
- indices[group][bindingIndex]);
- }
- }
- break;
- }
- case wgpu::BufferBindingType::Undefined:
- UNREACHABLE();
- }
- break;
-
- case BindingInfoType::Sampler:
- case BindingInfoType::Texture:
- // These binding types are handled in the separate sampler and texture
- // emulation
- break;
-
- case BindingInfoType::StorageTexture: {
- if (gl.GetVersion().IsDesktop()) {
- GLint location = gl.GetUniformLocation(mProgram, name.c_str());
- if (location != -1) {
- gl.Uniform1i(location, indices[group][bindingIndex]);
- }
- }
- break;
- }
- }
+ std::set<CombinedSampler> combinedSamplersSet;
+ for (SingleShaderStage stage : IterateStages(activeStages)) {
+ for (const CombinedSampler& combined : combinedSamplers[stage]) {
+ combinedSamplersSet.insert(combined);
}
}
- // Compute links between stages for combined samplers, then bind them to texture units
- {
- std::set<CombinedSampler> combinedSamplersSet;
- for (SingleShaderStage stage : IterateStages(activeStages)) {
- for (const CombinedSampler& combined : combinedSamplers[stage]) {
- combinedSamplersSet.insert(combined);
- }
+ mUnitsForSamplers.resize(layout->GetNumSamplers());
+ mUnitsForTextures.resize(layout->GetNumSampledTextures());
+
+ GLuint textureUnit = layout->GetTextureUnitsUsed();
+ for (const auto& combined : combinedSamplersSet) {
+ const std::string& name = combined.GetName();
+ GLint location = gl.GetUniformLocation(mProgram, name.c_str());
+
+ if (location == -1) {
+ continue;
}
- mUnitsForSamplers.resize(layout->GetNumSamplers());
- mUnitsForTextures.resize(layout->GetNumSampledTextures());
+ gl.Uniform1i(location, textureUnit);
- GLuint textureUnit = layout->GetTextureUnitsUsed();
- for (const auto& combined : combinedSamplersSet) {
- std::string name = combined.GetName();
- GLint location = gl.GetUniformLocation(mProgram, name.c_str());
+ bool shouldUseFiltering;
+ {
+ const BindGroupLayoutBase* bgl =
+ layout->GetBindGroupLayout(combined.textureLocation.group);
+ BindingIndex bindingIndex = bgl->GetBindingIndex(combined.textureLocation.binding);
- if (location == -1) {
- continue;
- }
+ GLuint textureIndex = indices[combined.textureLocation.group][bindingIndex];
+ mUnitsForTextures[textureIndex].push_back(textureUnit);
- gl.Uniform1i(location, textureUnit);
-
- bool shouldUseFiltering;
- {
+ shouldUseFiltering = bgl->GetBindingInfo(bindingIndex).texture.sampleType ==
+ wgpu::TextureSampleType::Float;
+ }
+ {
+ if (combined.useDummySampler) {
+ mDummySamplerUnits.push_back(textureUnit);
+ } else {
const BindGroupLayoutBase* bgl =
- layout->GetBindGroupLayout(combined.textureLocation.group);
+ layout->GetBindGroupLayout(combined.samplerLocation.group);
BindingIndex bindingIndex =
- bgl->GetBindingIndex(combined.textureLocation.binding);
+ bgl->GetBindingIndex(combined.samplerLocation.binding);
- GLuint textureIndex = indices[combined.textureLocation.group][bindingIndex];
- mUnitsForTextures[textureIndex].push_back(textureUnit);
-
- shouldUseFiltering = bgl->GetBindingInfo(bindingIndex).texture.sampleType ==
- wgpu::TextureSampleType::Float;
+ GLuint samplerIndex = indices[combined.samplerLocation.group][bindingIndex];
+ mUnitsForSamplers[samplerIndex].push_back({textureUnit, shouldUseFiltering});
}
- {
- if (combined.useDummySampler) {
- mDummySamplerUnits.push_back(textureUnit);
- } else {
- const BindGroupLayoutBase* bgl =
- layout->GetBindGroupLayout(combined.samplerLocation.group);
- BindingIndex bindingIndex =
- bgl->GetBindingIndex(combined.samplerLocation.binding);
-
- GLuint samplerIndex = indices[combined.samplerLocation.group][bindingIndex];
- mUnitsForSamplers[samplerIndex].push_back(
- {textureUnit, shouldUseFiltering});
- }
- }
-
- textureUnit++;
}
+
+ textureUnit++;
}
}
diff --git a/src/dawn_native/opengl/ShaderModuleGL.cpp b/src/dawn_native/opengl/ShaderModuleGL.cpp
index 8d8c311..b147626 100644
--- a/src/dawn_native/opengl/ShaderModuleGL.cpp
+++ b/src/dawn_native/opengl/ShaderModuleGL.cpp
@@ -179,38 +179,41 @@
// Change binding names to be "dawn_binding_<group>_<binding>".
// Also unsets the SPIRV "Binding" decoration as it outputs "layout(binding=)" which
// isn't supported on OSX's OpenGL.
- for (BindGroupIndex group(0); group < kMaxBindGroupsTyped; ++group) {
+ const PipelineLayout::BindingIndexInfo& indices = layout->GetBindingIndexInfo();
+
+ // Modify the decoration of variables so that SPIRV-Cross outputs only
+ // layout(binding=<index>) for interface variables.
+ //
+ // When the use_tint_generator toggle is on, Tint is used for the reflection of bindings
+ // for the implicit pipeline layout and pipeline/layout validation, but bindingInfo is set
+ // to mGLEntryPoints which is the SPIRV-Cross reflection. Tint reflects bindings used more
+ // precisely than SPIRV-Cross so some bindings in bindingInfo might not exist in the layout
+ // and querying the layout for them would cause an ASSERT. That's why we defensively check
+ // that bindings are in the layout before modifying them. This slight hack is ok because in
+ // the long term we will use Tint to produce GLSL.
+ for (BindGroupIndex group : IterateBitSet(layout->GetBindGroupLayoutsMask())) {
for (const auto& it : bindingInfo[group]) {
+ const BindGroupLayoutBase* bgl = layout->GetBindGroupLayout(group);
BindingNumber bindingNumber = it.first;
const auto& info = it.second;
- uint32_t resourceId;
- switch (info.bindingType) {
- // When the resource is a uniform or shader storage block, we should change the
- // block name instead of the instance name.
- case BindingInfoType::Buffer:
- resourceId = info.base_type_id;
- break;
- default:
- resourceId = info.id;
- break;
+ if (!bgl->HasBinding(bindingNumber)) {
+ continue;
}
- compiler.set_name(resourceId, GetBindingName(group, bindingNumber));
+ // Remove the name of the base type. This works around an issue where if the SPIRV
+ // has two uniform/storage interface variables that point to the same base type,
+ // then SPIRV-Cross would emit two bindings with type names that conflict:
+ //
+ // layout(binding=0) uniform Buf {...} binding0;
+ // layout(binding=1) uniform Buf {...} binding1;
+ compiler.set_name(info.base_type_id, "");
+
+ BindingIndex bindingIndex = bgl->GetBindingIndex(bindingNumber);
+
compiler.unset_decoration(info.id, spv::DecorationDescriptorSet);
- // OpenGL ES has no glShaderStorageBlockBinding call, so we adjust the SSBO binding
- // decoration here instead.
- if (version.IsES() && info.bindingType == BindingInfoType::Buffer &&
- (info.buffer.type == wgpu::BufferBindingType::Storage ||
- info.buffer.type == wgpu::BufferBindingType::ReadOnlyStorage)) {
- const auto& indices = layout->GetBindingIndexInfo();
- BindingIndex bindingIndex =
- layout->GetBindGroupLayout(group)->GetBindingIndex(bindingNumber);
- compiler.set_decoration(info.id, spv::DecorationBinding,
- indices[group][bindingIndex]);
- } else {
- compiler.unset_decoration(info.id, spv::DecorationBinding);
- }
+ compiler.set_decoration(info.id, spv::DecorationBinding,
+ indices[group][bindingIndex]);
}
}
