[PATCH] D74226: [mlir][spirv] Introduce spv.function

[Mahesh Ravishankar via Phabricator via llvm-commits]

mravishankar accepted this revision.
mravishankar added a comment.

Thanks Lei! Accepting conditioned on addressing other comments.



================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVStructureOps.td:251
+
+  let hasOpcode = 0;
+
----------------
antiagainst wrote:
> mravishankar wrote:
> > I think we can leave hasOpcode = 1. The autogenSerialization = 0 should avoid the serialization method to from being autogenerated.
> We cannot here because this is not a direct mapping.
Does it not map directly to OpFunction . If you name the op spv.Function , then the ODS will generate the dispatch for the serialization and deserialization. You just need to specialize the implementation of processOp in the (de)serializer for this to work. (See comment here : https://github.com/llvm/llvm-project/blob/master/mlir/lib/Dialect/SPIRV/Serialization/Serializer.cpp#L1664)
Anyway, this would be nice to have, is not necessary


================
Comment at: mlir/lib/Dialect/SPIRV/Serialization/Serializer.cpp:1645
       .Case([&](spirv::ConstantOp op) { return processConstantOp(op); })
-      .Case([&](FuncOp op) { return processFuncOp(op); })
+      .Case([&](spirv::FunctionOp op) { return processFuncOp(op); })
       .Case([&](spirv::GlobalVariableOp op) {
----------------
antiagainst wrote:
> mravishankar wrote:
> > If you set hasOpCode = 1 in the SPIRVBase.td this should be generated by ODS automatically.
> We cannot here because this is not a direct mapping: the op is not named as spv.Function; it was named spv.function and now is spv.func.
Why not name it spv.Function ? Curious. Wont make it a requirement. THis is fine too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74226