This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Handle debug information during (de)serialization.
ClosedPublic

Authored by denis13 on Apr 29 2020, 7:29 AM.

Details

Summary

This is an initial version, currently supports OpString and OpLine
for autogenerated operations during (de)serialization.

Diff Detail

Event Timeline

denis13 created this revision.Apr 29 2020, 7:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2020, 7:30 AM

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

mlir/lib/Dialect/SPIRV/Serialization/Deserializer.cpp
408

Add comment to explain what these three elements mean or maybe create a struct for it?

2040

OpNoLine should kill this definition. Could you add a process* function for it? It should be trivial. :)

mlir/lib/Dialect/SPIRV/Serialization/Serializer.cpp
589

if (!emitDebugInfo) return;

mlir/tools/mlir-tblgen/SPIRVUtilsGen.cpp
496

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.)

antiagainst requested changes to this revision.Apr 29 2020, 6:07 PM
This revision now requires changes to proceed.Apr 29 2020, 6:07 PM
denis13 marked an inline comment as done.Apr 30 2020, 7:41 AM
denis13 added inline comments.
mlir/lib/Dialect/SPIRV/Serialization/Deserializer.cpp
2040

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.
denis13 updated this revision to Diff 261228.Apr 30 2020, 7:42 AM

Addresses comments.

denis13 updated this revision to Diff 261231.Apr 30 2020, 7:48 AM

Remove new line.

denis13 updated this revision to Diff 261253.Apr 30 2020, 9:10 AM

Rebase on master.

antiagainst accepted this revision.Apr 30 2020, 12:53 PM
antiagainst marked an inline comment as done.

Nice! Thanks Denis! I just have a few more nits. Feel free to land directly after addressing them!

mlir/lib/Dialect/SPIRV/Serialization/Deserializer.cpp
81

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.

249

Discontinues

2040

Yup, correct. Thanks for handling the block end too!

2438

Can this function be replaced by op->hasTrait<IsTerminator>()?

This revision is now accepted and ready to land.Apr 30 2020, 12:53 PM
denis13 updated this revision to Diff 261343.Apr 30 2020, 1:46 PM

Addresses comments.

@antiagainst thanks for review, can you please take a look, to make sure I've updated the patch in right way. Thanks.

antiagainst accepted this revision.Apr 30 2020, 3:27 PM
antiagainst added inline comments.
mlir/lib/Dialect/SPIRV/Serialization/Deserializer.cpp
77

These constructors can be deleted too?

aprantl added inline comments.Apr 30 2020, 8:15 PM
mlir/lib/Dialect/SPIRV/Serialization/Deserializer.cpp
71

struct

denis13 marked 2 inline comments as done.May 1 2020, 4:06 AM
denis13 added inline comments.
mlir/lib/Dialect/SPIRV/Serialization/Deserializer.cpp
71

Thanks, fixed!

77

Thanks, fixed!

This revision was automatically updated to reflect the committed changes.