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,14 @@ 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);
+// Slice `slice + layer` of a target's depth array; an absent plane (address 0, as the depth plane of a
+// stencil-only target) stays absent.
+Graphics::DepthTarget DepthArraySlice(const Graphics::DepthTarget& target, std::uint32_t layer);
+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,25 @@ 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);
+}
+
+Graphics::DepthTarget DepthArraySlice(const Graphics::DepthTarget& target, std::uint32_t layer) {
+    auto slice = target;
+    if (slice.address != 0) slice.address += std::uint64_t{layer} * Graphics::DepthSliceBytes(target.extent, hasD16(target) ? 2u : 4u);
+    if (slice.stencilAddress != 0) slice.stencilAddress += std::uint64_t{layer} * Graphics::DepthSliceBytes(target.extent, 1u);
+    slice.slice += layer;
+    return slice;
+}
+
+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,30 +678,20 @@ 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 ?
         state.depth->lastSlice - state.depth->slice + 1u : 0u;
     if (depthLayers > 1) {
-        const auto& target = *state.depth;
-        const bool d16 = target.format == VK_FORMAT_D16_UNORM || target.format == VK_FORMAT_D16_UNORM_S8_UINT;
         for (std::uint32_t layer = 1; layer < depthLayers; ++layer) {
-            auto slice = target;
-            slice.address += std::uint64_t{layer} * Graphics::DepthSliceBytes(target.extent, d16 ? 2u : 4u);
-            if (slice.stencilAddress != 0) slice.stencilAddress += std::uint64_t{layer} * Graphics::DepthSliceBytes(target.extent, 1u);
-            slice.slice += layer;
+            const auto slice = DepthArraySlice(*state.depth, 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,42 @@ 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.
+    const auto slice = AgcDriver::Metal::DepthArraySlice(target, 1);
+    MetalTests::Require(slice.address == target.address + depthBytes && slice.stencilAddress == target.stencilAddress + stencilBytes && slice.slice == target.slice + 1,
+        "A depth array slice does not start one slice past the target");
+    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");
+
+    // A stencil-only target has no depth plane: its slices have none either, and retire nothing at low addresses.
+    const AgcDriver::Graphics::DepthTarget stencilOnly{0, 0x800000, {width, height}, VK_FORMAT_S8_UINT, 1, 0};
+    const auto stencilSlice = AgcDriver::Metal::DepthArraySlice(stencilOnly, 2);
+    MetalTests::Require(stencilSlice.address == 0 && stencilSlice.stencilAddress == stencilOnly.stencilAddress + 2 * stencilBytes,
+        "A stencil-only slice was given a depth plane");
+    auto low = cache.Acquire(AgcDriver::Graphics::DepthTarget{2 * depthBytes, 0, {width, height}, VK_FORMAT_D32_SFLOAT, 1, 0});
+    surfaces = {low};
+    AgcDriver::Metal::RetireDepthSurfaces(surfaces, stencilSlice);
+    MetalTests::Require(surfaces.size() == 1, "A stencil-only slice retired a depth surface at a fabricated depth address");
 }
 }
 

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)