[PATCH] D79091: [mlir][spirv] Handle debug information during (de)serialization.

[Lei Zhang via Phabricator via llvm-commits]

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