fix: Codex review findings on #480, #484, #485, #490 (relinker prefixes, Metal depth retirement, null degamma images)

core/libs/prx/libSceAgcDriver/Graphics/Metal/MetalDepthSurface.hpp

@@ -102,6 +102,11 @@ void DropReplacedDepthSurfaces(std::vector<std::shared_ptr<MetalDepthSurface>>&
 // brings it back with the texels it held.
 void RetireDepthSurfaces(std::vector<std::shared_ptr<MetalDepthSurface>>& surfaces, std::uint64_t address, std::uint64_t bytes,
     const MetalDepthSurface* keep = nullptr);
+// The same over the depth and stencil planes of a depth target, or over every fragment of a color target.
+void RetireDepthSurfaces(std::vector<std::shared_ptr<MetalDepthSurface>>& surfaces, const Graphics::DepthTarget& target,
+    const MetalDepthSurface* keep = nullptr);
+void RetireDepthSurfaces(std::vector<std::shared_ptr<MetalDepthSurface>>& surfaces, const Graphics::ColorTarget& target,
+    const MetalDepthSurface* keep = nullptr);
 
 // A color target with several fragments (CB_COLOR_ATTRIB NUM_FRAGMENTS) renders into a resident 2D
 // multisample texture, like multisampled depth: the guest's interleaved fragment and FMASK layout is

core/libs/prx/libSceAgcDriver/Graphics/Metal/MetalDepthSurface.mm

@@ -267,6 +267,17 @@ void RetireDepthSurfaces(std::vector<std::shared_ptr<MetalDepthSurface>>& surfac
     std::erase_if(surfaces, [&](const auto& surface) { return surface.get() != keep && surface->Overlaps(address, bytes); });
 }
 
+void RetireDepthSurfaces(std::vector<std::shared_ptr<MetalDepthSurface>>& surfaces, const Graphics::DepthTarget& target,
+    const MetalDepthSurface* keep) {
+    RetireDepthSurfaces(surfaces, target.address, Graphics::DepthSliceBytes(target.extent, hasD16(target) ? 2u : 4u) * target.samples, keep);
+    if (target.stencilAddress != 0) RetireDepthSurfaces(surfaces, target.stencilAddress, Graphics::DepthSliceBytes(target.extent, 1u) * target.samples, keep);
+}
+
+void RetireDepthSurfaces(std::vector<std::shared_ptr<MetalDepthSurface>>& surfaces, const Graphics::ColorTarget& target,
+    const MetalDepthSurface* keep) {
+    RetireDepthSurfaces(surfaces, target.address, target.bytes * std::max(target.samples, 1u), keep);
+}
+
 bool MetalDepthSurface::OverlapsMappings(std::span<const NativeGuestMemory::BorrowedRange> changed) const {
     const bool d16 = target.format == VK_FORMAT_D16_UNORM || target.format == VK_FORMAT_D16_UNORM_S8_UINT;
     const auto overlaps = [&](std::uint64_t address, std::uint64_t bytes) {

core/libs/prx/libSceAgcDriver/Graphics/Metal/MetalDraw.mm

@@ -678,16 +678,11 @@ MTLTextureSwizzle channelSwizzle(std::uint8_t value) {
         // depth surfaces its planes overlap (another stencil pairing of its depth plane, another extent
         // at its address) are retired until they are bound again.
         std::erase(depthSurfaces, depth);
-        const auto& target = depth->Target();
-        const bool d16 = target.format == VK_FORMAT_D16_UNORM || target.format == VK_FORMAT_D16_UNORM_S8_UINT;
-        RetireDepthSurfaces(depthSurfaces, target.address, Graphics::DepthSliceBytes(target.extent, d16 ? 2u : 4u) * target.samples);
-        if (target.stencilAddress != 0) {
-            RetireDepthSurfaces(depthSurfaces, target.stencilAddress, Graphics::DepthSliceBytes(target.extent, 1u) * target.samples);
-        }
+        RetireDepthSurfaces(depthSurfaces, depth->Target());
         depthSurfaces.push_back(depth);
     }
-    // A color target over a depth surface's memory writes where its planes were.
-    for (const auto& color : state.colors) RetireDepthSurfaces(depthSurfaces, color.address, color.bytes, depth.get());
+    // A color target over a depth surface's memory writes where its planes were (all its fragments).
+    for (const auto& color : state.colors) RetireDepthSurfaces(depthSurfaces, color, depth.get());
     // A depth-only draw that exports a render target index renders into slices slice..lastSlice of its
     // depth array at once; each slice also gets its surface, so later single-slice and array readers find it.
     const std::uint32_t depthLayers = depth && state.layered && state.colors.empty() && state.depth->samples == 1 ?
@@ -701,7 +696,7 @@ MTLTextureSwizzle channelSwizzle(std::uint8_t value) {
             if (slice.stencilAddress != 0) slice.stencilAddress += std::uint64_t{layer} * Graphics::DepthSliceBytes(target.extent, 1u);
             slice.slice += layer;
             auto surface = depthCache->Acquire(slice);
-            std::erase_if(depthSurfaces, [&](const auto& other) { return other != surface && other->Target().address == slice.address; });
+            RetireDepthSurfaces(depthSurfaces, slice, surface.get());
             if (std::find(depthSurfaces.begin(), depthSurfaces.end(), surface) == depthSurfaces.end()) depthSurfaces.push_back(surface);
         }
     }

core/libs/prx/libSceAgcDriver/Graphics/Metal/MetalShaderResources.mm

@@ -677,7 +677,9 @@ MTLPixelFormat SrgbTwin(MTLPixelFormat format) {
     }
 }
 
-// The same levels, slices and swizzle of `view` read through `format`.
+// The same levels, slices and swizzle of `view` read through `format`. A view of a view takes its swizzle
+// relative to the root texture, not composed with the parent's: passing `view.swizzle` keeps a non-identity
+// T# channel mapping applied once (ImageSampleForceDegamma samples an R/B swap through this view).
 id<MTLTexture> SrgbView(id<MTLTexture> view, MTLPixelFormat format) {
     const auto type = view.textureType;
     const NSUInteger slices = type == MTLTextureType3D ? 1u : type == MTLTextureTypeCube || type == MTLTextureTypeCubeArray ? view.arrayLength * 6u : view.arrayLength;

core/libs/prx/libSceAgcDriver/tests/execution/ImagePlaneBiasOnSingleLevelVolume.cpp

@@ -215,6 +215,14 @@ int main() {
             Draw(*device, integerTexture, forcedSampler, bias);
             Check(true);
         }
+        // A null T# (TYPE=0) reads zero whatever its other words say, also under FORCE_DEGAMMA.
+        auto nullTexture = integerTexture;
+        nullTexture[3] &= ~(0xfu << 28u);
+        nullTexture[4] = 1u;
+        Draw(*device, nullTexture, forcedSampler, 0.0f);
+        for (std::size_t index = 0; index < Pixels.size(); ++index) {
+            Require(std::to_integer<std::uint32_t>(Pixels[index]) == 0u, "a null FORCE_DEGAMMA image read " + std::to_string(std::to_integer<std::uint32_t>(Pixels[index])) + " at byte " + std::to_string(index));
+        }
         std::string mixedRefusal;
         try {
             Draw(*device, integerTexture, forcedSampler, 0.0f, true);

core/libs/prx/libSceAgcDriver/tests/execution/ImageSampleForceDegamma.cpp

@@ -44,13 +44,17 @@ std::array<std::uint32_t, 4> BufferDescriptor(const void* data, std::uint32_t by
     return {static_cast<std::uint32_t>(address), static_cast<std::uint32_t>((address >> 32u) & 0xffffu), bytes, 0x31016facu};
 }
 
-std::array<std::uint32_t, 8> TextureDescriptor(std::uint32_t format) {
+// DST_SEL_X..W in T# word 3: identity (R, G, B, A) and an R/B swap (B, G, R, A).
+constexpr std::uint32_t IdentitySelect = 4u | (5u << 3u) | (6u << 6u) | (7u << 9u);
+constexpr std::uint32_t SwapRbSelect = 6u | (5u << 3u) | (4u << 6u) | (7u << 9u);
+
+std::array<std::uint32_t, 8> TextureDescriptor(std::uint32_t format, std::uint32_t select = IdentitySelect) {
     const auto address = static_cast<std::uint64_t>(reinterpret_cast<std::uintptr_t>(Texels.data()));
     return {
         static_cast<std::uint32_t>(address >> 8u),
         static_cast<std::uint32_t>((address >> 40u) & 0xffu) | (format << 20u) | (((Size - 1u) & 3u) << 30u),
         ((Size - 1u) >> 2u) | ((Size - 1u) << 14u),
-        0xfacu | (Type2D << 28u),
+        select | (Type2D << 28u),
         0u,
         0u,
         0u,
@@ -93,11 +97,11 @@ float SrgbToLinear(std::uint8_t code) {
     return c <= 0.04045f ? c / 12.92f : std::pow((c + 0.055f) / 1.055f, 2.4f);
 }
 
-std::array<float, Threads * 4> Sample(AgcDriver::VulkanDevice& device, std::uint32_t format, std::uint32_t filter, std::uint32_t forced) {
+std::array<float, Threads * 4> Sample(AgcDriver::VulkanDevice& device, std::uint32_t format, std::uint32_t filter, std::uint32_t forced, std::uint32_t select = IdentitySelect) {
     std::vector<std::uint32_t> userData(20, 0u);
     const auto input = BufferDescriptor(Input.data(), static_cast<std::uint32_t>(sizeof(Input)));
     const auto output = BufferDescriptor(Output.data(), static_cast<std::uint32_t>(sizeof(Output)));
-    const auto texture = TextureDescriptor(format);
+    const auto texture = TextureDescriptor(format, select);
     const auto sampler = SamplerDescriptor(filter, forced);
     std::copy(input.begin(), input.end(), userData.begin());
     std::copy(output.begin(), output.end(), userData.begin() + 4);
@@ -125,12 +129,12 @@ void RequireSame(const std::array<float, Threads * 4>& forced, const std::array<
     }
 }
 
-void RequireDecoded(const std::array<float, Threads * 4>& sampled, const char* name) {
+void RequireDecoded(const std::array<float, Threads * 4>& sampled, const char* name, bool swapRb = false) {
     for (std::uint32_t tid = 0; tid < Threads; ++tid) {
         const auto x = tid % Size;
         const auto y = (tid / Size) % Size;
         for (std::uint32_t channel = 0; channel < 4u; ++channel) {
-            const auto code = Code8(x, y, channel);
+            const auto code = Code8(x, y, swapRb && channel != 1u && channel != 3u ? 2u - channel : channel);
             const float expected = channel == 3u ? static_cast<float>(code) / 255.0f : SrgbToLinear(code);
             const float value = sampled[tid * 4u + channel];
             Require(std::fabs(value - expected) <= 0.005f, std::string(name) + " through FORCE_DEGAMMA: thread " + std::to_string(tid) + " component " + std::to_string(channel) + " sampled " + std::to_string(value) + ", expected " + std::to_string(expected));
@@ -170,6 +174,10 @@ int main() {
         Require(unormPlain[0] != unormPoint[0] || unormPlain[4] != unormPoint[4], "8_8_8_8_UNORM without FORCE_DEGAMMA sampled the decoded values");
         FillInput(false);
         RequireSame(Sample(*device, Format8888UNorm, FilterBilinear, ForceDegamma), forcedBilinear, "8_8_8_8_UNORM, bilinear");
+        // A swizzled T# (R/B swap) keeps its mapping through the sRGB view: applied once, not composed twice.
+        FillInput(true);
+        RequireDecoded(Sample(*device, Format8888UNorm, FilterPoint, ForceDegamma, SwapRbSelect), "8_8_8_8_UNORM, R/B swapped", true);
+        RequireDecoded(Sample(*device, Format8888Srgb, FilterPoint, ForceDegamma, SwapRbSelect), "8_8_8_8_SRGB, R/B swapped", true);
         Reject(*device, Format32Float, "guest format 22, which is not sRGB and has no sRGB view, is sampled through a sampler that forces sRGB decoding");
         std::puts("image sample force degamma tests passed");
         return 0;

core/libs/prx/libSceAgcDriver/tests/metal/DepthResource.mm

@@ -402,6 +402,32 @@ void retiredDepth(const AgcDriver::Metal::MetalDevice& backend) {
     surfaces = {surface, wide};
     AgcDriver::Metal::RetireDepthSurfaces(surfaces, wide->Target().address, depthBytes * 2, wide.get());
     MetalTests::Require(surfaces.size() == 1 && surfaces[0] == wide, "Binding a depth target of another extent at the address kept the other surface");
+
+    // A multisampled color target covers bytes * samples: one whose later fragments reach a depth plane retires it.
+    AgcDriver::Graphics::ColorTarget color{};
+    color.address = target.address - 0x1000;
+    color.bytes = 0x1000;
+    color.samples = 4;
+    surfaces = {surface};
+    AgcDriver::Metal::RetireDepthSurfaces(surfaces, color);
+    MetalTests::Require(surfaces.empty(), "A multisampled color target over a depth plane in its later fragments did not retire it");
+    color.samples = 1;
+    surfaces = {surface};
+    AgcDriver::Metal::RetireDepthSurfaces(surfaces, color);
+    MetalTests::Require(surfaces.size() == 1, "A single-sample color target next to a depth plane retired it");
+
+    // A depth target retires surfaces over its stencil plane too, and not only those at its depth address:
+    // a layered draw retires per slice what each slice's planes overlap.
+    auto slice = target;
+    slice.address += depthBytes;
+    slice.stencilAddress += stencilBytes;
+    auto sliceSurface = cache.Acquire(slice);
+    auto insideDepth = cache.Acquire(AgcDriver::Graphics::DepthTarget{slice.address + 0x100, 0, {width, height}, VK_FORMAT_D32_SFLOAT, 1, 0});
+    auto onStencil = cache.Acquire(AgcDriver::Graphics::DepthTarget{0x2000000, slice.stencilAddress + 0x40, {width, height}, target.format, 1, 0});
+    surfaces = {surface, insideDepth, onStencil, sliceSurface};
+    AgcDriver::Metal::RetireDepthSurfaces(surfaces, slice, sliceSurface.get());
+    MetalTests::Require(surfaces.size() == 2 && surfaces[0] == surface && surfaces[1] == sliceSurface,
+        "A depth slice did not retire the surfaces inside its depth or stencil plane, or retired its neighbour or itself");
 }
 }
 

core/relinker/codegen/src/x86/ScalarCompareOperands.cpp

@@ -17,22 +17,31 @@ constexpr std::uint8_t kEqOq = 0x00;
 constexpr std::uint8_t kLtOs = 0x01;
 constexpr std::uint8_t kLeOs = 0x02;
 
-bool _segmentPrefix(const std::uint8_t byte) {
+// Legacy prefixes that leave a scalar compare a scalar compare: segment and address-size overrides.
+bool _neutralPrefix(const std::uint8_t byte) {
     using namespace X64OpcodeConstants;
-    return byte == PrefixSegCs || byte == PrefixSegSs || byte == PrefixSegDs || byte == PrefixSegEs || byte == PrefixSegFs || byte == PrefixSegGs;
+    return byte == PrefixSegCs || byte == PrefixSegSs || byte == PrefixSegDs || byte == PrefixSegEs || byte == PrefixSegFs || byte == PrefixSegGs ||
+        byte == PrefixAddressSize;
 }
 
 }
 
 std::optional<ScalarCompare> DecodeScalarCompare(const std::uint8_t* data, const std::size_t length) {
     using namespace X64OpcodeConstants;
+    // Legacy prefixes come in any order; the last F2/F3 among them is the mandatory one.
     std::size_t pos = 0;
-    while (pos < length && _segmentPrefix(data[pos]))
+    std::uint8_t repeat = 0;
+    while (pos < length && (_neutralPrefix(data[pos]) || data[pos] == PrefixRep || data[pos] == PrefixRepne)) {
+        if (data[pos] == PrefixRep || data[pos] == PrefixRepne)
+            repeat = data[pos];
         ++pos;
+    }
     if (pos >= length)
         return std::nullopt;
     // The immediate is the instruction's last byte; the ModRM byte (and any SIB/displacement) sits between it and the opcode.
     if (data[pos] == OneByteVex2 || data[pos] == OneByteVex3) {
+        if (repeat != 0)
+            return std::nullopt;  // F2/F3 before a VEX escape is #UD
         const bool vex3 = data[pos] == OneByteVex3;
         const std::size_t opcodeOffset = pos + (vex3 ? 3 : 2);
         if (opcodeOffset + 2 >= length || data[opcodeOffset] != kOpcodeCmp)
@@ -44,10 +53,9 @@ std::optional<ScalarCompare> DecodeScalarCompare(const std::uint8_t* data, const
             return std::nullopt;
         return ScalarCompare{true, pp == kVexPpF2, static_cast<std::uint8_t>(data[length - 1] & kVexPredicateMask)};
     }
-    if (data[pos] != PrefixRep && data[pos] != PrefixRepne)
+    if (repeat == 0)
         return std::nullopt;
-    const bool isDouble = data[pos] == PrefixRepne;
-    ++pos;
+    const bool isDouble = repeat == PrefixRepne;
     if (pos < length && data[pos] >= RexMin && data[pos] <= RexMax)
         ++pos;
     if (pos + 3 >= length || data[pos] != TwoByteOpcodeEscape || data[pos + 1] != kOpcodeCmp)

core/relinker/codegen/tests/Amd64OnlyConverterTests.cpp

@@ -1708,13 +1708,30 @@ void converterScalarCompare() {
     require(Codegen::RosettaSplitsScalarCompare(*unord) && Codegen::RosettaSplitsScalarCompare(*neq) && Codegen::RosettaSplitsScalarCompare(*nle) && Codegen::RosettaSplitsScalarCompare(*trueUs)
         && Codegen::RosettaSplitsScalarCompare(*decoded(vexFalse)), "A compare Rosetta mistranslates is not split");
     require(!Codegen::RosettaSplitsScalarCompare(*lt) && !Codegen::RosettaSplitsScalarCompare(*decoded(vexLe)), "LT or LE was split");
+    // An address-size override (or a segment override) may precede the VEX escape or sit on either side of F2/F3.
+    const Bytes addr32Vex = {0x67, 0xC5, 0xFA, 0xC2, 0x05, 0x10, 0x00, 0x00, 0x00, 0x04};  // vcmpneqss xmm0, xmm0, [eip + 0x10]
+    const Bytes addr32Legacy = {0x67, 0xF3, 0x0F, 0xC2, 0x05, 0x10, 0x00, 0x00, 0x00, 0x04};  // cmpneqss xmm0, [eip + 0x10]
+    const Bytes repThenAddr32 = {0xF2, 0x67, 0x41, 0x0F, 0xC2, 0x00, 0x06};                  // cmpnlesd xmm0, [r8d]
+    const Bytes repBeforeVex = {0xF3, 0xC5, 0xFA, 0xC2, 0xC1, 0x04};                         // #UD, not a compare
+    const auto addr32VexCompare = decoded(addr32Vex);
+    require(addr32VexCompare && addr32VexCompare->Vex && !addr32VexCompare->Double && addr32VexCompare->Predicate == 4, "VCMPNEQSS with an address-size override was not decoded");
+    const auto addr32LegacyCompare = decoded(addr32Legacy);
+    require(addr32LegacyCompare && !addr32LegacyCompare->Vex && !addr32LegacyCompare->Double && addr32LegacyCompare->Predicate == 4, "CMPNEQSS with an address-size override was not decoded");
+    const auto repThenAddr32Compare = decoded(repThenAddr32);
+    require(repThenAddr32Compare && !repThenAddr32Compare->Vex && repThenAddr32Compare->Double && repThenAddr32Compare->Predicate == 6, "CMPNLESD with F2 before an address-size override was not decoded");
+    require(!decoded(repBeforeVex), "A VEX escape after F3 was decoded as a compare");
 
     const auto rosetta = Codegen::MakeAmd64OnlyInstructionMatcher(Codegen::Amd64OnlyTarget::Rosetta);
     const auto lowered = rosetta->Match(vexUnord.data(), vexUnord.size());
     require(lowered && lowered->Lowering == Codegen::Amd64OnlyLowering::Trampoline && lowered->Optional && lowered->InstructionName == "VCMPSS", "VCMPUNORDSS was not moved out of line for Rosetta");
     require(lowered && Bytes(lowered->StubBody.begin(), lowered->StubBody.begin() + static_cast<std::ptrdiff_t>(lowered->ReturnBranchOffset)) == vexUnord, "The compare stub does not run the unchanged instruction");
     const auto rip = rosetta->Match(ripNeq.data(), ripNeq.size());
     require(rip && rip->Relocations.size() == 1 && rip->Relocations[0].SiteTarget == 9 + 0x10 && rip->Relocations[0].DisplacementOffset == 4, "A RIP-relative compare does not keep its target in the stub");
+    for (const auto* bytes : {&addr32Vex, &addr32Legacy}) {
+        const auto moved = rosetta->Match(bytes->data(), bytes->size());
+        require(moved && moved->Lowering == Codegen::Amd64OnlyLowering::Trampoline && moved->Relocations.size() == 1 && moved->Relocations[0].SiteTarget == 10 + 0x10
+            && moved->Relocations[0].DisplacementOffset == 5, "An address-size-prefixed compare was not moved out of line with its target");
+    }
     require(!rosetta->Match(vexLe.data(), vexLe.size()) && !rosetta->Match(packed.data(), packed.size()), "VCMPLESS or a packed compare was moved out of line");
     require(!Codegen::MakeAmd64OnlyInstructionMatcher()->Match(vexUnord.data(), vexUnord.size()), "A scalar compare was moved out of line for an Intel host");
 

core/shader/recompiler/Optimization/src/ResourceMaterializer.cpp

@@ -509,6 +509,8 @@ void materializeSnapshot(const IrResourcePlan& plan, const SrtRuntime& runtime,
 void materializeForcedDegamma(const IrResourcePlan& plan, ResourceSnapshot& snapshot) {
     for (std::uint32_t index = 0; index < plan.info.images.size(); ++index) {
         auto& descriptor = snapshot.images.at(index);
+        // A null image reads zero whatever its other words say; its format and extent are not checked.
+        if (nullImageDescriptor(descriptor)) continue;
         if (((descriptor.dwords[1] >> 20u) & 0x1ffu) != static_cast<std::uint32_t>(IrBufferFormat::Format8_8_8_8UInt)) continue;
         bool forced = false;
         bool plain = false;