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

Mon Feb 10 07:48:27 PST 2020

antiagainst added a comment.

> I would actually prefer that we call this FuncOp('func'). It is consistent with all of the other functions that have been defined, and seems a little weird to deviate. For example, spirv::ModuleOp/ConstantOp are also named the same as other common ops.

The purpose was trying to avoid potential confusions between std ones but you are right this is already happening for other SPIR-V ops. So consistency wins. Done.

================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVOps.h:49
+// spirv::Function ops hash just like pointers.
+template <> struct DenseMapInfo<mlir::spirv::FunctionOp> {
+  static mlir::spirv::FunctionOp getEmptyKey() {
----------------
rriddle wrote:
> Could we auto-generate these in OpDefGen?
Yup we can. But do we want to do that for each op? With so many ops having this specialization, it can increase C++ compilers' burden quite a bit. I'd defer that to when there are more ops wanting this.

================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVStructureOps.td:251
+
+  let hasOpcode = 0;
+
----------------
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.

================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVStructureOps.td:277
+    ///    parameters they won't have any attributes.
+    void setType(FunctionType newType);
+
----------------
mehdi_amini wrote:
> The 4 methods above all seem like they could be added to FunctionLike trait?
Other function-like ops (like gpu.func and llvm.func) might want slightly different behavior for some of the ops but yes this can be moved to `FunctionLike` to be a default impl. Done.

================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRVPass.cpp:41
       spirv::lookupTargetEnvOrDefault(module), context);
-  target->addDynamicallyLegalOp<FuncOp>(
-      [&](FuncOp op) { return typeConverter.isSignatureLegal(op.getType()); });
+  // Additionally allow builtin module and module terminator ops because they
+  // wrap tests.
----------------
mravishankar wrote:
> I dont think we need this. This is only for testing. So not adding this, but leaving the conversion as `applyPartialConversion` is fine. 
It's tricky here. We need to pull in `func` to `spv.func` conversion so that we can materialize function signature conversion, which is needed for load/store op conversion in subview tests. Once that pattern is pulled in, it might cause that some test IR is in an invalid state. For example, `mulf %arg, %arg: tensor<4xf32>` won't be converted because we don't have a pattern to convert binary ops that require type conversion. But the `%arg` will be converted anyway because of `func` to `spv.func` change. So we will see a validation failure on `mulf`. I was trying to address this by turning the pass to do full conversion here given this pass is mainly for testing so we don't care that much. I think another way to address this is to disable the test for `mulf %arg, %arg: tensor<4xf32>`. It's actually just a negative test that checks we are **missing** a pattern. Done.

================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVOps.cpp:1745
+  if (getType().getNumResults() > 1)
+    return emitOpError("cannot have more than one result");
+
----------------
mehdi_amini wrote:
> Seems like this should be checked in `verifyType()`?
> 
> I'd expect verifyBody to actually find the return ops and check for the matching, as well as other possible SPIRV specific properties.
Good points. Moved this check to `verifyBody` and bumped checks for return ops here.

================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVOps.cpp:2604
+  // for writing tests.
+  auto fnType = getEnclosingFuncOpType<spirv::FunctionOp>(retValOp);
+  if (!fnType)
----------------
mravishankar wrote:
> I dont think we should do this. spv.return must stay in spv.func. If for testing we can use func and return and the body can have spv.* operations. We just need to not do `applyFullConversion` and it should be fine.
Bumped the check to `spv.function` so we don't need special-casing the enclosing function types here. But yes it's allowing return ops to be used at more places, which is more friendly to interop between different dialects. If ops from other dialects would like to use SPIR-V return ops then it's their responsibility to make sure the semantics match with their cases. This is consistent with how we handle ops within SPIR-V dialect: we allow non-region ops to be mixable with other dialects but we enforce checks in region ops (like `spv.module`, `spv.func`) to verify SPIR-V semantics at the scope of those ops. 

More reasons of allowing return ops to be interop with other dialect ops: ideally I'd like to enforce the `spv.func` op to verify its region to only contain SPIR-V ops, and I'd like to make the `func` to `spv.func` op conversion to only kick in when a `func` op's region already only contains SPIR-V ops. That means, we need to allow other ops that can be placed inside functions to be converted first. If instead we require SPIR-V return ops to only work with `spv.func`, that causes a circular dependency... Theoretically it can be solved by throwing all patterns in the same pass maybe, but I'm not sure we want to go that far right now. Besides, `func` and `std.return` actually are in different dialects and likely we will have pass boundaries between their patterns anyway.

================
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:
> 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74226