[glsl] Fixup binding comments from review.
This Cl cleans up some of the missed comments from the bindings CL
review.
Bug: 340582170
Change-Id: I5172b9352c6b32fabbe3792708fe9a9f420bfd04
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/194281
Auto-Submit: dan sinclair <dsinclair@chromium.org>
Commit-Queue: Stephen White <senorblanco@chromium.org>
Reviewed-by: Stephen White <senorblanco@chromium.org>
diff --git a/src/dawn/native/opengl/ShaderModuleGL.cpp b/src/dawn/native/opengl/ShaderModuleGL.cpp
index 9a8bce8..7893c86 100644
--- a/src/dawn/native/opengl/ShaderModuleGL.cpp
+++ b/src/dawn/native/opengl/ShaderModuleGL.cpp
@@ -211,13 +211,6 @@
tint::BindingPoint srcBindingPoint{static_cast<uint32_t>(group),
static_cast<uint32_t>(binding)};
- // For buffer bindings that can be sharable across stages, we need to rename them to
- // avoid GL program link failures due to block naming issues.
- if (std::holds_alternative<BufferBindingInfo>(shaderBindingInfo.bindingInfo) &&
- stage != SingleShaderStage::Compute) {
- req.bufferBindingVariables.emplace_back(shaderBindingInfo.name);
- }
-
BindingIndex bindingIndex = bgl->GetBindingIndex(binding);
auto& bindingIndexInfo = layout->GetBindingIndexInfo()[group];
uint32_t shaderIndex = bindingIndexInfo[bindingIndex];
@@ -227,6 +220,12 @@
std::get_if<BufferBindingInfo>(&shaderBindingInfo.bindingInfo);
if (bufferBindingInfo) {
+ // For buffer bindings that can be shareable across stages, we need to rename them
+ // to avoid GL program link failures due to block naming issues.
+ if (stage != SingleShaderStage::Compute) {
+ req.bufferBindingVariables.emplace_back(shaderBindingInfo.name);
+ }
+
switch (bufferBindingInfo->type) {
case wgpu::BufferBindingType::Uniform:
bindings.uniform.emplace(
@@ -261,12 +260,14 @@
const auto& expansion = etBindingMap.find(binding);
DAWN_ASSERT(expansion != etBindingMap.end());
+ using BindingInfo = tint::glsl::writer::binding::BindingInfo;
+
const auto& bindingExpansion = expansion->second;
- tint::glsl::writer::binding::BindingInfo plane0{
+ const BindingInfo plane0{
bindingIndexInfo[bgl->GetBindingIndex(bindingExpansion.plane0)]};
- tint::glsl::writer::binding::BindingInfo plane1{
+ const BindingInfo plane1{
bindingIndexInfo[bgl->GetBindingIndex(bindingExpansion.plane1)]};
- tint::glsl::writer::binding::BindingInfo metadata{
+ const BindingInfo metadata{
bindingIndexInfo[bgl->GetBindingIndex(bindingExpansion.params)]};
tint::BindingPoint plane1WGSLBindingPoint{
@@ -310,7 +311,7 @@
// Note, the rest of Dawn is expecting to use the un-modified WGSL binding points when
// looking up information on the combined samplers. Tint is expecting Dawn to provide
- // the final expected values for those entry points. So, we end up using the origina
+ // the final expected values for those entry points. So, we end up using the original
// values for the AppendCombinedSampler calls and the remapped binding points when we
// put things in the tint bindings structure.
@@ -321,7 +322,7 @@
texBindPoint.binding = texIt->second.binding;
} else {
// The plane0 texture will be in external_textures, not textures, so we have to set
- // the `sampler_texture_name` based on the external_texture value.
+ // the `sampler_texture_to_name` based on the external_texture value.
auto exIt = bindings.external_texture.find(texBindPoint);
if (exIt != bindings.external_texture.end()) {
texBindPoint.group = 0;
diff --git a/src/tint/lang/glsl/writer/common/option_helpers.cc b/src/tint/lang/glsl/writer/common/option_helpers.cc
index c3d72fe..b7becc3 100644
--- a/src/tint/lang/glsl/writer/common/option_helpers.cc
+++ b/src/tint/lang/glsl/writer/common/option_helpers.cc
@@ -40,7 +40,7 @@
tint::Hashmap<tint::BindingPoint, binding::BindingInfo, 8> seen_wgsl_bindings{};
tint::Hashmap<binding::BindingInfo, tint::BindingPoint, 8> seen_glsl_bindings{};
- // Both wgsl_seen and spirv_seen check to see if the pair of [src, dst] are unique. If
+ // Both wgsl_seen and glsl_seen check to see if the pair of [src, dst] are unique. If
// we have multiple entries that map the same [src, dst] pair, that's fine. We treat it as valid
// as it's possible for multiple entry points to use the remapper at the same time. If the pair
// doesn't match, then we report an error about a duplicate binding point.
