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

[Lei Zhang via Phabricator via llvm-commits]

antiagainst marked an inline comment as done.
antiagainst added inline comments.


================
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.
----------------
antiagainst wrote:
> 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. 
Oh, replied before seeing your new comments. Sure, I'll address these. :) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74226