From ae5c5f0c8146ae95aeae6f5082431148e8c27c0e Mon Sep 17 00:00:00 2001 From: Fraser Cormack Date: Tue, 21 Nov 2023 10:05:04 +0000 Subject: [PATCH 1/3] [spirv-ll] Return debug strings as optional values Knowing whether a debug string was actually provided is sometimes useful, e.g., for error handling. By returning an empty string we are unable to tell whether a string was provided and was empty, or whether one wasn't provided. Changing the return type of this function to `std::optional` can provide this info for us. --- modules/compiler/spirv-ll/include/spirv-ll/module.h | 4 ++-- modules/compiler/spirv-ll/source/builder_core.cpp | 7 +++---- modules/compiler/spirv-ll/source/module.cpp | 7 +++++-- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/modules/compiler/spirv-ll/include/spirv-ll/module.h b/modules/compiler/spirv-ll/include/spirv-ll/module.h index 3bedda3fd..92cca3ca8 100644 --- a/modules/compiler/spirv-ll/include/spirv-ll/module.h +++ b/modules/compiler/spirv-ll/include/spirv-ll/module.h @@ -392,8 +392,8 @@ class Module : public ModuleHeader { /// /// @param[in] id The ID the string is associated with. /// - /// @return The string or an empty one if the ID isn't found. - std::string getDebugString(spv::Id id) const; + /// @return The string or std::nullopt if the ID isn't found. + std::optional getDebugString(spv::Id id) const; /// @brief A type containing an LLVM debug location and the beginning of the /// range it corresponds to. diff --git a/modules/compiler/spirv-ll/source/builder_core.cpp b/modules/compiler/spirv-ll/source/builder_core.cpp index d56673f62..992485edb 100644 --- a/modules/compiler/spirv-ll/source/builder_core.cpp +++ b/modules/compiler/spirv-ll/source/builder_core.cpp @@ -104,9 +104,8 @@ llvm::Error Builder::create(const OpSource *op) { source += ", Version: " + std::to_string(op->Version()); if (op->wordCount() > 3) { - std::string file_path = module.getDebugString(op->File()); - if (!file_path.empty()) { - source += ", Source file: " + file_path + "\r\n"; + if (auto file_path = module.getDebugString(op->File())) { + source += ", Source file: " + file_path.value() + "\r\n"; } if (op->wordCount() > 4) { @@ -139,7 +138,7 @@ llvm::DIFile *Builder::getOrCreateDIFile(const OpLine *op_line) { return file; } - std::string filePath = module.getDebugString(op_line->File()); + std::string filePath = module.getDebugString(op_line->File()).value_or(""); std::string fileName = filePath.substr(filePath.find_last_of("\\/") + 1); std::string fileDir = filePath.substr(0, filePath.find_last_of("\\/")); diff --git a/modules/compiler/spirv-ll/source/module.cpp b/modules/compiler/spirv-ll/source/module.cpp index 12156690a..fd1b12123 100644 --- a/modules/compiler/spirv-ll/source/module.cpp +++ b/modules/compiler/spirv-ll/source/module.cpp @@ -213,8 +213,11 @@ bool spirv_ll::Module::addDebugString(spv::Id id, const std::string &string) { return DebugStrings.try_emplace(id, string).second; } -std::string spirv_ll::Module::getDebugString(spv::Id id) const { - return DebugStrings.lookup(id); +std::optional spirv_ll::Module::getDebugString(spv::Id id) const { + if (auto iter = DebugStrings.find(id); iter != DebugStrings.end()) { + return iter->getSecond(); + } + return std::nullopt; } void spirv_ll::Module::setCurrentOpLineRange( From 1cb318d320ccba216e395582130e414a4ddbca4d Mon Sep 17 00:00:00 2001 From: Fraser Cormack Date: Tue, 21 Nov 2023 10:09:10 +0000 Subject: [PATCH 2/3] [spirv-ll] Associate debug subprograms with function IDs Instead of mapping from each llvm::Function to its debug subprogram, we want to instead associate the original SPIR-V ID representing that function. This is primarily to help the upcoming translation of the DebugInfo extensions, as the `DebugFunction` instructions take a *forward reference* to a function (meaning we only have its ID), and we want to associate the translated subprogram with the function when the function is finally processed later in the module. --- .../spirv-ll/include/spirv-ll/module.h | 12 ++++++------ .../compiler/spirv-ll/source/builder_core.cpp | 18 +++++++++++++++--- modules/compiler/spirv-ll/source/module.cpp | 8 ++++---- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/modules/compiler/spirv-ll/include/spirv-ll/module.h b/modules/compiler/spirv-ll/include/spirv-ll/module.h index 92cca3ca8..4ee881c91 100644 --- a/modules/compiler/spirv-ll/include/spirv-ll/module.h +++ b/modules/compiler/spirv-ll/include/spirv-ll/module.h @@ -455,18 +455,18 @@ class Module : public ModuleHeader { /// @brief Add a DISubprogram to the module, and associate it with an ID. /// - /// @param function Function to associate the `DISubprogram` with. + /// @param function_id Function to associate the `DISubprogram` with. /// @param function_scope `DISubprogram` to add to the module. - void addDebugFunctionScope(llvm::Function *function, + void addDebugFunctionScope(spv::Id function_id, llvm::DISubprogram *function_scope); /// @brief Get the `DISubProgram` associated with a given function /// - /// @param function Pointer to the function to look up. + /// @param function_id ID of the function to look up. /// /// @return Pointer to the `DISubprogram` associated with the function, or /// nullptr if there isn't one - llvm::DISubprogram *getDebugFunctionScope(llvm::Function *function) const; + llvm::DISubprogram *getDebugFunctionScope(spv::Id function_id) const; /// @brief Store control mask metadata created by OpLoopMerge /// @@ -1054,8 +1054,8 @@ class Module : public ModuleHeader { std::optional CurrentOpLineRange; /// @brief Map of BasicBlock to associated `DILexicalBlock`. llvm::DenseMap LexicalBlocks; - /// @brief Map of function to associated `DISubprogram`. - llvm::DenseMap FunctionScopes; + /// @brief Map of function IDs to their associated `DISubprogram`s. + llvm::DenseMap FunctionScopes; /// @brief A mapping between spirv block id's and LLVM loop control mask. /// /// For each entry, the LLVM block generated by the spirv block `Id` will diff --git a/modules/compiler/spirv-ll/source/builder_core.cpp b/modules/compiler/spirv-ll/source/builder_core.cpp index 992485edb..96702fbf0 100644 --- a/modules/compiler/spirv-ll/source/builder_core.cpp +++ b/modules/compiler/spirv-ll/source/builder_core.cpp @@ -164,13 +164,25 @@ llvm::DICompileUnit *Builder::getOrCreateDICompileUnit(const OpLine *op_line) { llvm::DISubprogram *Builder::getOrCreateDebugFunctionScope( llvm::Function *function, const OpLine *op_line) { - if (auto *function_scope = module.getDebugFunctionScope(function)) { + if (!function) { + return nullptr; + } + const OpFunction *opFunction = module.get(function); + // If we have a llvm::Function we should have an OpFunction. + SPIRV_LL_ASSERT_PTR(opFunction); + spv::Id function_id = opFunction->IdResult(); + + if (auto *function_scope = module.getDebugFunctionScope(function_id)) { + // If we've created the scope before creating the function, link the two + // together here if we haven't already. + if (!function->getSubprogram()) { + function->setSubprogram(function_scope); + } return function_scope; } llvm::SmallVector dbg_function_param_types; - const OpFunction *opFunction = module.get(function); const OpTypeFunction *opTypeFunction = module.get(opFunction->FunctionType()); @@ -194,7 +206,7 @@ llvm::DISubprogram *Builder::getOrCreateDebugFunctionScope( function->setSubprogram(function_scope); // Track this sub-program for later - module.addDebugFunctionScope(function, function_scope); + module.addDebugFunctionScope(function_id, function_scope); return function_scope; } diff --git a/modules/compiler/spirv-ll/source/module.cpp b/modules/compiler/spirv-ll/source/module.cpp index fd1b12123..3ff893e69 100644 --- a/modules/compiler/spirv-ll/source/module.cpp +++ b/modules/compiler/spirv-ll/source/module.cpp @@ -254,13 +254,13 @@ llvm::DILexicalBlock *spirv_ll::Module::getLexicalBlock( } void spirv_ll::Module::addDebugFunctionScope( - llvm::Function *function, llvm::DISubprogram *function_scope) { - FunctionScopes.insert({function, function_scope}); + spv::Id function_id, llvm::DISubprogram *function_scope) { + FunctionScopes.insert({function_id, function_scope}); } llvm::DISubprogram *spirv_ll::Module::getDebugFunctionScope( - llvm::Function *function) const { - auto found = FunctionScopes.find(function); + spv::Id function_id) const { + auto found = FunctionScopes.find(function_id); return found != FunctionScopes.end() ? found->getSecond() : nullptr; } From d21de0e5d63211fd70f2bf88fd8cab164cf572aa Mon Sep 17 00:00:00 2001 From: Fraser Cormack Date: Tue, 21 Nov 2023 10:14:47 +0000 Subject: [PATCH 3/3] [spirv-ll] Add method to translate StorageClass to addrspace This will be used by the DebugInfo translations. This also makes failures in the translation more robust by making the functions return an error if the storage class is unknown/unsupported, rather than aborting. --- .../spirv-ll/include/spirv-ll/module.h | 9 ++- .../compiler/spirv-ll/source/builder_core.cpp | 4 +- modules/compiler/spirv-ll/source/module.cpp | 79 +++++++++++-------- .../spirv-ll/test/spvasm/CMakeLists.txt | 11 ++- .../unsupported_storage_class.spvasm | 38 +++++++++ 5 files changed, 104 insertions(+), 37 deletions(-) create mode 100644 modules/compiler/spirv-ll/test/spvasm/invalid-spir-v/unsupported_storage_class.spvasm diff --git a/modules/compiler/spirv-ll/include/spirv-ll/module.h b/modules/compiler/spirv-ll/include/spirv-ll/module.h index 4ee881c91..3a39ed6c6 100644 --- a/modules/compiler/spirv-ll/include/spirv-ll/module.h +++ b/modules/compiler/spirv-ll/include/spirv-ll/module.h @@ -759,10 +759,15 @@ class Module : public ModuleHeader { /// @param member_id ID of the member type that was defined. void updateIncompleteStruct(spv::Id member_id); + /// @brief Return the LLVM address space for the given storage class, or an + /// error if the storage class is unknown/unsupported. + llvm::Expected translateStorageClassToAddrSpace( + uint32_t storage_class) const; + /// @brief Add a complete pointer. /// /// @param pointer_type OpCode object that describes the pointer type. - void addCompletePointer(const OpTypePointer *pointer_type); + llvm::Error addCompletePointer(const OpTypePointer *pointer_type); /// @brief Add an incomplete pointer and its missing type IDs to the module. /// @@ -774,7 +779,7 @@ class Module : public ModuleHeader { /// @brief Update an incomplete pointer type with a newly defined type. /// /// @param type_id ID of the type that was defined. - void updateIncompletePointer(spv::Id type_id); + llvm::Error updateIncompletePointer(spv::Id type_id); /// @brief Add id, image and sampler to the module. /// diff --git a/modules/compiler/spirv-ll/source/builder_core.cpp b/modules/compiler/spirv-ll/source/builder_core.cpp index 96702fbf0..f200f29e0 100644 --- a/modules/compiler/spirv-ll/source/builder_core.cpp +++ b/modules/compiler/spirv-ll/source/builder_core.cpp @@ -708,7 +708,9 @@ llvm::Error Builder::create(const OpTypePointer *op) { if (module.isForwardPointer(typeId)) { module.addIncompletePointer(op, typeId); } else { - module.addCompletePointer(op); + if (auto err = module.addCompletePointer(op)) { + return err; + } } return llvm::Error::success(); } diff --git a/modules/compiler/spirv-ll/source/module.cpp b/modules/compiler/spirv-ll/source/module.cpp index 3ff893e69..28461eeb1 100644 --- a/modules/compiler/spirv-ll/source/module.cpp +++ b/modules/compiler/spirv-ll/source/module.cpp @@ -15,10 +15,13 @@ // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception #include +#include #include #include +#include #include #include +#include #include @@ -576,64 +579,71 @@ void spirv_ll::Module::updateIncompleteStruct(spv::Id member_id) { } } -void spirv_ll::Module::addCompletePointer(const OpTypePointer *op) { - spv::Id typeId = op->Type(); - SPIRV_LL_ASSERT(!isForwardPointer(typeId), "typeId is a forward pointer"); - llvm::Type *type = getLLVMType(typeId); - SPIRV_LL_ASSERT_PTR(type); - - // Pointer to void type isn't legal in llvm, so substitute char* in such - // cases. - if (type->isVoidTy()) { - type = llvm::Type::getInt8Ty(llvmModule->getContext()); - } - - int AddressSpace = 0; - - switch (op->StorageClass()) { +llvm::Expected spirv_ll::Module::translateStorageClassToAddrSpace( + uint32_t storage_class) const { + switch (storage_class) { + default: + return makeStringError("Unknown StorageClass " + + std::to_string(storage_class)); case spv::StorageClassFunction: case spv::StorageClassPrivate: case spv::StorageClassAtomicCounter: case spv::StorageClassInput: case spv::StorageClassOutput: // private - break; + return 0; case spv::StorageClassUniform: case spv::StorageClassCrossWorkgroup: case spv::StorageClassImage: case spv::StorageClassStorageBuffer: // global - AddressSpace = 1; - break; + return 1; case spv::StorageClassUniformConstant: case spv::StorageClassPushConstant: // constant - AddressSpace = 2; - break; + return 2; case spv::StorageClassWorkgroup: // local - AddressSpace = 3; - break; - case spv::StorageClassGeneric: { + return 3; + case spv::StorageClassGeneric: if (isExtensionEnabled("SPV_codeplay_usm_generic_storage_class")) { - AddressSpace = 0; - } else { - AddressSpace = 4; + return 0; } - } break; - default: - llvm_unreachable("unknown StorageClass"); + return 4; + } +} + +llvm::Error spirv_ll::Module::addCompletePointer(const OpTypePointer *op) { + spv::Id type_id = op->Type(); + SPIRV_LL_ASSERT(!isForwardPointer(type_id), "type_id is a forward pointer"); + llvm::Type *type = getLLVMType(type_id); + SPIRV_LL_ASSERT_PTR(type); + + // Pointer to void type isn't legal in llvm, so substitute char* in such + // cases. + if (type->isVoidTy()) { + type = llvm::Type::getInt8Ty(llvmModule->getContext()); + } + + auto addrspace_or_error = + translateStorageClassToAddrSpace(op->StorageClass()); + if (auto err = addrspace_or_error.takeError()) { + return err; } - llvm::Type *pointer_type = llvm::PointerType::get(type, AddressSpace); + llvm::Type *pointer_type = + llvm::PointerType::get(type, addrspace_or_error.get()); addID(op->IdResult(), op, pointer_type); if (isForwardPointer(op->IdResult())) { removeForwardPointer(op->IdResult()); updateIncompleteStruct(op->IdResult()); - updateIncompletePointer(op->IdResult()); + if (auto err = updateIncompletePointer(op->IdResult())) { + return err; + } } + return llvm::Error::success(); } void spirv_ll::Module::addIncompletePointer(const OpTypePointer *pointer_type, @@ -641,13 +651,15 @@ void spirv_ll::Module::addIncompletePointer(const OpTypePointer *pointer_type, IncompletePointers.insert({pointer_type, missing_type}); } -void spirv_ll::Module::updateIncompletePointer(spv::Id type_id) { +llvm::Error spirv_ll::Module::updateIncompletePointer(spv::Id type_id) { for (auto it = IncompletePointers.begin(), end = IncompletePointers.end(); it != end;) { // if the newly declared type id is found in an incomplete pointer, if (type_id == it->second) { // complete the pointer - addCompletePointer(it->first); + if (auto err = addCompletePointer(it->first)) { + return err; + } // and remove the now completed pointer from the map IncompletePointers.erase(it); it = IncompletePointers.begin(); @@ -655,6 +667,7 @@ void spirv_ll::Module::updateIncompletePointer(spv::Id type_id) { ++it; } } + return llvm::Error::success(); } void spirv_ll::Module::addSampledImage(spv::Id id, llvm::Value *image, diff --git a/modules/compiler/spirv-ll/test/spvasm/CMakeLists.txt b/modules/compiler/spirv-ll/test/spvasm/CMakeLists.txt index b3e8d13be..e8b59afea 100644 --- a/modules/compiler/spirv-ll/test/spvasm/CMakeLists.txt +++ b/modules/compiler/spirv-ll/test/spvasm/CMakeLists.txt @@ -2201,11 +2201,16 @@ set(SPVASM_V1_1_FILES opencl_debug_info_100_clean.spvasm ) +set(SPVASM_V1_5_FILES + invalid-spir-v/unsupported_storage_class.spvasm +) + set(UNVERIFIABLE_SPVASM_FILES # Valid SPIR-V as per the extension, but even current versions of spirv-val # don't recognize it. intel_arbitrary_precision_integers.spvasm invalid-spir-v/invalid_storage_class.spvasm + invalid-spir-v/unsupported_storage_class.spvasm codeplay/opencl_usm_generic_address_space.spvasm codeplay/opencl_group_async_copy_2d2d.spvasm codeplay/opencl_group_async_copy_3d3d.spvasm @@ -2216,7 +2221,7 @@ set(UNVERIFIABLE_SPVASM_FILES # Remove obsolete lit test inputs from the binary directory. file(GLOB obsoleteTests ${CMAKE_CURRENT_BINARY_DIR}/*.spvasm) -foreach(testFile ${SPVASM_FILES} ${SPVASM_V1_1_FILES}) +foreach(testFile ${SPVASM_FILES} ${SPVASM_V1_1_FILES} ${SPVASM_V1_5_FILES}) list(REMOVE_ITEM obsoleteTests ${CMAKE_CURRENT_BINARY_DIR}/${testFile}) endforeach() if(obsoleteTests) @@ -2287,6 +2292,10 @@ foreach(spvasm ${SPVASM_V1_1_FILES}) ca_assemble_spirv_ll_lit_test(${spvasm} TGT_ENV spv1.1) endforeach() +foreach(spvasm ${SPVASM_V1_5_FILES}) + ca_assemble_spirv_ll_lit_test(${spvasm} TGT_ENV spv1.5) +endforeach() + get_property(LitTestFiles GLOBAL PROPERTY CA_SPIRV_LL_SPVASM_TEST_FILES) add_custom_target(spirv-ll-spvasm-lit DEPENDS ${LitTestFiles}) diff --git a/modules/compiler/spirv-ll/test/spvasm/invalid-spir-v/unsupported_storage_class.spvasm b/modules/compiler/spirv-ll/test/spvasm/invalid-spir-v/unsupported_storage_class.spvasm new file mode 100644 index 000000000..4d2d41a97 --- /dev/null +++ b/modules/compiler/spirv-ll/test/spvasm/invalid-spir-v/unsupported_storage_class.spvasm @@ -0,0 +1,38 @@ +; Copyright (C) Codeplay Software Limited +; +; Licensed under the Apache License, Version 2.0 (the "License") with LLVM +; Exceptions; you may not use this file except in compliance with the License. +; You may obtain a copy of the License at +; +; https://github.com/codeplaysoftware/oneapi-construction-kit/blob/main/LICENSE.txt +; +; Unless required by applicable law or agreed to in writing, software +; distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +; WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +; License for the specific language governing permissions and limitations +; under the License. +; +; SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +; This is invalid SPIR-V as we should be declaring the +; PhysicalStorageBufferAddresses capability before using the +; PhysicalStorageBuffer storage class, but we specifically want to test the +; error handling of the translation of OpTypePointer with the unsupported +; storage class. If we added 'OpCapability PhysicalStorageBufferAddresses' we'd +; see an error about the unsupported capability instead. + +; RUN: %if online-spirv-as %{ spirv-as --target-env spv1.5 -o %spv_file_s %s %} +; RUN: not spirv-ll-tool -a OpenCL -b 64 %spv_file_s 2>&1 | FileCheck %s + +; CHECK: Unknown StorageClass 5349 + + OpCapability Addresses + OpCapability Linkage + OpCapability Kernel + OpCapability Int64 + OpCapability Int8 + %1 = OpExtInstImport "OpenCL.std" + OpMemoryModel Physical64 OpenCL + OpSource OpenCL_CPP 100000 + %uint = OpTypeInt 32 0 +%_ptr_Function_uint = OpTypePointer PhysicalStorageBuffer %uint