Merge pull request #3561 from XairRrR/fix/agc-swap-alt-review-guards

fix(agc): reject constant color factors and memory-writing stages in split SWAP_ALT blends
This commit is contained in:
Adrià 2026-10-11 23:34:15 +02:00 committed by GitHub
commit ada499b635
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 75 additions and 20 deletions

View File

@ -107,6 +107,7 @@ void ValidateDepthBounds(const Context& context, const State& state);
void ValidateProvokingVertex(const Context& context, const State& state, std::span<const CompiledShader> shaders = {});
void ValidateShaderPair(const ShaderRecompiler::RecompileResult& vertex, const ShaderRecompiler::RecompileResult& fragment);
bool ShadersWriteMemory(std::span<const CompiledShader> shaders);
// Returns the color attachment locations the pixel shader writes.
std::set<std::uint32_t> ValidateShaders(std::span<const CompiledShader> shaders, const State& state, const VkPhysicalDeviceSubgroupProperties& subgroup, bool fragmentShaderBarycentric, bool descriptorIndexing = false, bool imageInt64Atomics = false, bool geometryShader = false, bool sampleRateShading = false, bool bufferInt64Atomics = false);

View File

@ -115,23 +115,10 @@ void ValidateViewport(const Context& context, const VkViewport& viewport) {
Require(std::min(viewport.y, viewport.y + viewport.height) >= context.limits.viewportBoundsRange[0] && std::max(viewport.y, viewport.y + viewport.height) <= context.limits.viewportBoundsRange[1], "viewport Y exceeds device bounds");
}
namespace {
VkShaderStageFlags pushStagesOf(const Context& context) {
return PreRasterizationPushStages(context.meshShader, context.tessellationShader, context.geometryShader);
}
template<typename TValue>
void appendKey(std::vector<std::byte>& key, const TValue& value) {
static_assert(std::is_trivially_copyable_v<TValue>);
const auto bytes = std::as_bytes(std::span(&value, 1));
key.insert(key.end(), bytes.begin(), bytes.end());
}
bool fragmentWritesMemory(std::span<const CompiledShader> shaders) {
bool ShadersWriteMemory(std::span<const CompiledShader> shaders) {
const auto any = [](const std::vector<bool>& written, std::uint32_t count) { return written.size() < count || std::ranges::any_of(written, [](bool element) { return element; }); };
for (const auto& shader : shaders) {
if (shader.stage != ShaderRecompiler::ShaderStage::Fragment || shader.program == nullptr) continue;
if (shader.program == nullptr) continue;
for (const auto& binding : shader.program->bindings) {
switch (binding.role) {
case ShaderRecompiler::DescriptorRole::GuestBuffers:
@ -154,6 +141,19 @@ bool fragmentWritesMemory(std::span<const CompiledShader> shaders) {
return false;
}
namespace {
VkShaderStageFlags pushStagesOf(const Context& context) {
return PreRasterizationPushStages(context.meshShader, context.tessellationShader, context.geometryShader);
}
template<typename TValue>
void appendKey(std::vector<std::byte>& key, const TValue& value) {
static_assert(std::is_trivially_copyable_v<TValue>);
const auto bytes = std::as_bytes(std::span(&value, 1));
key.insert(key.end(), bytes.begin(), bytes.end());
}
bool libraryKeys(PipelineLibraryKeys& keys, const Context& context, const State& state, const VertexInputLayout& input, const ShaderResources& resources, std::span<const CompiledShader> shaders) {
using Stage = ShaderRecompiler::ShaderStage;
if (!context.graphicsPipelineLibrary || context.pipelineExecutableInfo || state.depthBiasPerFace || state.rectList || SplitsAlternateAlpha(state)) return false;
@ -379,7 +379,7 @@ Pipeline::Pipeline(const Context& context, const State& state, const VertexInput
if (alternateAlpha) {
const auto keepsStencil = [](const VkStencilOpState& face) { return face.writeMask == 0 || (face.failOp == VK_STENCIL_OP_KEEP && face.passOp == VK_STENCIL_OP_KEEP && face.depthFailOp == VK_STENCIL_OP_KEEP); };
Require(!(state.depth && (state.depthWrite || (state.stencilTest && (!keepsStencil(state.stencilFront) || !keepsStencil(state.stencilBack))))) && !state.depthBiasPerFace, "blending into an 8_8 color target with the alternate component order and a separate alpha blend from a draw that writes depth or stencil is not implemented");
Require(!fragmentWritesMemory(shaders), "blending into an 8_8 color target with the alternate component order and a separate alpha blend from a pixel shader that writes memory is not implemented");
Require(!ShadersWriteMemory(shaders), "blending into an 8_8 color target with the alternate component order and a separate alpha blend from a draw whose shaders write memory is not implemented");
for (std::size_t index = 0; index < secondChannels.size(); ++index) {
const bool split = index < state.colors.size() && state.colors[index].componentMapping == 0xecu && state.blends[index].blendEnable;
if (!split) {

View File

@ -771,6 +771,8 @@ State DecodeState(const QueueState& queue, bool vertexWorkgroup) {
if (mapping == 0xecu) {
const auto destinationAlpha = [](VkBlendFactor factor) { return factor == VK_BLEND_FACTOR_DST_ALPHA || factor == VK_BLEND_FACTOR_ONE_MINUS_DST_ALPHA || factor == VK_BLEND_FACTOR_SRC_ALPHA_SATURATE; };
if (destinationAlpha(state.srcColorBlendFactor) || destinationAlpha(state.dstColorBlendFactor) || destinationAlpha(state.srcAlphaBlendFactor) || destinationAlpha(state.dstAlphaBlendFactor)) throw std::runtime_error("AGC graphics: blending into an 8_8 color target with the alternate component order with a factor of the destination alpha, which the target keeps in its second channel, is not implemented");
const auto weighted = [](VkBlendOp operation) { return operation != VK_BLEND_OP_MIN && operation != VK_BLEND_OP_MAX; };
if ((weighted(state.colorBlendOp) && (constantColor(state.srcColorBlendFactor) || constantColor(state.dstColorBlendFactor))) || (weighted(state.alphaBlendOp) && (constantColor(state.srcAlphaBlendFactor) || constantColor(state.dstAlphaBlendFactor)))) throw std::runtime_error("AGC graphics: blending into an 8_8 color target with the alternate component order with a constant color factor, which Vulkan applies to the second channel with the green blend constant instead of the alpha one, is not implemented");
}
}
if (color.packing != ShaderRecompiler::ColorExportPacking::None) {

View File

@ -878,6 +878,29 @@ void TuningFieldTests() {
}
}
void ShadersWriteMemoryTests() {
const auto binding = [](ShaderRecompiler::DescriptorKind kind, ShaderRecompiler::DescriptorRole role, std::vector<bool> written) {
ShaderRecompiler::DescriptorBinding result{};
result.kind = kind;
result.role = role;
result.count = 1;
if (role == ShaderRecompiler::DescriptorRole::GuestImages) result.imageWritten = std::move(written);
else result.bufferWritten = std::move(written);
return result;
};
ShaderRecompiler::RecompileResult reads;
reads.bindings.push_back(binding(ShaderRecompiler::DescriptorKind::StorageBuffer, ShaderRecompiler::DescriptorRole::GuestBuffers, {false}));
reads.bindings.push_back(binding(ShaderRecompiler::DescriptorKind::StorageImage, ShaderRecompiler::DescriptorRole::GuestImages, {false}));
ShaderRecompiler::RecompileResult stores;
stores.bindings.push_back(binding(ShaderRecompiler::DescriptorKind::StorageBuffer, ShaderRecompiler::DescriptorRole::GuestBuffers, {true}));
using Stage = ShaderRecompiler::ShaderStage;
const auto writes = [](std::initializer_list<AgcDriver::Graphics::CompiledShader> shaders) { return AgcDriver::Graphics::ShadersWriteMemory(std::vector<AgcDriver::Graphics::CompiledShader>(shaders)); };
Require(!writes({{Stage::Vertex, &reads, 0}, {Stage::Fragment, &reads, 0}}), "shaders that only read memory were reported as writing it");
Require(writes({{Stage::Vertex, &reads, 0}, {Stage::Fragment, &stores, 0}}), "a pixel shader that stores to a buffer was not reported");
Require(writes({{Stage::Vertex, &stores, 0}, {Stage::Fragment, &reads, 0}}), "a vertex shader that stores to a buffer was not reported");
Require(writes({{Stage::Geometry, &stores, 0}, {Stage::Fragment, &reads, 0}}), "a geometry shader that stores to a buffer was not reported");
}
void ReversedComponentOrderTests() {
{
auto queue = makeState();
@ -891,6 +914,12 @@ void ReversedComponentOrderTests() {
Require(!AgcDriver::Graphics::SplitsAlternateAlpha(blended), "one blend equation for color and alpha split an 8_8 SWAP_ALT draw");
queue.context[0x1e0] = 0x60000001u;
Require(AgcDriver::Graphics::SplitsAlternateAlpha(AgcDriver::Graphics::DecodeState(queue)), "a separate alpha blend into an 8_8 SWAP_ALT target did not split its second channel");
queue.context[0x1e0] = (1u << 30u) | 13u;
expectFailure([&] { AgcDriver::Graphics::DecodeState(queue); }, "constant color factor");
queue.context[0x1e0] = (1u << 30u) | (1u << 29u) | 1u | (1u << 16u) | (14u << 24u);
expectFailure([&] { AgcDriver::Graphics::DecodeState(queue); }, "constant color factor");
queue.context[0x1e0] = (1u << 30u) | (2u << 5u) | 13u;
Require(AgcDriver::Graphics::DecodeState(queue).blends[0].colorBlendOp == VK_BLEND_OP_MIN, "a constant color factor of a MIN blend into an 8_8 SWAP_ALT target was rejected");
}
for (const auto& [swap, mapping] : {std::pair{2u, 0x1bu}, std::pair{3u, 0x93u}}) {
auto queue = makeState();
@ -4305,6 +4334,7 @@ int main(int argc, char** argv) {
OpaqueDestinationAlphaTests();
CompactedExportTests();
ReversedComponentOrderTests();
ShadersWriteMemoryTests();
DualSourceBlendTests();
metadataPassTests();
cmaskTests();

View File

@ -31,6 +31,7 @@
#include <iostream>
#include <span>
#include <string>
#include <string_view>
#include <vector>
namespace {
@ -53,6 +54,12 @@ alignas(256) constexpr std::array<std::uint32_t, 6> VertexCode{
0xe0382000, 0x80000005, 0xbf8c3f70, 0xf80008cf, 0x03020100, 0xbf810000,
};
alignas(256) constexpr std::array<std::uint32_t, 9> StoringVertexCode{
0xe0382000, 0x80000005, 0xbf8c3f70, 0xe0700000, 0x80010000, 0xbf8c3f70, 0xf80008cf, 0x03020100, 0xbf810000,
};
alignas(256) std::array<std::uint32_t, 64> Stored{};
constexpr std::array<std::array<float, 4>, 3> Triangle{{
{-1.0f, -1.0f, 0.5f, 1.0f}, {3.0f, -1.0f, 0.5f, 1.0f}, {-1.0f, 3.0f, 0.5f, 1.0f}
}};
@ -163,16 +170,19 @@ VkPipelineColorBlendAttachmentState Blend(VkBlendFactor source, VkBlendFactor de
return blend;
}
void Draw(AgcDriver::VulkanDevice& device, const AgcDriver::Graphics::ColorTarget& color, const VkPipelineColorBlendAttachmentState& blend, std::span<const std::uint32_t> pixelCode) {
void Draw(AgcDriver::VulkanDevice& device, const AgcDriver::Graphics::ColorTarget& color, const VkPipelineColorBlendAttachmentState& blend, std::span<const std::uint32_t> pixelCode, std::span<const std::uint32_t> vertexCode = VertexCode) {
const auto target = device.Target();
constexpr std::uint32_t waveSize = 64;
std::vector<std::uint32_t> vertexUserData(4, 0u);
std::vector<std::uint32_t> vertexUserData(8, 0u);
const auto address = reinterpret_cast<std::uintptr_t>(Triangle.data());
const std::array<std::uint32_t, 4> vertexBuffer{static_cast<std::uint32_t>(address), static_cast<std::uint32_t>((address >> 32u) & 0xffffu) | (16u << 16u), static_cast<std::uint32_t>(Triangle.size()), 0x01016facu};
std::copy(vertexBuffer.begin(), vertexBuffer.end(), vertexUserData.begin());
const std::array<ShaderRecompiler::MemoryRegion, 1> vertexMemory{{{reinterpret_cast<std::uintptr_t>(VertexCode.data()), std::as_bytes(std::span(VertexCode))}}};
const auto stored = reinterpret_cast<std::uintptr_t>(Stored.data());
const std::array<std::uint32_t, 4> storeBuffer{static_cast<std::uint32_t>(stored), static_cast<std::uint32_t>((stored >> 32u) & 0xffffu), static_cast<std::uint32_t>(sizeof(Stored)), 0x01016facu};
std::copy(storeBuffer.begin(), storeBuffer.end(), vertexUserData.begin() + 4);
const std::array<ShaderRecompiler::MemoryRegion, 1> vertexMemory{{{reinterpret_cast<std::uintptr_t>(vertexCode.data()), std::as_bytes(vertexCode)}}};
ShaderRecompiler::RecompileRequest vertex{
{ShaderStage::Vertex, reinterpret_cast<std::uintptr_t>(VertexCode.data()), VertexCode, 0, {}},
{ShaderStage::Vertex, reinterpret_cast<std::uintptr_t>(vertexCode.data()), vertexCode, 0, {}},
{waveSize, 0, vertexUserData, std::nullopt, std::nullopt, ShaderRecompiler::ShaderVertexStageInfo{}, vertexMemory},
target,
{0, 0, 0, 64}
@ -281,6 +291,18 @@ int main() {
{"ONE + ONE_MINUS_SRC_ALPHA for color and alpha", Blend(VK_BLEND_FACTOR_ONE, VK_BLEND_FACTOR_ONE_MINUS_SRC_ALPHA, VK_BLEND_OP_ADD, VK_BLEND_FACTOR_ONE, VK_BLEND_FACTOR_ONE_MINUS_SRC_ALPHA, VK_BLEND_OP_ADD), false,
[&](float r, float g) { return std::array<float, 2>{x + r * (1.0f - w), w + g * (1.0f - w)}; }},
}};
{
Fill(block, layout);
const auto color = DecodeTarget(block);
bool rejected = false;
try {
Draw(*device, color, cases[0].blend, Program(exported), StoringVertexCode);
} catch (const std::exception& error) {
rejected = std::string_view(error.what()).find("a draw whose shaders write memory") != std::string_view::npos;
if (!rejected) throw;
}
Require(rejected, "a split 8_8 SWAP_ALT draw whose vertex shader stores to memory was not rejected");
}
for (const auto& test : cases) {
const auto before = Fill(block, layout);
const auto color = DecodeTarget(block);