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

[Lei Zhang via Phabricator via llvm-commits]

antiagainst added inline comments.


================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVOps.h:49
+// spirv::Function ops hash just like pointers.
+template <> struct DenseMapInfo<mlir::spirv::FuncOp> {
+  static mlir::spirv::FuncOp getEmptyKey() {
----------------
rriddle wrote:
> This needs to be formatted.
This is actually the result after running format fix internally. Maybe because clang-format version differences.


================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVStructureOps.td:251
+
+  let hasOpcode = 0;
+
----------------
mravishankar wrote:
> 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
OpFunction is just one instruction composing the whole function; others include OpFunctionParameter, OpFunctionEnd. spv.func here actually includes all of them. We have been following the convention to use lower case names for such cases. 


================
Comment at: mlir/include/mlir/IR/FunctionSupport.h:141
+  /// Adds an entry block to an empty function, and set up the block arguments
+  /// to match the signature of the function. The newly inserted entry block
+  /// is returned.
----------------
rriddle wrote:
> rriddle wrote:
> > rriddle wrote:
> > > None of these are valid if the type of the function isn't the builtin `FunctionType`, i.e. LLVMFuncOp doesn't work with any of these methods.
> > If you want to add these, the things that rely on FunctionType should be moved to a separate section and there should be disclaimers/comments on the top-level comment of FunctionLike denoting that these methods need to be hidden/overridden if the underlying type isn't FunctionType. You should also make sure that these are all hidden for LLVMFuncOp.
> An alternative that may work is to static assert that the type returned by `std::declval<ConcreteType>().getType()` is equal to `FunctionType` inside of each of those methods.
Yup. But won't this still be good defaults for other func ops (std, gpu, spv)? llvm side one can hide these methods. (And that's what happening per the current impl.) Let me know if this is okay, I can always put these back and copy on the SPIR-V side. 


================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVOps.cpp:1741
+      if (fnType.getNumResults() != 0) {
+        retOp.emitOpError("cannot be used in functions returning value");
+        return WalkResult::interrupt();
----------------
rriddle wrote:
> You can return the errors directly. `failure` implicitly converts to WalkResult::interrupt. You just need to set the return type on the lambda to WalkResult.
Oh, that's nice! Thanks for the suggestion! Changed.


================
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) {
----------------
mravishankar wrote:
> 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.
Explained at another place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74226