Skip to content

Commit

Permalink
[spirv-ll] Fix various debug info generation issues
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
frasercrmck committed Nov 15, 2023
1 parent bbab273 commit 1a6d31e
Show file tree
Hide file tree
Showing 8 changed files with 324 additions and 133 deletions.
21 changes: 20 additions & 1 deletion modules/compiler/spirv-ll/include/spirv-ll/builder.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
///
Expand All @@ -335,13 +335,32 @@ 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();

/// @brief Finalizes and adds any metadata to LLVM that was generated by
/// 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.
Expand Down
29 changes: 20 additions & 9 deletions modules/compiler/spirv-ll/include/spirv-ll/module.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand All @@ -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<llvm::DILocation *, llvm::BasicBlock::iterator>
getCurrentOpLineRange() const;
std::optional<LineRangeBeginTy> getCurrentOpLineRange() const;

/// @brief Add a basic block and associated lexical block to the module.
///
Expand Down Expand Up @@ -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<llvm::DILocation *, llvm::BasicBlock::iterator> CurrentOpLineRange;
/// @brief DILocation/basic block iterator pair to store the beginning of the
/// current OpLine range.
std::optional<LineRangeBeginTy> CurrentOpLineRange;
/// @brief Map of BasicBlock to associated `DILexicalBlock`.
llvm::DenseMap<llvm::BasicBlock *, llvm::DILexicalBlock *> LexicalBlocks;
/// @brief Map of function to associated `DISubprogram`.
Expand Down
Loading

0 comments on commit 1a6d31e

Please sign in to comment.