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

[Mahesh Ravishankar via Phabricator via llvm-commits]

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

THanks for taking this one now. Its already touching so much code, if we left it for longer it would be more effort to use spv.function



================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVStructureOps.td:251
+
+  let hasOpcode = 0;
+
----------------
I think we can leave hasOpcode = 1. The autogenSerialization = 0 should avoid the serialization method to from being autogenerated.


================
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.
----------------
I dont think we need this. This is only for testing. So not adding this, but leaving the conversion as `applyPartialConversion` is fine. 


================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVOps.cpp:2604
+  // for writing tests.
+  auto fnType = getEnclosingFuncOpType<spirv::FunctionOp>(retValOp);
+  if (!fnType)
----------------
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.


================
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) {
----------------
If you set hasOpCode = 1 in the SPIRVBase.td this should be generated by ODS automatically.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74226