[PATCH] D93591: [MLIR][SPIRV] Add (de-)serialization support for SpecConstantOpeation.

[Lei Zhang via Phabricator via cfe-commits]

antiagainst requested changes to this revision.
antiagainst added a comment.
This revision now requires changes to proceed.

Awesome! I just have a few nits left now. Marked as blocking because we need to avoid change Clang code.



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:623
             {"referencesProvider", true},
-            {"astProvider", true},
+            {"astProvider", true}, // clangd extension
             {"executeCommandProvider",
----------------
Accidental change or rebase leftover?

Anyway we need to revert the change here. :)


================
Comment at: mlir/lib/Target/SPIRV/Deserialization.cpp:557
+  // mapping.
+  DenseMap<uint32_t, std::tuple<spirv::Opcode, uint32_t, ArrayRef<uint32_t>>>
+      specConstOperationMap;
----------------
Just define a `struct` instead of using `std::tuple`? It will make the code more readable. Also we should use `SmallVector` instead of `ArrayRef` here. I guess ArrayRef is okay because we have access to the underlying SPIR-V blob right now; but it can be a potential hazard given ArrayRef does not owns the data.


================
Comment at: mlir/lib/Target/SPIRV/Deserialization.cpp:1811
+  // that there is no need to update this fake ID since we only need to
+  // reference the creaed Value for the enclosed op from the spv::YieldOp
+  // created later in this method (both of which are the only values in their
----------------
s/creaed/created/


================
Comment at: mlir/lib/Target/SPIRV/Deserialization.cpp:1816
+  // previous Value assigned to it isn't visible in the current scope anyway.
+  DenseMap<uint32_t, Value> newValueMap;
+  std::swap(valueMap, newValueMap);
----------------
What about using `llvm::SaveAndRestore` to avoid manually swap?


================
Comment at: mlir/lib/Target/SPIRV/Deserialization.cpp:1791
+
+  // Since the enclosed op is emitted in the current block, split it in a
+  // separate new block.
----------------
ergawy wrote:
> antiagainst wrote:
> > What about first creating the spec op, set the insertion point to its body (with proper guard), and then process the transient instruction? This can avoid us from doing the block manipulation right? Following the above comment, this is part of swapping the "context".
> If we do this, since the transient instruction has operands that are constants, global vars, or spec constants, then the instructions to materialize these references will be emitted in the spec op's region. This is the main reason I think we need such manipulation here; we first need to materialize all references, if any, in the parent region and then isolate the spec op's enclosed op.
> 
> Let me know if what you are saying is going over my head :D.
You are right. Sorry I left this comment earlier than the comment about serializing into `typesGlobalValues`. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93591