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

[Lei Zhang via Phabricator via llvm-commits]

antiagainst added a comment.

Awesome! Thanks a lot for adding this, Denis! Overall LGTM; just a few nits.



================
Comment at: mlir/lib/Dialect/SPIRV/Serialization/Deserializer.cpp:391
+  /// processing instruction.
+  SmallVector<uint32_t, 3> debugLine;
+
----------------
Add comment to explain what these three elements mean or maybe create a struct for it?


================
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();
----------------
OpNoLine should kill this definition. Could you add a `process*` function for it? It should be trivial. :)


================
Comment at: mlir/lib/Dialect/SPIRV/Serialization/Serializer.cpp:585
+void Serializer::processDebugInfo() {
+  if (emitDebugInfo) {
+    auto fileLoc = module.getLoc().dyn_cast<FileLineColLoc>();
----------------
if (!emitDebugInfo) return;


================
Comment at: mlir/tools/mlir-tblgen/SPIRVUtilsGen.cpp:496
+/// Emits OpLine instruction for the given `opVar` and `binary`.
+static void emitDebugLineOp(StringRef opVar, StringRef binary,
+                            raw_ostream &os) {
----------------
We have a problem with the generated code size at the moment. Can we turn this into a utilty function in Serializer.cpp and call it here instead of embedding the logc for each op? (I'm also gonna clean up existing duplicated logic in the autogen'ed code soon.) 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79091/new/

https://reviews.llvm.org/D79091