[PATCH] D75649: [mlir] Introduce OwningFuncRef following the same pattern as OwningModuleRef

Wed Mar 11 03:47:24 PDT 2020

tberghammer added a comment.

In D75649#1916265 <https://reviews.llvm.org/D75649#1916265>, @rriddle wrote:

> In D75649#1916222 <https://reviews.llvm.org/D75649#1916222>, @mehdi_amini wrote:
>
> > > using `OpBuilder::create` is much more idiomatic with the rest of the infra.
> >
> > The builder APIs don't express clear ownership though, having a RAII type would be nice!
> >  Could the OpBuilder::create always return an OwningOpRef<OpTy> ?
>
>
> That doesn't really make sense to me, OpBuilder::create generally inserts the operation into a block. I'm not sure what ownership semantics you are assigning to it here. Am I missing something? OpBuilder::create isn't a static method.

I think there is still value in being able to create a FuncOp and any other "block type" operation without adding it to the parent block for error reporting and error recovery. Also I can think of cases where you don't know the parent block at the time when we started to create a block (e.g. we add the FuncOp to a different module based on some property we derive later).

A valid point from Mehdi is that we should change FuncOp::create to return OwningFuncRef so it makes the semantics clear but I would prefer to do that separately as it touches quite a few files.

P.S.: Looking at mlir/examples/toy/Ch3/mlir/MLIRGen.cpp it uses the exact pattern I want to avoid where in case of a failure we have to manually erase the FuncOp to avoid a memory leak.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75649