Revert "Fix location selection for pixel center polyfill." This reverts commit 5f59eb343ae38fd1e8f3a9f6d4c6709b2051c407. Reason for revert: Caused a clusterfuzz error, appears to have created a duplicate location 0. Original change's description: > Fix location selection for pixel center polyfill. > > When creating the pixel center polyfill we need to create a location in > which to assign the value. This location must match between the vertex > and fragment shaders. > > It's currently possible to setup the WGSL shader such that the current > mechanism will not select the same location. > > Instead, pass the location to use for the polyfill into the SPIR-V > writer. Dawn will use the entry point information and the locations used > in both the vertex and fragment shaders to select the lowest unused > location. > > Fixed: 517522769 > Change-Id: Ica81e95c9ab89587be6773b1950ee01f7661eee9 > Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/313217 > Commit-Queue: Corentin Wallez <cwallez@chromium.org> > Reviewed-by: Corentin Wallez <cwallez@chromium.org> > Auto-Submit: dan sinclair <dsinclair@chromium.org> > Reviewed-by: James Price <jrprice@google.com> TBR=cwallez@chromium.org,dsinclair@chromium.org,jrprice@google.com,dawn-scoped@luci-project-accounts.iam.gserviceaccount.com No-Presubmit: true No-Tree-Checks: true No-Try: true Change-Id: I911614567f6131380d677758a83fffcf8f65aa15 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/314336 Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com> Reviewed-by: dan sinclair <dsinclair@chromium.org> Commit-Queue: dan sinclair <dsinclair@chromium.org>
diff --git a/src/dawn/native/vulkan/RenderPipelineVk.cpp b/src/dawn/native/vulkan/RenderPipelineVk.cpp index 5fba0ce..8a3b45b 100644 --- a/src/dawn/native/vulkan/RenderPipelineVk.cpp +++ b/src/dawn/native/vulkan/RenderPipelineVk.cpp
@@ -27,6 +27,7 @@ #include "src/dawn/native/vulkan/RenderPipelineVk.h" +#include <memory> #include <string> #include <utility> #include <vector> @@ -505,37 +506,14 @@ return {}; }; - std::optional<uint32_t> pixelCenterPolyfillLocation = std::nullopt; - if (NeedsPixelCenterPolyfill()) { - const EntryPointMetadata* vtx = GetStage(SingleShaderStage::Vertex).metadata; - for (size_t i = 0; i < vtx->usedInterStageVariables.size(); ++i) { - if (vtx->usedInterStageVariables[i] == false) { - pixelCenterPolyfillLocation = uint32_t(i); - break; - } - } - if (!pixelCenterPolyfillLocation.has_value()) { - return DAWN_INTERNAL_ERROR( - "unable to find a free vertex location for the pixel center polyfill"); - } - - if (HasStage(SingleShaderStage::Fragment)) { - const EntryPointMetadata* frag = GetStage(SingleShaderStage::Fragment).metadata; - // Because the fragment stage must be a subset of the vertex stage, if the value was - // free in the vertex stage it _must_ be free in the fragment stage. - DAWN_ASSERT(frag->usedInterStageVariables[pixelCenterPolyfillLocation.value()] == - false); - } - } - // Add the vertex stage that's always present. DAWN_TRY(AddShaderStage({ .stage = &GetStage(SingleShaderStage::Vertex), .layout = layout, .immediateMask = GetImmediateMask(), .ycbcrExternalTextures = &specialization.ycbcrExternalTextures, - .polyfillPixelCenter = pixelCenterPolyfillLocation, .emitPointSize = GetPrimitiveTopology() == wgpu::PrimitiveTopology::PointList, + .polyfillPixelCenter = NeedsPixelCenterPolyfill(), .pipelineUsesFramebufferFetch = UsesFramebufferFetch(), })); @@ -546,7 +524,7 @@ .layout = layout, .immediateMask = GetImmediateMask(), .ycbcrExternalTextures = &specialization.ycbcrExternalTextures, - .polyfillPixelCenter = pixelCenterPolyfillLocation, + .polyfillPixelCenter = NeedsPixelCenterPolyfill(), .pipelineUsesFramebufferFetch = UsesFramebufferFetch(), .needsMultisampledFramebufferFetch = UseSampleRateShading() && UsesFramebufferFetch(), }));
diff --git a/src/dawn/native/vulkan/ShaderModuleVk.h b/src/dawn/native/vulkan/ShaderModuleVk.h index 693d9fb..337af93 100644 --- a/src/dawn/native/vulkan/ShaderModuleVk.h +++ b/src/dawn/native/vulkan/ShaderModuleVk.h
@@ -72,8 +72,8 @@ ImmediateMask immediateMask; raw_ptr<const absl::flat_hash_set<APIBindPoint>> ycbcrExternalTextures; - std::optional<uint32_t> polyfillPixelCenter = std::nullopt; bool emitPointSize = false; + bool polyfillPixelCenter = false; bool pipelineUsesFramebufferFetch = false; bool needsMultisampledFramebufferFetch = false; };
diff --git a/src/dawn/tests/BUILD.gn b/src/dawn/tests/BUILD.gn index 73d213a..5e533fc 100644 --- a/src/dawn/tests/BUILD.gn +++ b/src/dawn/tests/BUILD.gn
@@ -648,7 +648,6 @@ "end2end/ShaderAtomicTests.cpp", "end2end/ShaderBuiltinPartialConstArgsErrorTests.cpp", "end2end/ShaderF16Tests.cpp", - "end2end/ShaderIOPolyfillTests.cpp", "end2end/ShaderModuleCachingTests.cpp", "end2end/ShaderPrintTests.cpp", "end2end/ShaderTests.cpp",
diff --git a/src/dawn/tests/CMakeLists.txt b/src/dawn/tests/CMakeLists.txt index 9b64d0b..ec77cff 100644 --- a/src/dawn/tests/CMakeLists.txt +++ b/src/dawn/tests/CMakeLists.txt
@@ -160,7 +160,6 @@ "end2end/ShaderAtomicTests.cpp" "end2end/ShaderBuiltinPartialConstArgsErrorTests.cpp" "end2end/ShaderF16Tests.cpp" - "end2end/ShaderIOPolyfillTests.cpp" "end2end/ShaderModuleCachingTests.cpp" "end2end/ShaderPrintTests.cpp" "end2end/ShaderTests.cpp"
diff --git a/src/dawn/tests/end2end/ShaderIOPolyfillTests.cpp b/src/dawn/tests/end2end/ShaderIOPolyfillTests.cpp deleted file mode 100644 index 20501ff..0000000 --- a/src/dawn/tests/end2end/ShaderIOPolyfillTests.cpp +++ /dev/null
@@ -1,111 +0,0 @@ -// Copyright 2026 The Dawn & Tint Authors -// -// Redistribution and use in source and binary forms, with or without -// modification, are permitted provided that the following conditions are met: -// -// 1. Redistributions of source code must retain the above copyright notice, this -// list of conditions and the following disclaimer. -// -// 2. Redistributions in binary form must reproduce the above copyright notice, -// this list of conditions and the following disclaimer in the documentation -// and/or other materials provided with the distribution. -// -// 3. Neither the name of the copyright holder nor the names of its -// contributors may be used to endorse or promote products derived from -// this software without specific prior written permission. -// -// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" -// AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE -// IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE -// DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE -// FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL -// DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR -// SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER -// CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, -// OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE -// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - -#include <utility> - -#include "src/dawn/tests/DawnTest.h" -#include "src/dawn/utils/ComboRenderPipelineDescriptor.h" -#include "src/dawn/utils/WGPUHelpers.h" - -namespace dawn { -namespace { - -using ShaderIOPolyfillTests = DawnTest; - -// https://crbug.com/517522769 -TEST_P(ShaderIOPolyfillTests, DivergentLocations) { - wgpu::ShaderModule vsModule = utils::CreateShaderModule(device, R"( - struct VOut { - @builtin(position) pos : vec4f, - @location(0) a : vec4f, - @location(1) @interpolate(flat) b : i32, - }; - @vertex fn vs(@builtin(vertex_index) vi : u32) -> VOut { - var p = array<vec2f, 3>( - vec2f(-1, -1), - vec2f( 3, -1), - vec2f(-1, 3)); - var o : VOut; - o.pos = vec4f(p[vi], 0, 1); - o.a = vec4f(1, 0, 0, 1); - o.b = 0x7eadbeef; - return o; - } - )"); - - wgpu::ShaderModule fsModule = utils::CreateShaderModule(device, R"( - @fragment fn fs( - @location(0) a : vec4f, - @builtin(sample_index) s : u32, - @builtin(position) p : vec4f) -> @location(0) vec4f { - return a + vec4f(p.z, f32(s) * 0, 0, 0); - } - )"); - - utils::ComboRenderPipelineDescriptor pipelineDesc; - pipelineDesc.vertex.module = std::move(vsModule); - pipelineDesc.cFragment.module = std::move(fsModule); - pipelineDesc.multisample.count = 4; - pipelineDesc.cTargets[0].format = wgpu::TextureFormat::RGBA8Unorm; - - wgpu::RenderPipeline pipeline = device.CreateRenderPipeline(&pipelineDesc); - - wgpu::TextureDescriptor msaaDesc; - msaaDesc.size = {64, 64, 1}; - msaaDesc.sampleCount = 4; - msaaDesc.format = wgpu::TextureFormat::RGBA8Unorm; - msaaDesc.usage = wgpu::TextureUsage::RenderAttachment; - wgpu::Texture msaa = device.CreateTexture(&msaaDesc); - - wgpu::TextureDescriptor resolveDesc; - resolveDesc.size = {64, 64, 1}; - resolveDesc.format = wgpu::TextureFormat::RGBA8Unorm; - resolveDesc.usage = wgpu::TextureUsage::RenderAttachment | wgpu::TextureUsage::CopySrc; - wgpu::Texture resolve = device.CreateTexture(&resolveDesc); - - wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - utils::ComboRenderPassDescriptor renderPass({msaa.CreateView()}); - renderPass.cColorAttachments[0].resolveTarget = resolve.CreateView(); - renderPass.cColorAttachments[0].clearValue = {0.0, 0.0, 0.0, 0.0}; - renderPass.cColorAttachments[0].loadOp = wgpu::LoadOp::Clear; - renderPass.cColorAttachments[0].storeOp = wgpu::StoreOp::Discard; - - wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass); - pass.SetPipeline(pipeline); - pass.Draw(3); - pass.End(); - - wgpu::CommandBuffer commands = encoder.Finish(); - queue.Submit(1, &commands); - - EXPECT_PIXEL_RGBA8_EQ(utils::RGBA8::kRed, resolve, 32, 32); -} - -DAWN_INSTANTIATE_TEST(ShaderIOPolyfillTests, VulkanBackend()); - -} // namespace -} // namespace dawn
diff --git a/src/tint/lang/spirv/writer/common/options.h b/src/tint/lang/spirv/writer/common/options.h index 2ae83f1..c9a5dd9 100644 --- a/src/tint/lang/spirv/writer/common/options.h +++ b/src/tint/lang/spirv/writer/common/options.h
@@ -229,9 +229,8 @@ /// from all vertex shaders in the module. bool emit_vertex_point_size = true; - /// If the optional is set, then we apply the builtin 'position' pixel center emulation with a - /// location provided by the optional. - std::optional<uint32_t> polyfill_pixel_center = std::nullopt; + /// Set to `true` to apply builtin 'position' pixel center emulation. + bool polyfill_pixel_center = false; /// Set to `true` if framebuffer fetch should be multisampled bool multisampled_framebuffer_fetch = false;
diff --git a/src/tint/lang/spirv/writer/raise/shader_io.cc b/src/tint/lang/spirv/writer/raise/shader_io.cc index 3149c44..820623c 100644 --- a/src/tint/lang/spirv/writer/raise/shader_io.cc +++ b/src/tint/lang/spirv/writer/raise/shader_io.cc
@@ -99,12 +99,32 @@ /// Add a new interpolant that will be used to emulate the position builtin as if it always is /// pixel centered. + /// @param entries the entries to emit /// @param addrspace the address to use for the global variables - uint32_t AddCenterPosInterpolant(core::AddressSpace addrspace) { + uint32_t AddCenterPosInterpolant(Vector<core::type::Manager::StructMemberDesc, 4>& entries, + core::AddressSpace addrspace) { + // Verbose way of finding the smallest free location (id). This of course needs to be the + // same id value for both vertex and fragment. + std::set<uint32_t> existing_locations; + for (auto io : entries) { + if (io.attributes.location.has_value()) { + existing_locations.insert(io.attributes.location.value()); + } + } + uint32_t free_location = 0u; + // We only need to search through existing_locations.size + 1 because either we will simply + // add an index to the end or there will be a hole in the range of locations + for (uint32_t i = 0u; i < (existing_locations.size() + 1); i++) { + if (existing_locations.find(i) == existing_locations.end()) { + free_location = i; + break; + } + } + // For our 'position' -> 'FragCoord' polyfill we must use 'perspective' because // 'interpolateAtOffset' may not be supported for 'linear'. auto io_attrib = core::IOAttributes{ - .location = config.polyfill_pixel_center.value(), + .location = free_location, .interpolation = core::Interpolation{.type = core::InterpolationType::kPerspective, .sampling = core::InterpolationSampling::kCenter}}; @@ -126,12 +146,12 @@ core::Access access, const char* name_suffix) { if (func->IsVertex() && addrspace == core::AddressSpace::kOut && - config.polyfill_pixel_center.has_value()) { - center_pos_vert_idx = AddCenterPosInterpolant(addrspace); + config.polyfill_pixel_center) { + center_pos_vert_idx = AddCenterPosInterpolant(entries, addrspace); } else if (func->IsFragment() && addrspace == core::AddressSpace::kIn && - config.polyfill_pixel_center.has_value()) { - center_pos_frag_idx = AddCenterPosInterpolant(addrspace); + config.polyfill_pixel_center) { + center_pos_frag_idx = AddCenterPosInterpolant(entries, addrspace); } for (auto io : entries) {
diff --git a/src/tint/lang/spirv/writer/raise/shader_io.h b/src/tint/lang/spirv/writer/raise/shader_io.h index dc9a721..27a00cb 100644 --- a/src/tint/lang/spirv/writer/raise/shader_io.h +++ b/src/tint/lang/spirv/writer/raise/shader_io.h
@@ -61,7 +61,7 @@ /// true if f16 IO types should be replaced with f32 types and converted bool polyfill_f16_io = false; /// true if we should force pixel centers via polyfill when multi-sampling. - std::optional<uint32_t> polyfill_pixel_center = std::nullopt; + bool polyfill_pixel_center = false; /// true if the framebuffer fetch should be multisampled bool multisampled_framebuffer_fetch = false; /// offsets for clamping frag depth
diff --git a/src/tint/lang/spirv/writer/raise/shader_io_test.cc b/src/tint/lang/spirv/writer/raise/shader_io_test.cc index 0b79059..08928cc 100644 --- a/src/tint/lang/spirv/writer/raise/shader_io_test.cc +++ b/src/tint/lang/spirv/writer/raise/shader_io_test.cc
@@ -1422,7 +1422,7 @@ core::ir::transform::ImmediateDataLayout immediate_data; ShaderIOConfig config{immediate_data}; - config.polyfill_pixel_center = 1; + config.polyfill_pixel_center = true; Run(ShaderIO, config); EXPECT_EQ(expect, str()); @@ -1457,7 +1457,7 @@ $B1: { # root %foo_position_Input:ptr<__in, vec4<f32>, read> = var undef @builtin(position) %foo_sample_index_Input:ptr<__in, u32, read> = var undef @interpolate(flat) @builtin(sample_index) - %foo_loc1_Input:ptr<__in, vec4<f32>, read> = var undef @location(1) @interpolate(perspective, center) + %foo_loc0_Input:ptr<__in, vec4<f32>, read> = var undef @location(0) @interpolate(perspective, center) } %foo_inner = func(%position:vec4<f32>, %idx:u32):void { @@ -1472,7 +1472,7 @@ %10:vec2<f32> = swizzle %9, xy %11:vec2<f32> = floor %10 %12:vec2<f32> = add %11, vec2<f32>(0.5f) - %13:vec4<f32> = spirv.interpolate_at_offset %foo_loc1_Input, vec2<f32>(0.0f) + %13:vec4<f32> = spirv.interpolate_at_offset %foo_loc0_Input, vec2<f32>(0.0f) %14:f32 = swizzle %13, z %15:f32 = swizzle %13, w %16:f32 = div %14, %15 @@ -1487,7 +1487,7 @@ core::ir::transform::ImmediateDataLayout immediate_data; ShaderIOConfig config{immediate_data}; - config.polyfill_pixel_center = 1; + config.polyfill_pixel_center = true; Run(ShaderIO, config); EXPECT_EQ(expect, str()); @@ -1552,7 +1552,7 @@ core::ir::transform::ImmediateDataLayout immediate_data; ShaderIOConfig config{immediate_data}; - config.polyfill_pixel_center = 1; + config.polyfill_pixel_center = true; Run(ShaderIO, config); EXPECT_EQ(expect, str()); @@ -1641,7 +1641,7 @@ core::ir::transform::ImmediateDataLayout immediate_data; ShaderIOConfig config{immediate_data}; - config.polyfill_pixel_center = 0; + config.polyfill_pixel_center = true; Run(ShaderIO, config); EXPECT_EQ(expect, str()); @@ -1725,7 +1725,7 @@ core::ir::transform::ImmediateDataLayout immediate_data; ShaderIOConfig config{immediate_data}; - config.polyfill_pixel_center = 0; + config.polyfill_pixel_center = true; Run(ShaderIO, config); EXPECT_EQ(expect, str()); @@ -1792,7 +1792,7 @@ core::ir::transform::ImmediateDataLayout immediate_data; ShaderIOConfig config{immediate_data}; - config.polyfill_pixel_center = 1; + config.polyfill_pixel_center = true; Run(ShaderIO, config); EXPECT_EQ(expect, str());
diff --git a/src/tint/lang/spirv/writer/writer_test.cc b/src/tint/lang/spirv/writer/writer_test.cc index 0dc44b4..9d2b1c0 100644 --- a/src/tint/lang/spirv/writer/writer_test.cc +++ b/src/tint/lang/spirv/writer/writer_test.cc
@@ -457,7 +457,7 @@ }); Options options; - options.polyfill_pixel_center = 0; + options.polyfill_pixel_center = true; auto result = Generate(options); ASSERT_EQ(result, Success) << result.Failure() << output_; EXPECT_INST(R"(