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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
