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

[River Riddle via Phabricator via llvm-commits]

rriddle 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.



================
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() {
----------------
Could we auto-generate these in OpDefGen?


================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVOps.cpp:79
+static FunctionType getEnclosingFuncOpType(Operation *op) {
+  auto funcOp = op->getParentOfType<FuncOpTy>();
+  if (!funcOp)
----------------
if (auto funcOp = ...)
  return funcOp.getType();
return FunctionType();

?


================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVOps.cpp:1782
+  auto oldType = getType();
+  for (int i = newType.getNumInputs(), e = oldType.getNumInputs(); i < e; i++) {
+    removeAttr(getArgAttrName(i, nameBuf));
----------------
Drop trivial brace.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74226