[PATCH] D79091: [mlir][spirv] Handle debug information during (de)serialization.
Lei Zhang via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 30 12:53:55 PDT 2020
antiagainst accepted this revision.
antiagainst marked an inline comment as done.
antiagainst added a comment.
This revision is now accepted and ready to land.
Nice! Thanks Denis! I just have a few more nits. Feel free to land directly after addressing them!
================
Comment at: mlir/lib/Dialect/SPIRV/Serialization/Deserializer.cpp:2021
+ return emitError(unknownLoc, "OpLine must have 3 operands");
+ debugLine = {operands.begin(), operands.end()};
+ return success();
----------------
denis13 wrote:
> antiagainst wrote:
> > OpNoLine should kill this definition. Could you add a `process*` function for it? It should be trivial. :)
> Thanks for review, if I got it right, we have to clear debug line in 3 cases:
> 1) end of block.
> 2) OpNoLine.
> 3) Another OpLine.
Yup, correct. Thanks for handling the block end too!
================
Comment at: mlir/lib/Dialect/SPIRV/Serialization/Deserializer.cpp:81
+
+ bool isEmpty() { return fileID == 0; }
+ void clear() { fileID = line = col = 0; }
----------------
Actually all zeros might be a triple in real SPIR-V blobs. fileID=0 can map to a %0 = OpString and line=0 and col=0 can mean something related to the whole file.
Instead of baking the invalid/empty state in this struct, what about make the `debugLine` in `Deserializer` as `Optional<DebugLine>` so we can use `llvm::None` to mean no debug info? Then I think we can remove all the functions in this struct.
================
Comment at: mlir/lib/Dialect/SPIRV/Serialization/Deserializer.cpp:253
+
+ /// Discontinue any source-level location information that might be active
+ /// from a previous OpLine instruction.
----------------
Discontinues
================
Comment at: mlir/lib/Dialect/SPIRV/Serialization/Deserializer.cpp:2445
+bool Deserializer::isTerminationOperation(Operation *op) {
+ if (!op)
----------------
Can this function be replaced by `op->hasTrait<IsTerminator>()`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79091/new/
https://reviews.llvm.org/D79091
More information about the llvm-commits
mailing list