shader: parse vertex metadata once when a shader is mapped

src/graphics/shader/shader.cpp

@@ -40,9 +40,15 @@
 
 namespace Libs::Graphics {
 
+// A mapped shader and its vertex metadata, parsed once when it is mapped.
+struct ShaderMapData: ShaderMappedData {
+	ShaderVertexMetadata vertex;
+	const char*          vertex_error = nullptr;
+};
+
 struct ShaderMapEntry {
-	ShaderMappedData data;
+	ShaderMapData data;
-	uint64_t hash;
+	uint64_t      hash;
 };
 
 static std::unique_ptr<std::unordered_map<uint64_t, ShaderMapEntry>> g_shader_map;
@@ -64,8 +70,11 @@ void ShaderMapUserData(uint64_t addr, const ShaderMappedData& data) {
 		     addr, data.code_size_bytes);
 	}
 	const auto hash = XXH3_64bits(reinterpret_cast<const void*>(addr), data.code_size_bytes);
+	ShaderMapData mapped {data};
+	mapped.vertex_error =
+	    ShaderReadVertexMetadata(data, HW::UserSgprInfo::SGPRS_MAX, mapped.vertex);
 	std::scoped_lock lock(g_shader_map_mutex);
-	(*g_shader_map)[addr] = {data, hash};
+	(*g_shader_map)[addr] = {mapped, hash};
 }
 
 static ShaderMapEntry ShaderGetMappedData(uint64_t addr, const char* label) {
@@ -261,8 +270,8 @@ static uint32_t ShaderCalcPsSystemInputBase(const HW::ShaderRegisters& regs) {
 
 static bool ShaderGetStaticVertexInputInfo(uint64_t shader_addr, const HW::UserSgprInfo& user_sgpr,
                                            uint32_t user_sgpr_num, const HW::ShaderRegisters& sh,
-                                           const ShaderMappedData& data,
+                                           const ShaderMapData&   data,
-                                           ShaderVertexInputInfo&  info) {
+                                           ShaderVertexInputInfo& info) {
 	KYTY_PROFILER_FUNCTION();
 
 	info = {};
@@ -277,13 +286,12 @@ static bool ShaderGetStaticVertexInputInfo(uint64_t shader_addr, const HW::UserS
 		     " user_sgpr_num=%u\n",
 		     shader_addr, user_sgpr_num);
 	}
-	ShaderVertexMetadata metadata;
+	if (data.vertex_error != nullptr) {
-	std::string          metadata_error;
-	if (!ShaderReadVertexMetadata(data, HW::UserSgprInfo::SGPRS_MAX, metadata, &metadata_error)) {
 		LOGF("ShaderGetVertexInputInfo(): invalid AGC metadata shader=0x%016" PRIx64 ": %s\n",
-		     shader_addr, metadata_error.c_str());
+		     shader_addr, data.vertex_error);
 		return false;
 	}
+	const auto& metadata = data.vertex;
 
 	if (metadata.vertex_attrib_reg >= 0) {
 		info.fetch_external   = false;

src/graphics/shader/shader.h

@@ -15,17 +15,6 @@
 
 namespace Libs::Graphics {
 
-namespace ShaderError {
-
-[[nodiscard]] inline bool Fail(std::string* error, std::string_view message) {
-	if (error != nullptr) {
-		error->assign(message.data(), message.size());
-	}
-	return false;
-}
-
-} // namespace ShaderError
-
 namespace HW {
 struct VertexShaderInfo;
 struct PixelShaderInfo;

src/graphics/shader/shaderVertexMetadata.cpp

@@ -5,10 +5,10 @@
 
 namespace Libs::Graphics {
 
-bool ShaderReadVertexMetadata(const ShaderMappedData& data, uint32_t max_user_sgprs,
+const char* ShaderReadVertexMetadata(const ShaderMappedData& data, uint32_t max_user_sgprs,
-                              ShaderVertexMetadata& metadata, std::string* error) {
+                                     ShaderVertexMetadata& metadata) {
 	if (data.user_data == nullptr) {
-		return ShaderError::Fail(error, "missing AGC user-data header");
+		return "missing AGC user-data header";
 	}
 
 	ShaderUserData user_data {};
@@ -17,8 +17,7 @@ bool ShaderReadVertexMetadata(const ShaderMappedData& data, uint32_t max_user_sg
 	constexpr uint32_t DirectResourceCount =
 	    static_cast<uint32_t>(AgcDirectResourceType::Last) + 1u;
 	if (user_data.direct_resource_count > DirectResourceCount) {
-		return ShaderError::Fail(error,
+		return "AGC direct-resource count exceeds the known resource domain";
-		                         "AGC direct-resource count exceeds the known resource domain");
 	}
 
 	std::array<uint16_t, DirectResourceCount> direct_offsets {};
@@ -26,7 +25,7 @@ bool ShaderReadVertexMetadata(const ShaderMappedData& data, uint32_t max_user_sg
 	    static_cast<uint64_t>(user_data.direct_resource_count) * sizeof(uint16_t);
 	if (direct_size != 0) {
 		if (user_data.direct_resource_offset == nullptr) {
-			return ShaderError::Fail(error, "missing AGC direct-resource offsets");
+			return "missing AGC direct-resource offsets";
 		}
 		std::memcpy(direct_offsets.data(), user_data.direct_resource_offset, direct_size);
 	}
@@ -47,33 +46,33 @@ bool ShaderReadVertexMetadata(const ShaderMappedData& data, uint32_t max_user_sg
 	}
 
 	if (next.vertex_attrib_reg >= 0 && next.vertex_buffer_reg < 0) {
-		return ShaderError::Fail(error, "vertex attribute table requires a vertex buffer table");
+		return "vertex attribute table requires a vertex buffer table";
 	}
 	if (next.vertex_buffer_reg < 0) {
 		metadata = next;
-		return true;
+		return nullptr;
 	}
 	if (static_cast<uint32_t>(next.vertex_buffer_reg) + 1u >= max_user_sgprs) {
-		return ShaderError::Fail(error, "vertex table pointer exceeds the user-SGPR domain");
+		return "vertex table pointer exceeds the user-SGPR domain";
 	}
 	if (next.vertex_attrib_reg < 0) {
 		metadata = next;
-		return true;
+		return nullptr;
 	}
 	if (static_cast<uint32_t>(next.vertex_attrib_reg) + 1u >= max_user_sgprs) {
-		return ShaderError::Fail(error, "vertex table pointer exceeds the user-SGPR domain");
+		return "vertex table pointer exceeds the user-SGPR domain";
 	}
 	if (data.num_input_semantics == 0 ||
 	    data.num_input_semantics > ShaderVertexInputInfo::RES_MAX) {
-		return ShaderError::Fail(error, "vertex semantic count is outside the supported domain");
+		return "vertex semantic count is outside the supported domain";
 	}
 
 	if (data.input_semantics == nullptr) {
-		return ShaderError::Fail(error, "missing vertex input semantics");
+		return "missing vertex input semantics";
 	}
 	next.input_semantics = {data.input_semantics, data.num_input_semantics};
 	metadata                   = next;
-	return true;
+	return nullptr;
 }
 
 } // namespace Libs::Graphics

src/graphics/shader/shaderVertexMetadata.h

@@ -4,7 +4,6 @@
 #include "graphics/shader/shader.h"
 
 #include <span>
-#include <string>
 
 namespace Libs::Graphics {
 
@@ -14,9 +13,10 @@ struct ShaderVertexMetadata {
 	std::span<const ShaderSemantic> input_semantics;
 };
 
-// Validates the vertex metadata and returns a view into the shader header.
+// Reads the vertex metadata as a view into the shader header. Returns nullptr if it is
-bool ShaderReadVertexMetadata(const ShaderMappedData& data, uint32_t max_user_sgprs,
+// valid, otherwise why it was rejected.
-                              ShaderVertexMetadata& metadata, std::string* error);
+const char* ShaderReadVertexMetadata(const ShaderMappedData& data, uint32_t max_user_sgprs,
+                                     ShaderVertexMetadata& metadata);
 
 } // namespace Libs::Graphics
 

tests/ShaderVertexMetadataTests.cpp

@@ -40,9 +40,7 @@ void CheckRejected(const ShaderMappedData& data, const char* text) {
 	output.vertex_buffer_reg = 37;
 	output.vertex_attrib_reg = 41;
 	output.input_semantics   = prior_semantics;
-	std::string error;
+	Check(ShaderReadVertexMetadata(data, 64, output) != nullptr, text);
-	Check(!ShaderReadVertexMetadata(data, 64, output, &error), text);
-	Check(!error.empty(), "metadata rejection omitted its diagnostic");
 	Check(output.vertex_buffer_reg == 37 && output.vertex_attrib_reg == 41 &&
 	          output.input_semantics.data() == prior_semantics.data() &&
 	          output.input_semantics.size() == prior_semantics.size(),
@@ -52,8 +50,7 @@ void CheckRejected(const ShaderMappedData& data, const char* text) {
 void TestValidAndInvalidMetadata() {
 	Fixture              fixture;
 	ShaderVertexMetadata output;
-	std::string          error;
+	Check(ShaderReadVertexMetadata(fixture.mapped, 64, output) == nullptr,
-	Check(ShaderReadVertexMetadata(fixture.mapped, 64, output, &error),
 	      "valid AGC vertex metadata was rejected");
 	Check(output.vertex_buffer_reg == 2 && output.vertex_attrib_reg == 4 &&
 	          output.input_semantics.size() == 1,
@@ -64,7 +61,7 @@ void TestValidAndInvalidMetadata() {
 	    AGC_ILLEGAL_DIRECT_OFFSET;
 	buffer_only.mapped.input_semantics     = nullptr;
 	buffer_only.mapped.num_input_semantics = 0;
-	Check(ShaderReadVertexMetadata(buffer_only.mapped, 64, output, &error),
+	Check(ShaderReadVertexMetadata(buffer_only.mapped, 64, output) == nullptr,
 	      "vertex-buffer-only AGC metadata was rejected");
 	Check(output.vertex_buffer_reg == 2 && output.vertex_attrib_reg == -1 &&
 	          output.input_semantics.empty(),

tests/shaderCfgTests.cpp

@@ -1906,6 +1906,13 @@ void TestVertexBufferGrouping() {
               input.resources[2].Base48() == input.buffers[0].addr,
           "a later lower-address attribute or changed table left stale vertex offsets");
   }
+  // The vertex table metadata is parsed when the shader is mapped.
+  user_data.direct_resource_count = 0;
+  (void)PrepareProgram(regs, HW::Context{}, HW::UserConfig{}, input);
+  Check(input.fetch_attrib_reg == 2, "a draw reread the vertex metadata of a mapped shader");
+  ShaderMapUserData(regs.es_regs.data_addr, mapped);
+  (void)PrepareProgram(regs, HW::Context{}, HW::UserConfig{}, input);
+  Check(!input.fetch_embedded, "remapping a shader kept its previous vertex metadata");
 }
 
 void TestNggVertexEntryState() {