From a9d884e58db82c2c7e6227a24096616f4c6c8783 Mon Sep 17 00:00:00 2001 From: David Neto Date: Tue, 26 Nov 2024 17:57:32 -0500 Subject: [PATCH] spirv-as: Assume target from spvasm text (#5893) * spirv-as: Assume target from spvasm text Assume the desired SPIR-V target version when the command line option is not provided. This is especially important when the source being assembled does not match the default (latest version), because it may create an invalid SPIR-V binary result. Also, clarify the wording for where a copy of spirv-headers is expected (to match expected capitalization). * fix formatting * Add unit tests, static asserts on supported SPIR-V versions Rework the algorithm so it accepts earlier termination of the version string. That made testing easier and more intuitive. * Fix compilation problems --------- Co-authored-by: mmoult <30711895+mmoult@users.noreply.github.com> --- external/CMakeLists.txt | 2 +- source/spirv_target_env.cpp | 120 ++++++++++++++++++++++++++++-------- source/spirv_target_env.h | 7 +++ test/target_env_test.cpp | 57 ++++++++++++++++- tools/as/as.cpp | 47 +++++++------- 5 files changed, 180 insertions(+), 53 deletions(-) diff --git a/external/CMakeLists.txt b/external/CMakeLists.txt index 5d8a3dab09..1ccab19640 100644 --- a/external/CMakeLists.txt +++ b/external/CMakeLists.txt @@ -45,7 +45,7 @@ if (IS_DIRECTORY ${SPIRV_HEADER_DIR}) endif() else() message(FATAL_ERROR - "SPIRV-Headers was not found - please checkout a copy under external/.") + "SPIRV-Headers was not found - please checkout a copy at external/spirv-headers.") endif() if (NOT ${SPIRV_SKIP_TESTS}) diff --git a/source/spirv_target_env.cpp b/source/spirv_target_env.cpp index 585f8b65a2..a787481d9b 100644 --- a/source/spirv_target_env.cpp +++ b/source/spirv_target_env.cpp @@ -14,11 +14,13 @@ #include "source/spirv_target_env.h" +#include #include +#include #include #include -#include +#include "source/latest_version_spirv_header.h" #include "source/spirv_constant.h" #include "spirv-tools/libspirv.h" @@ -128,32 +130,43 @@ uint32_t spvVersionForTargetEnv(spv_target_env env) { return SPV_SPIRV_VERSION_WORD(0, 0); } -static const std::pair spvTargetEnvNameMap[] = { - {"vulkan1.1spv1.4", SPV_ENV_VULKAN_1_1_SPIRV_1_4}, - {"vulkan1.0", SPV_ENV_VULKAN_1_0}, - {"vulkan1.1", SPV_ENV_VULKAN_1_1}, - {"vulkan1.2", SPV_ENV_VULKAN_1_2}, - {"vulkan1.3", SPV_ENV_VULKAN_1_3}, - {"spv1.0", SPV_ENV_UNIVERSAL_1_0}, - {"spv1.1", SPV_ENV_UNIVERSAL_1_1}, - {"spv1.2", SPV_ENV_UNIVERSAL_1_2}, - {"spv1.3", SPV_ENV_UNIVERSAL_1_3}, - {"spv1.4", SPV_ENV_UNIVERSAL_1_4}, - {"spv1.5", SPV_ENV_UNIVERSAL_1_5}, - {"spv1.6", SPV_ENV_UNIVERSAL_1_6}, - {"opencl1.2embedded", SPV_ENV_OPENCL_EMBEDDED_1_2}, - {"opencl1.2", SPV_ENV_OPENCL_1_2}, - {"opencl2.0embedded", SPV_ENV_OPENCL_EMBEDDED_2_0}, - {"opencl2.0", SPV_ENV_OPENCL_2_0}, - {"opencl2.1embedded", SPV_ENV_OPENCL_EMBEDDED_2_1}, - {"opencl2.1", SPV_ENV_OPENCL_2_1}, - {"opencl2.2embedded", SPV_ENV_OPENCL_EMBEDDED_2_2}, - {"opencl2.2", SPV_ENV_OPENCL_2_2}, - {"opengl4.0", SPV_ENV_OPENGL_4_0}, - {"opengl4.1", SPV_ENV_OPENGL_4_1}, - {"opengl4.2", SPV_ENV_OPENGL_4_2}, - {"opengl4.3", SPV_ENV_OPENGL_4_3}, - {"opengl4.5", SPV_ENV_OPENGL_4_5}, +// When a new SPIR-V version is released, update this table. +static_assert(spv::Version == 0x10600); +constexpr auto ordered_universal_envs = std::array{ + SPV_ENV_UNIVERSAL_1_0, SPV_ENV_UNIVERSAL_1_1, SPV_ENV_UNIVERSAL_1_2, + SPV_ENV_UNIVERSAL_1_3, SPV_ENV_UNIVERSAL_1_4, SPV_ENV_UNIVERSAL_1_5, + SPV_ENV_UNIVERSAL_1_6, +}; + +// When a new SPIR-V version is released, update this table. +static_assert(spv::Version == 0x10600); +inline constexpr std::pair spvTargetEnvNameMap[] = + { + {"vulkan1.1spv1.4", SPV_ENV_VULKAN_1_1_SPIRV_1_4}, + {"vulkan1.0", SPV_ENV_VULKAN_1_0}, + {"vulkan1.1", SPV_ENV_VULKAN_1_1}, + {"vulkan1.2", SPV_ENV_VULKAN_1_2}, + {"vulkan1.3", SPV_ENV_VULKAN_1_3}, + {"spv1.0", SPV_ENV_UNIVERSAL_1_0}, + {"spv1.1", SPV_ENV_UNIVERSAL_1_1}, + {"spv1.2", SPV_ENV_UNIVERSAL_1_2}, + {"spv1.3", SPV_ENV_UNIVERSAL_1_3}, + {"spv1.4", SPV_ENV_UNIVERSAL_1_4}, + {"spv1.5", SPV_ENV_UNIVERSAL_1_5}, + {"spv1.6", SPV_ENV_UNIVERSAL_1_6}, + {"opencl1.2embedded", SPV_ENV_OPENCL_EMBEDDED_1_2}, + {"opencl1.2", SPV_ENV_OPENCL_1_2}, + {"opencl2.0embedded", SPV_ENV_OPENCL_EMBEDDED_2_0}, + {"opencl2.0", SPV_ENV_OPENCL_2_0}, + {"opencl2.1embedded", SPV_ENV_OPENCL_EMBEDDED_2_1}, + {"opencl2.1", SPV_ENV_OPENCL_2_1}, + {"opencl2.2embedded", SPV_ENV_OPENCL_EMBEDDED_2_2}, + {"opencl2.2", SPV_ENV_OPENCL_2_2}, + {"opengl4.0", SPV_ENV_OPENGL_4_0}, + {"opengl4.1", SPV_ENV_OPENGL_4_1}, + {"opengl4.2", SPV_ENV_OPENGL_4_2}, + {"opengl4.3", SPV_ENV_OPENGL_4_3}, + {"opengl4.5", SPV_ENV_OPENGL_4_5}, }; bool spvParseTargetEnv(const char* s, spv_target_env* env) { @@ -172,6 +185,59 @@ bool spvParseTargetEnv(const char* s, spv_target_env* env) { return false; } +bool spvReadEnvironmentFromText(const std::vector& text, + spv_target_env* env) { + // Version is expected to match "; Version: 1.X" + // Version string must occur in header, that is, initial lines of comments + // Once a non-comment line occurs, the header has ended + for (std::size_t i = 0; i < text.size(); ++i) { + char c = text[i]; + + if (c == ';') { + // Try to match against the expected version string + constexpr const char* kVersionPrefix = "; Version: 1."; + constexpr const auto kPrefixLength = 13; + // 'minor_digit_pos' is the expected position of the version digit. + const auto minor_digit_pos = i + kPrefixLength; + if (minor_digit_pos >= text.size()) return false; + + // Match the prefix. + auto j = 1; + for (; j < kPrefixLength; ++j) { + if (kVersionPrefix[j] != text[i + j]) break; + } + // j will match the prefix length if all characters before matched + if (j == kPrefixLength) { + // This expects only one digit in the minor number. + static_assert(((spv::Version >> 8) & 0xff) < 10); + char minor = text[minor_digit_pos]; + char next_char = + minor_digit_pos + 1 < text.size() ? text[minor_digit_pos + 1] : 0; + if (std::isdigit(minor) && !std::isdigit(next_char)) { + const auto index = minor - '0'; + assert(index >= 0); + if (static_cast(index) < ordered_universal_envs.size()) { + *env = ordered_universal_envs[index]; + return true; + } + } + } + + // If no match, determine whether the header has ended (in which case, + // assumption has failed.) + // Skip until the next line. + i = j; + for (; i < text.size(); ++i) { + if (text[i] == '\n') break; + } + } else if (!std::isspace(c)) { + // Allow blanks, but end the search if we find something else. + break; + } + } + return false; +} + #define VULKAN_VER(MAJOR, MINOR) ((MAJOR << 22) | (MINOR << 12)) #define SPIRV_VER(MAJOR, MINOR) ((MAJOR << 16) | (MINOR << 8)) diff --git a/source/spirv_target_env.h b/source/spirv_target_env.h index f3b0c2f6f2..4378f06fa4 100644 --- a/source/spirv_target_env.h +++ b/source/spirv_target_env.h @@ -16,6 +16,8 @@ #define SOURCE_SPIRV_TARGET_ENV_H_ #include +#include +#include #include "spirv-tools/libspirv.h" @@ -46,4 +48,9 @@ std::string spvLogStringForEnv(spv_target_env env); // occur to satisfy this limit. std::string spvTargetEnvList(const int pad, const int wrap); +// Reads the target environment from the header comments of disassembly. Returns +// true if valid name found, false otherwise. +bool spvReadEnvironmentFromText(const std::vector& text, + spv_target_env* env); + #endif // SOURCE_SPIRV_TARGET_ENV_H_ diff --git a/test/target_env_test.cpp b/test/target_env_test.cpp index 7917cbfb44..faffa8a1e0 100644 --- a/test/target_env_test.cpp +++ b/test/target_env_test.cpp @@ -170,5 +170,60 @@ INSTANTIATE_TEST_SUITE_P( {VK(99, 0), SPV(1, 0), false, SPV_ENV_UNIVERSAL_1_0}, })); -} // namespace +// A test case for parsing the text header of disassembly. +struct ParseEnvInDisassemblyCase { + std::string text; + bool success; // Expect to successfully parse? + spv_target_env env; // The parsed environment, if successful. +}; + +using TargetParseEnvInDisassemblyTest = + ::testing::TestWithParam; + +constexpr spv_target_env kSentinelEnv = SPV_ENV_OPENCL_2_2; + +TEST_P(TargetParseEnvInDisassemblyTest, Samples) { + const std::string& text = GetParam().text; + const std::vector text_vec(text.begin(), text.end()); + spv_target_env got_env = kSentinelEnv; + bool parsed = spvReadEnvironmentFromText(text_vec, &got_env); + EXPECT_EQ(parsed, GetParam().success); + EXPECT_EQ(got_env, GetParam().env) << '"' << text << '"'; +} + +INSTANTIATE_TEST_SUITE_P( + TargetTextParsing, TargetParseEnvInDisassemblyTest, + ValuesIn(std::vector{ + {"; Version: 1.0", true, SPV_ENV_UNIVERSAL_1_0}, + {"; Version: 1.1", true, SPV_ENV_UNIVERSAL_1_1}, + {"; Version: 1.2", true, SPV_ENV_UNIVERSAL_1_2}, + {"; Version: 1.3", true, SPV_ENV_UNIVERSAL_1_3}, + {"; Version: 1.4", true, SPV_ENV_UNIVERSAL_1_4}, + {"; Version: 1.5", true, SPV_ENV_UNIVERSAL_1_5}, + {"; Version: 1.6", true, SPV_ENV_UNIVERSAL_1_6}, + {"; Version: 1.7", false, kSentinelEnv}, + {"; Version: 1.8", false, kSentinelEnv}, + {"; Version: 1.9", false, kSentinelEnv}, + {"; Version: 2.0", false, kSentinelEnv}, + + // Check trailing text + {"; Version: 1.1\n", true, SPV_ENV_UNIVERSAL_1_1}, + {"; Version: 1.1\t", true, SPV_ENV_UNIVERSAL_1_1}, + {"; Version: 1.1 ", true, SPV_ENV_UNIVERSAL_1_1}, + {"; Version: 1.1x", true, SPV_ENV_UNIVERSAL_1_1}, + // Not a digit. + {"; Version: 1.10", false, kSentinelEnv}, + + // Unexpected prefix + {";Version: 1.1", false, kSentinelEnv}, + + // Leading spaces + {" \t ; Version: 1.1", true, SPV_ENV_UNIVERSAL_1_1}, + // Previous lines + {"; SPIR-V\n; Version: 1.1", true, SPV_ENV_UNIVERSAL_1_1}, + + // After a non-header line + {"OpCapability Shader\n; Version: 1.1", false, kSentinelEnv}})); + +} // anonymous namespace } // namespace spvtools diff --git a/tools/as/as.cpp b/tools/as/as.cpp index ab18aeb359..8e821701de 100644 --- a/tools/as/as.cpp +++ b/tools/as/as.cpp @@ -22,7 +22,7 @@ #include "tools/io.h" #include "tools/util/flags.h" -static const auto kDefaultEnvironment = "spv1.6"; +constexpr auto kDefaultTarget = SPV_ENV_UNIVERSAL_1_6; static const std::string kHelpText = R"(%s - Create a SPIR-V binary module from SPIR-V assembly text @@ -48,12 +48,13 @@ is used. )"; // clang-format off -FLAG_SHORT_bool( h, /* default_value= */ false, /* required= */ false); -FLAG_LONG_bool( help, /* default_value= */ false, /* required= */false); -FLAG_LONG_bool( version, /* default_value= */ false, /* required= */ false); -FLAG_LONG_bool( preserve_numeric_ids, /* default_value= */ false, /* required= */ false); -FLAG_SHORT_string(o, /* default_value= */ "", /* required= */ false); -FLAG_LONG_string( target_env, /* default_value= */ kDefaultEnvironment, /* required= */ false); +// flag name= default_value= required= +FLAG_SHORT_bool( h, false, false); +FLAG_LONG_bool( help, false, false); +FLAG_LONG_bool( version, false, false); +FLAG_LONG_bool( preserve_numeric_ids, false, false); +FLAG_SHORT_string(o, "", false); +FLAG_LONG_string( target_env, "", false); // clang-format on int main(int, const char** argv) { @@ -68,17 +69,8 @@ int main(int, const char** argv) { } if (flags::version.value()) { - spv_target_env target_env; - bool success = spvParseTargetEnv(kDefaultEnvironment, &target_env); - assert(success && "Default environment should always parse."); - if (!success) { - fprintf(stderr, - "error: invalid default target environment. Please report this " - "issue."); - return 1; - } printf("%s\n", spvSoftwareVersionDetailsString()); - printf("Target: %s\n", spvTargetEnvDescription(target_env)); + printf("Target: %s\n", spvTargetEnvDescription(kDefaultTarget)); return 0; } @@ -92,13 +84,6 @@ int main(int, const char** argv) { options |= SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS; } - spv_target_env target_env; - if (!spvParseTargetEnv(flags::target_env.value().c_str(), &target_env)) { - fprintf(stderr, "error: Unrecognized target env: %s\n", - flags::target_env.value().c_str()); - return 1; - } - if (flags::positional_arguments.size() != 1) { fprintf(stderr, "error: exactly one input file must be specified.\n"); return 1; @@ -108,6 +93,20 @@ int main(int, const char** argv) { std::vector contents; if (!ReadTextFile(inFile.c_str(), &contents)) return 1; + // Can only deduce target after the file has been read + spv_target_env target_env; + if (flags::target_env.value().empty()) { + if (!spvReadEnvironmentFromText(contents, &target_env)) { + // Revert to default version since deduction failed + target_env = kDefaultTarget; + } + } else if (!spvParseTargetEnv(flags::target_env.value().c_str(), + &target_env)) { + fprintf(stderr, "error: Unrecognized target env: %s\n", + flags::target_env.value().c_str()); + return 1; + } + spv_binary binary; spv_diagnostic diagnostic = nullptr; spv_context context = spvContextCreate(target_env);