From 1a6d31e9e81f848a922fff8d9b090a5830673ec5 Mon Sep 17 00:00:00 2001 From: Fraser Cormack Date: Tue, 14 Nov 2023 15:04:29 +0000 Subject: [PATCH] [spirv-ll] Fix various debug info generation issues There were a few noted and unnoted issues with our generation of debug info from `OpLine` opcodes. * Empty line ranges could crash the translator. * `OpLine`s placed in empty basic blocks could crash the translator. * `OpLabel` opcodes placed before `OpFunction` or `OpLabel` instructions (anywhere outside of a block) wouldn't influence the line/column of those entities, as is the intention. * DISubprograms weren't attached to functions. With this commit, all of these problems are resolved. There is some refactoring here to make the code easier to work with. Firstly, we change the representation of the current line range. This improves the internal representation of the internally tracked current "open" line range (i.e., one begun with `OpLine`). We were previously using special undocumented variables to denote that the current range was closed. This is better expressed as a `std::optional`, which is set to a valid pair when the range is open and `std::nullopt` when it's closed. We also close line ranges more in adherence to the SPIR-V specification. We used to close open line ranges when starting a new basic block, or ending a function. The specification says that ranges close when they hit a *block termination instruction*: one of a certain set of opcodes that end a block or function. Note that the one such opcode we *don't* process is `OpTerminateInvocation`, but we have no support for that as it's too new (SPIR-V 1.6 and above). Secondly, we process blocks targeted by branches "on demand". Before, we used to collect branch-like instructions (`OpBranch`, `OpBranchConditional`, and `OpSwitch`) and process them once the function was complete. This was harder to intuit, harder to maintain a self-consistent builder class (the special casing was done outside of its control) and was harder to maintain coherent set of debug scopes. Now, if we process a branch instruction which forward-references a block, we create that basic block on the fly. When we come to the OpLabel for that block, we'll already have a sensible block. The only fixing up is for cosmetic reasons: we move the block to the end of the function to better maintain the layout of the SPIR-V program. This also removes the special casing for branches when closing a line range. This was necessary before because there wasn't actually a branch at the time we closed the range, so the current iterators weren't quite right and we had to pre-empty the insertion of a branch later. --- .../spirv-ll/include/spirv-ll/builder.h | 21 +- .../spirv-ll/include/spirv-ll/module.h | 29 +- .../compiler/spirv-ll/source/builder_core.cpp | 273 ++++++++++++------ modules/compiler/spirv-ll/source/context.cpp | 30 +- modules/compiler/spirv-ll/source/module.cpp | 14 +- .../spirv-ll/test/spvasm/CMakeLists.txt | 1 + .../spirv-ll/test/spvasm/debug_info.spvasm | 13 +- .../spvasm/debug_info_funcs_and_blocks.spvasm | 76 +++++ 8 files changed, 324 insertions(+), 133 deletions(-) create mode 100644 modules/compiler/spirv-ll/test/spvasm/debug_info_funcs_and_blocks.spvasm diff --git a/modules/compiler/spirv-ll/include/spirv-ll/builder.h b/modules/compiler/spirv-ll/include/spirv-ll/builder.h index e62a9eeac..ed05446ab 100644 --- a/modules/compiler/spirv-ll/include/spirv-ll/builder.h +++ b/modules/compiler/spirv-ll/include/spirv-ll/builder.h @@ -326,7 +326,7 @@ class Builder { void accessChain(OpCode const &opc); /// @brief Check if an OpLine range is in progress and end it if there is - void checkEndOpLineRange(bool is_branch = false); + void checkEndOpLineRange(); /// @brief Return a `DIType` object that represents the given type /// @@ -335,6 +335,22 @@ class Builder { /// @return pointer to `DIType` derived from the given `Type` llvm::DIType *getDIType(llvm::Type *type); + /// @brief Gets (or creates) a DIFile for the given OpLine. + llvm::DIFile *getOrCreateDIFile(const OpLine *op_line); + + /// @brief Gets (or creates) a DICompileUnit for the given OpLine. + llvm::DICompileUnit *getOrCreateDICompileUnit(const OpLine *op_line); + + /// @brief Gets (or creates) a DISubprogram for the given function and + /// OpLine. + llvm::DISubprogram *getOrCreateDebugFunctionScope(llvm::Function *function, + const OpLine *op_line); + + /// @brief Creates the beginning of a line range for the given OpLine, + /// contained within the basic block. + Module::LineRangeBeginTy createLineRangeBegin(const OpLine *op_line, + llvm::BasicBlock &bb); + /// @brief Add debug metadata to the appropriate instructions void addDebugInfoToModule(); @@ -342,6 +358,9 @@ class Builder { /// SpvBuilder void finalizeMetadata(); + /// @brief Gets (or creates) the BasicBlock for a spv::Id OpLabel. + llvm::BasicBlock *getOrCreateBasicBlock(spv::Id label); + /// @brief Generates code in a basic block to initialize a builtin variable. /// /// @param builtin SPIR-V builtin enum denoting which builtin to initialize. diff --git a/modules/compiler/spirv-ll/include/spirv-ll/module.h b/modules/compiler/spirv-ll/include/spirv-ll/module.h index b3a9ff4c0..d46e40de7 100644 --- a/modules/compiler/spirv-ll/include/spirv-ll/module.h +++ b/modules/compiler/spirv-ll/include/spirv-ll/module.h @@ -393,12 +393,23 @@ class Module : public ModuleHeader { /// @return The string or an empty one if the ID isn't found. std::string getDebugString(spv::Id id) const; - /// @brief Set basic block iterator at start of the current OpLine range. + /// @brief A type containing an LLVM debug location and the beginning of the + /// range it corresponds to. + struct LineRangeBeginTy { + const OpLine *op_line; + llvm::DILocation *loc; + llvm::BasicBlock::iterator range_begin = llvm::BasicBlock::iterator(); + }; + + /// @brief Opens up a new OpLine range, setting it to the current one. /// - /// @param location DILocation containing the line/column info. - /// @param iter Basic block iterator at the start of the new range. - void setCurrentOpLineRange(llvm::DILocation *location, - llvm::BasicBlock::iterator iter); + /// Does *not* close the current one. Call closeCurrentOpLineRange() first. + /// + /// @param range_begin LineRangeBeginTy information about the range to begin. + void setCurrentOpLineRange(const LineRangeBeginTy &range_begin); + + /// @brief Closes (i.e., clears) the current OpLine range. + void closeCurrentOpLineRange(); /// @brief Add a completed OpLine range to the module. /// @@ -424,8 +435,7 @@ class Module : public ModuleHeader { /// /// @return Pair containing `DILocation` and iterator range, location will be /// nullptr if there isn't an ongoing range - std::pair - getCurrentOpLineRange() const; + std::optional getCurrentOpLineRange() const; /// @brief Add a basic block and associated lexical block to the module. /// @@ -1037,8 +1047,9 @@ class Module : public ModuleHeader { /// translated in its entirety, so we can't construct the `iterator_range` /// until that's happened but we still need to store the range. OpLineRangeMap OpLineRanges; - /// @brief DILocation/basic block iterator pair to store current OpLine range. - std::pair CurrentOpLineRange; + /// @brief DILocation/basic block iterator pair to store the beginning of the + /// current OpLine range. + std::optional CurrentOpLineRange; /// @brief Map of BasicBlock to associated `DILexicalBlock`. llvm::DenseMap LexicalBlocks; /// @brief Map of function to associated `DISubprogram`. diff --git a/modules/compiler/spirv-ll/source/builder_core.cpp b/modules/compiler/spirv-ll/source/builder_core.cpp index 597c72b1d..581d2e80d 100644 --- a/modules/compiler/spirv-ll/source/builder_core.cpp +++ b/modules/compiler/spirv-ll/source/builder_core.cpp @@ -25,6 +25,7 @@ #include #include #include +#include #include #include #include @@ -131,104 +132,155 @@ cargo::optional Builder::create(const OpString *op) { return cargo::nullopt; } -template <> -cargo::optional Builder::create(const OpLine *op) { - // having an OpLine before you start a function is valid, but we won't be able - // to do anything with it - if (!getCurrentFunction()) { - return cargo::nullopt; +llvm::DIFile *Builder::getOrCreateDIFile(const OpLine *op_line) { + if (llvm::DIFile *file = module.getDIFile()) { + return file; } - checkEndOpLineRange(); + std::string filePath = module.getDebugString(op_line->File()); + std::string fileName = filePath.substr(filePath.find_last_of("\\/") + 1); + std::string fileDir = filePath.substr(0, filePath.find_last_of("\\/")); - llvm::DIFile *file = module.getDIFile(); + llvm::DIFile *file = DIBuilder.createFile(fileName, fileDir); - if (!file) { - std::string filePath = module.getDebugString(op->File()); - std::string fileName = filePath.substr(filePath.find_last_of("\\/") + 1); - std::string fileDir = filePath.substr(0, filePath.find_last_of("\\/")); + module.setDIFile(file); + return file; +} - file = DIBuilder.createFile(fileName, fileDir); - module.setDIFile(file); +llvm::DICompileUnit *Builder::getOrCreateDICompileUnit(const OpLine *op_line) { + if (llvm::DICompileUnit *compile_unit = module.getCompileUnit()) { + return compile_unit; } - llvm::DICompileUnit *compile_unit = module.getCompileUnit(); + auto *di_file = getOrCreateDIFile(op_line); - if (!compile_unit) { - compile_unit = DIBuilder.createCompileUnit(llvm::dwarf::DW_LANG_OpenCL, - file, "", false, "", 0, ""); - module.setCompileUnit(compile_unit); + llvm::DICompileUnit *compile_unit = DIBuilder.createCompileUnit( + llvm::dwarf::DW_LANG_OpenCL, di_file, "", false, "", 0, ""); + + module.setCompileUnit(compile_unit); + return compile_unit; +} + +llvm::DISubprogram *Builder::getOrCreateDebugFunctionScope( + llvm::Function *function, const OpLine *op_line) { + if (auto *function_scope = module.getDebugFunctionScope(function)) { + return function_scope; } - llvm::DISubprogram *function_scope = - module.getDebugFunctionScope(IRBuilder.GetInsertBlock()->getParent()); + llvm::SmallVector dbg_function_types; - if (!function_scope) { - llvm::Function *function = IRBuilder.GetInsertBlock()->getParent(); + for (auto &arg : function->args()) { + dbg_function_types.push_back(getDIType(arg.getType())); + } - llvm::SmallVector dbg_function_types; + llvm::DISubroutineType *dbg_function_type = DIBuilder.createSubroutineType( + DIBuilder.getOrCreateTypeArray(dbg_function_types)); - for (auto &arg : function->args()) { - dbg_function_types.push_back(getDIType(arg.getType())); - } + auto *di_file = getOrCreateDIFile(op_line); + auto *di_compile_unit = getOrCreateDICompileUnit(op_line); + + // TODO: pass mangled name here when we're mangling names + auto *function_scope = DIBuilder.createFunction( + di_compile_unit, function->getName(), function->getName(), di_file, + op_line->Line(), dbg_function_type, 1, llvm::DINode::FlagZero, + llvm::DISubprogram::SPFlagDefinition); + + // Set the function's debug sub-program + function->setSubprogram(function_scope); - llvm::DISubroutineType *dbg_function_type = DIBuilder.createSubroutineType( - DIBuilder.getOrCreateTypeArray(dbg_function_types)); + // Track this sub-program for later + module.addDebugFunctionScope(function, function_scope); + + return function_scope; +} - // TODO: pass mangled name here when we're mangling names - function_scope = DIBuilder.createFunction( - compile_unit, function->getName(), function->getName(), file, - op->Line(), dbg_function_type, 1, llvm::DINode::FlagZero, - llvm::DISubprogram::SPFlagDefinition); +Module::LineRangeBeginTy Builder::createLineRangeBegin(const OpLine *op_line, + llvm::BasicBlock &bb) { + // Get or create the DILexicalBlock for this basic block. + llvm::DILexicalBlock *di_block = module.getLexicalBlock(&bb); - module.addDebugFunctionScope(function, function_scope); + if (!di_block) { + auto *di_file = getOrCreateDIFile(op_line); + auto *function_scope = + getOrCreateDebugFunctionScope(bb.getParent(), op_line); + di_block = DIBuilder.createLexicalBlock(function_scope, di_file, + op_line->Line(), op_line->Column()); + + module.addLexicalBlock(&bb, di_block); } - llvm::DILexicalBlock *block = - module.getLexicalBlock(IRBuilder.GetInsertBlock()); + // If there aren't any instructions in the basic block yet just go from the + // start of the block. + llvm::BasicBlock::iterator iter = + bb.empty() ? bb.begin() : bb.back().getIterator(); + + auto *di_loc = llvm::DILocation::get(di_block->getContext(), op_line->Line(), + op_line->Column(), di_block); + + return Module::LineRangeBeginTy{op_line, di_loc, iter}; +} + +template <> +cargo::optional Builder::create(const OpLine *op) { + // Close the current range, if applicable. + checkEndOpLineRange(); + + llvm::Function *current_function = getCurrentFunction(); - if (!block) { - block = DIBuilder.createLexicalBlock(function_scope, file, op->Line(), - op->Column()); + // Having an OpLine before you start a function is valid. We'll attach the + // current range onto the function when we create it. + if (!current_function) { + // Create a location within the current file + auto *loc = llvm::DILocation::get(*context.llvmContext, op->Line(), + op->Column(), module.getDIFile()); + module.setCurrentOpLineRange(Module::LineRangeBeginTy{op, loc}); - module.addLexicalBlock(IRBuilder.GetInsertBlock(), block); + return cargo::nullopt; } - llvm::BasicBlock::iterator iter; + llvm::DISubprogram *function_scope = + getOrCreateDebugFunctionScope(current_function, op); - // if there aren't any instructions in the basic block yet just go from the - // start - if (IRBuilder.GetInsertBlock()->empty()) { - iter = IRBuilder.GetInsertBlock()->begin(); - } else { - iter = IRBuilder.GetInsertBlock()->back().getIterator(); + auto *current_block = IRBuilder.GetInsertBlock(); + + // Having an OpLine before you start a block is valid. We'll attach the + // current range onto the block when we create it. + if (!current_block) { + // Create a location within the current function. + auto *loc = llvm::DILocation::get(*context.llvmContext, op->Line(), + op->Column(), function_scope); + module.setCurrentOpLineRange(Module::LineRangeBeginTy{op, loc}); + + return cargo::nullopt; } - auto *loc = llvm::DILocation::get(block->getContext(), op->Line(), - op->Column(), block); - module.setCurrentOpLineRange(loc, iter); + module.setCurrentOpLineRange(createLineRangeBegin(op, *current_block)); + return cargo::nullopt; } -void Builder::checkEndOpLineRange(bool is_branch) { - if (module.getCurrentOpLineRange().first) { - llvm::BasicBlock::iterator begin_iter = - module.getCurrentOpLineRange().second; +void Builder::checkEndOpLineRange() { + auto line_range = module.getCurrentOpLineRange(); + if (!line_range) { + return; + } - llvm::BasicBlock::iterator end_iter; + auto [op_line, loc, begin_iter] = *line_range; - if (is_branch) { - end_iter = IRBuilder.GetInsertBlock()->end(); - } else { - end_iter = IRBuilder.GetInsertBlock()->back().getIterator(); - } + llvm::BasicBlock *current_block = IRBuilder.GetInsertBlock(); - auto range = std::make_pair(begin_iter, end_iter); + // A closed range on an empty block means nothing. + if (current_block && !current_block->empty()) { + llvm::BasicBlock::iterator end_iter = + IRBuilder.GetInsertBlock()->back().getIterator(); - module.addCompleteOpLineRange(module.getCurrentOpLineRange().first, range); + auto range = std::make_pair(begin_iter, end_iter); - module.setCurrentOpLineRange(nullptr, llvm::BasicBlock::iterator()); + module.addCompleteOpLineRange(loc, range); } + + // Close off the current range. + module.closeCurrentOpLineRange(); } template <> @@ -2033,6 +2085,13 @@ cargo::optional Builder::create(const OpFunction *op) { function->setCallingConv(CC); setCurrentFunction(function); + // If there's a line range currently open at this point, create and attach + // the DISubprogram for this function. If there isn't, we'll generate one on + // the fly when we hit an OpLine. + if (auto current_range = module.getCurrentOpLineRange()) { + getOrCreateDebugFunctionScope(function, current_range->op_line); + } + module.addID(op->IdResult(), op, function); return cargo::nullopt; } @@ -5954,16 +6013,37 @@ cargo::optional Builder::create( return cargo::nullopt; } +llvm::BasicBlock *Builder::getOrCreateBasicBlock(spv::Id label) { + auto *bb = llvm::dyn_cast_or_null(module.getValue(label)); + if (bb) { + return bb; + } + + llvm::Function *current_function = getCurrentFunction(); + SPIRV_LL_ASSERT_PTR(current_function); + + bb = llvm::BasicBlock::Create(*context.llvmContext, module.getName(label), + current_function); + module.addID(label, /*Op*/ nullptr, bb); + return bb; +} + template <> cargo::optional Builder::create(const OpLabel *op) { llvm::Function *current_function = getCurrentFunction(); SPIRV_LL_ASSERT_PTR(current_function); - // This signifies the end of a block, so close out any existing OpLine range. - checkEndOpLineRange(true); + auto *bb = getOrCreateBasicBlock(op->IdResult()); + SPIRV_LL_ASSERT_PTR(bb); - llvm::BasicBlock *bb = llvm::BasicBlock::Create( - *context.llvmContext, module.getName(op->IdResult()), current_function); + // If we've already created this basic block before reaching the OpLabel + // (through a forward reference), then it's in the "wrong" place in terms of + // the linear layout of the function. Remove and re-insert the basic block at + // the end of the current function. + if (bb->getIterator() != std::prev(current_function->end())) { + bb->removeFromParent(); + current_function->insert(current_function->end(), bb); + } IRBuilder.SetInsertPoint(bb); @@ -5977,31 +6057,34 @@ cargo::optional Builder::create(const OpLabel *op) { generateSpecConstantOps(); } + if (auto current_range = module.getCurrentOpLineRange()) { + module.setCurrentOpLineRange( + createLineRangeBegin(current_range->op_line, *bb)); + } + module.addID(op->IdResult(), op, bb); return cargo::nullopt; } template <> cargo::optional Builder::create(const OpBranch *op) { - llvm::Value *label = module.getValue(op->TargetLabel()); - SPIRV_LL_ASSERT_PTR(label); - llvm::BasicBlock *bb = llvm::dyn_cast(label); + llvm::BasicBlock *bb = getOrCreateBasicBlock(op->TargetLabel()); SPIRV_LL_ASSERT_PTR(bb); IRBuilder.CreateBr(bb); + + // This instruction ends a block. + checkEndOpLineRange(); + return cargo::nullopt; } template <> cargo::optional Builder::create( const OpBranchConditional *op) { - llvm::Value *true_label = module.getValue(op->TrueLabel()); - SPIRV_LL_ASSERT_PTR(true_label); - llvm::BasicBlock *true_bb = llvm::dyn_cast(true_label); + llvm::BasicBlock *true_bb = getOrCreateBasicBlock(op->TrueLabel()); SPIRV_LL_ASSERT_PTR(true_bb); - llvm::Value *false_label = module.getValue(op->FalseLabel()); - SPIRV_LL_ASSERT_PTR(false_label); - llvm::BasicBlock *false_bb = llvm::dyn_cast(false_label); + llvm::BasicBlock *false_bb = getOrCreateBasicBlock(op->FalseLabel()); SPIRV_LL_ASSERT_PTR(false_bb); llvm::Value *cond = module.getValue(op->Condition()); SPIRV_LL_ASSERT_PTR(cond); @@ -6037,6 +6120,10 @@ cargo::optional Builder::create( "MDTuple", llvm::MDTuple::get(*context.llvmContext, md_arr)); } } + + // This instruction ends a block. + checkEndOpLineRange(); + return cargo::nullopt; } @@ -6045,10 +6132,8 @@ cargo::optional Builder::create(const OpSwitch *op) { llvm::Value *selector = module.getValue(op->Selector()); SPIRV_LL_ASSERT_PTR(selector); - llvm::Value *defaultDest = module.getValue(op->Default()); - SPIRV_LL_ASSERT_PTR(defaultDest); - - llvm::BasicBlock *destBB = llvm::cast(defaultDest); + llvm::BasicBlock *destBB = getOrCreateBasicBlock(op->Default()); + SPIRV_LL_ASSERT_PTR(destBB); llvm::SwitchInst *switchInst = IRBuilder.CreateSwitch(selector, destBB); // Check how many words long our literals are. They are the same width as @@ -6057,15 +6142,17 @@ cargo::optional Builder::create(const OpSwitch *op) { std::max(1u, selector->getType()->getScalarSizeInBits() / 32); for (auto target : op->Target(literalWords)) { - llvm::Value *caseDest = module.getValue(target.Label); - SPIRV_LL_ASSERT_PTR(caseDest); - - llvm::BasicBlock *caseBB = llvm::cast(caseDest); + llvm::BasicBlock *caseBB = getOrCreateBasicBlock(target.Label); + SPIRV_LL_ASSERT_PTR(caseBB); llvm::Constant *caseVal = llvm::ConstantInt::get(selector->getType(), target.Literal); switchInst->addCase(llvm::cast(caseVal), caseBB); } + + // This instruction ends a block. + checkEndOpLineRange(); + return cargo::nullopt; } @@ -6073,6 +6160,10 @@ template <> cargo::optional Builder::create(const OpKill *) { // This instruction is only valid in the Fragment execuction model, which is // not supported. + + // This instruction ends a block. + checkEndOpLineRange(); + return cargo::nullopt; } @@ -6080,6 +6171,10 @@ template <> cargo::optional Builder::create(const OpReturn *) { SPIRV_LL_ASSERT_PTR(getCurrentFunction()); IRBuilder.CreateRetVoid(); + + // This instruction ends a block. + checkEndOpLineRange(); + return cargo::nullopt; } @@ -6091,12 +6186,20 @@ cargo::optional Builder::create(const OpReturnValue *op) { SPIRV_LL_ASSERT_PTR(value); IRBuilder.CreateRet(value); + + // This instruction ends a block. + checkEndOpLineRange(); + return cargo::nullopt; } template <> cargo::optional Builder::create(const OpUnreachable *) { IRBuilder.CreateUnreachable(); + + // This instruction ends a block. + checkEndOpLineRange(); + return cargo::nullopt; } diff --git a/modules/compiler/spirv-ll/source/context.cpp b/modules/compiler/spirv-ll/source/context.cpp index 705aa86ad..af6a40986 100644 --- a/modules/compiler/spirv-ll/source/context.cpp +++ b/modules/compiler/spirv-ll/source/context.cpp @@ -142,7 +142,6 @@ cargo::expected spirv_ll::Context::translate( // Store the branch instructions found in the current function, as we need to // generate them after all the basic blocks have been generated. using OpIRLocTy = std::pair; - llvm::SmallVector Branches; // Store the Phi nodes in order to add the values after all the basic blocks // have been generated llvm::SmallVector Phis; @@ -313,29 +312,6 @@ cargo::expected spirv_ll::Context::translate( break; case spv::OpFunctionEnd: error = builder.create(op); - // Generate all the branches in the function. We are doing it here - // because - // at this point all the basic blocks will have been created - IPStack.push_back(builder.getIRBuilder().saveIP()); - for (auto &IterPos : Branches) { - builder.getIRBuilder().restoreIP(IterPos.second); - OpCode const BranchOp = IterPos.first; - switch (BranchOp.opCode()) { - default: - std::abort(); - case spv::OpBranch: - error = builder.create(*cast(&IterPos.first)); - break; - case spv::OpBranchConditional: - error = builder.create( - *cast(&IterPos.first)); - break; - case spv::OpSwitch: - error = builder.create(*cast(&IterPos.first)); - } - } - Branches.clear(); - IPStack.pop_back(); // Populate all the incoming edges for the Phi nodes we have generated IPStack.push_back(builder.getIRBuilder().saveIP()); for (auto &IterPos : Phis) { @@ -887,9 +863,13 @@ cargo::expected spirv_ll::Context::translate( error = builder.create(op); break; case spv::OpBranch: + error = builder.create(op); + break; case spv::OpBranchConditional: + error = builder.create(op); + break; case spv::OpSwitch: - Branches.push_back(std::make_pair(op, builder.getIRBuilder().saveIP())); + error = builder.create(op); break; case spv::OpKill: error = builder.create(op); diff --git a/modules/compiler/spirv-ll/source/module.cpp b/modules/compiler/spirv-ll/source/module.cpp index 4ad1290c1..87e6a473d 100644 --- a/modules/compiler/spirv-ll/source/module.cpp +++ b/modules/compiler/spirv-ll/source/module.cpp @@ -62,7 +62,7 @@ spirv_ll::Module::Module( sourceMetadataString(), CompileUnit(nullptr), File(nullptr), - CurrentOpLineRange(nullptr, llvm::BasicBlock::iterator()), + CurrentOpLineRange(std::nullopt), LoopControl(), specInfo(specInfo), PushConstantStructVariable(nullptr), @@ -87,7 +87,7 @@ spirv_ll::Module::Module(spirv_ll::Context &context, sourceMetadataString(), CompileUnit(nullptr), File(nullptr), - CurrentOpLineRange(nullptr, llvm::BasicBlock::iterator()), + CurrentOpLineRange(std::nullopt), LoopControl(), specInfo(), PushConstantStructVariable(nullptr), @@ -217,11 +217,13 @@ std::string spirv_ll::Module::getDebugString(spv::Id id) const { return DebugStrings.lookup(id); } -void spirv_ll::Module::setCurrentOpLineRange(llvm::DILocation *block, - llvm::BasicBlock::iterator pos) { - CurrentOpLineRange = std::make_pair(block, pos); +void spirv_ll::Module::setCurrentOpLineRange( + const LineRangeBeginTy &range_begin) { + CurrentOpLineRange = range_begin; } +void spirv_ll::Module::closeCurrentOpLineRange() { CurrentOpLineRange.reset(); } + void spirv_ll::Module::addCompleteOpLineRange( llvm::DILocation *location, std::pair range) { @@ -232,7 +234,7 @@ spirv_ll::Module::OpLineRangeMap &spirv_ll::Module::getOpLineRanges() { return OpLineRanges; } -std::pair +std::optional spirv_ll::Module::getCurrentOpLineRange() const { return CurrentOpLineRange; } diff --git a/modules/compiler/spirv-ll/test/spvasm/CMakeLists.txt b/modules/compiler/spirv-ll/test/spvasm/CMakeLists.txt index bd8b5362d..cb0b9e04b 100644 --- a/modules/compiler/spirv-ll/test/spvasm/CMakeLists.txt +++ b/modules/compiler/spirv-ll/test/spvasm/CMakeLists.txt @@ -22,6 +22,7 @@ endif() set(SPVASM_FILES debug_info.spvasm + debug_info_funcs_and_blocks.spvasm ext_variable_pointers_op_variable.spvasm ext_variable_pointers_function_call.spvasm op_atomic_and_function.spvasm diff --git a/modules/compiler/spirv-ll/test/spvasm/debug_info.spvasm b/modules/compiler/spirv-ll/test/spvasm/debug_info.spvasm index 471254e4b..c2e09616a 100644 --- a/modules/compiler/spirv-ll/test/spvasm/debug_info.spvasm +++ b/modules/compiler/spirv-ll/test/spvasm/debug_info.spvasm @@ -39,8 +39,10 @@ %int = OpTypeInt 32 1 %_ptr_Function_int = OpTypePointer Function %int %13 = OpConstant %int 42 + OpLine %file 4 0 %main = OpFunction %void None %3 -; CHECK: define spir_kernel void @main() +; CHECK: define spir_kernel void @main(){{.*}} !dbg [[mainSubprogram:![0-9]+]] + OpLine %file 5 2 %5 = OpLabel OpLine %file 6 3 %a = OpVariable %_ptr_Function_bool Function @@ -85,13 +87,8 @@ ; ; CHECK: [[CompileUnit]] = distinct !DICompileUnit(language: DW_LANG_OpenCL, file: [[File:![0-9]+]],{{( producer: "Codeplay SPIR-V translator",)?}} isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug{{(, enums: ![0-9]+)?}}{{(, subprograms: ![0-9]+)?}}) ; CHECK: [[File]] = !DIFile(filename: "debug_info.comp", directory: "modules/spirv-ll-tool/test/spvasm") -; CHECK: [[aLocation]] = !DILocation(line: 6, column: 3, scope: [[mainLexicalBlock:![0-9]+]]) -; TODO The line and column for `mainLexicalBlock` are incorrect and instead -; point to those for the variable `a`, adding an OpLine before `main`'s -; OpFunction has no effect. -; CHECK: [[mainLexicalBlock]] = distinct !DILexicalBlock(scope: [[mainSubprogram:![0-9]+]], file: [[File]], line: 6, column: 3) ; CHECK: [[mainSubprogram]] = distinct !DISubprogram(name: "main", linkageName: "main", scope: null, file: [[File]], -; CHECK-SAME: line: 6, type: [[mainSubroutineType:![0-9]+]] +; CHECK-SAME: line: 4, type: [[mainSubroutineType:![0-9]+]] ; CHECK-SAME: {{(, isLocal: true, isDefinition: true)?}}, scopeLine: 1 ; CHECK-SAME: {{(, isOptimized: false)?}} ; CHECK-SAME: {{(, spFlags: DISPFlagDefinition)?}} @@ -99,6 +96,8 @@ ; CHECK-SAME: {{(, (variables|retainedNodes): ![0-9]+)?}} ; CHECK-SAME: ) ; CHECK: [[mainSubroutineType]] = !DISubroutineType(types: !{{[0-9]+}}) +; CHECK: [[aLocation]] = !DILocation(line: 6, column: 3, scope: [[mainLexicalBlock:![0-9]+]]) +; CHECK: [[mainLexicalBlock]] = distinct !DILexicalBlock(scope: [[mainSubprogram]], file: [[File]], line: 5, column: 2) ; CHECK: [[bLocation]] = !DILocation(line: 7, column: 3, scope: [[mainLexicalBlock]]) ; CHECK: [[ifConditionLocation]] = !DILocation(line: 8, column: 3, scope: [[mainLexicalBlock]]) ; CHECK: [[ifTrueBlockLocation]] = !DILocation(line: 8, column: 3, scope: [[ifLexicalBlock:![0-9]+]]) diff --git a/modules/compiler/spirv-ll/test/spvasm/debug_info_funcs_and_blocks.spvasm b/modules/compiler/spirv-ll/test/spvasm/debug_info_funcs_and_blocks.spvasm new file mode 100644 index 000000000..c08002c53 --- /dev/null +++ b/modules/compiler/spirv-ll/test/spvasm/debug_info_funcs_and_blocks.spvasm @@ -0,0 +1,76 @@ +; 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 + +; RUN: %if online-spirv-as %{ spirv-as --target-env %spv_tgt_env -o %spv_file_s %s %} +; RUN: %if online-spirv-as %{ spirv-val %spv_file_s %} +; RUN: spirv-ll-tool -a OpenCL -b 64 %spv_file_s | FileCheck %s + + OpCapability Kernel + OpCapability Addresses + OpMemoryModel Physical64 OpenCL + OpEntryPoint Kernel %testfn "testfn" + %file = OpString "spvasm/debug_info_funcs_and_blocks.spvasm" + OpName %testfn "testfn" + OpName %file "file" + OpName %entry "entry" + %void = OpTypeVoid + %3 = OpTypeFunction %void + +; Test an empty line range outside a function - this shouldn't do anything to +; the following function, blocks, or instructions. + OpLine %file 0 0 + OpNoLine + +; CHECK: define spir_kernel void @testfn(){{.*}}!dbg [[mainSubprogram:![0-9]+]] + %testfn = OpFunction %void None %3 + +; Test an empty line range outside a block - this shouldn't do anything to the +; following blocks or instructions + OpLine %file 2 1 + OpNoLine + +; OpLine %file 3 2 + %entry = OpLabel + +; Test an empty range inside a block - this shouldn't do anything to the +; following instructions + OpLine %file 4 4 + OpNoLine + +; CHECK: ret void +; CHECK: } + OpReturn + OpFunctionEnd + +; CHECK: !llvm.dbg.cu = !{[[CompileUnit:![0-9]+]]} +; +; CHECK: [[CompileUnit]] = distinct !DICompileUnit(language: DW_LANG_OpenCL, file: [[File:![0-9]+]],{{( producer: "Codeplay SPIR-V translator",)?}} isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug{{(, enums: ![0-9]+)?}}{{(, subprograms: ![0-9]+)?}}) +; CHECK: [[File]] = !DIFile(filename: "debug_info_funcs_and_blocks.spvasm", directory: "spvasm") + +; Note we need a line to create the subprogram. It isn't always obvious which +; line to take - as in this test - but we take the line of the first open OpLine +; at the time the function is declared, or if there is none then we take the +; first OpLine inside the function. Hence line 2. +; CHECK: [[mainSubprogram]] = distinct !DISubprogram(name: "testfn", linkageName: "testfn", scope: null, file: [[File]], +; CHECK-SAME: line: 2, type: [[mainSubroutineType:![0-9]+]] +; CHECK-SAME: {{(, isLocal: true, isDefinition: true)?}}, scopeLine: 1 +; CHECK-SAME: {{(, isOptimized: false)?}} +; CHECK-SAME: {{(, spFlags: DISPFlagDefinition)?}} +; CHECK-SAME: {{(, unit: ![0-9]+)?}} +; CHECK-SAME: {{(, (variables|retainedNodes): ![0-9]+)?}} +; CHECK-SAME: ) + +; CHECK: [[mainSubroutineType]] = !DISubroutineType(types: !{{[0-9]+}})