[PATCH] D75649: [mlir] Introduce OwningFuncRef following the same pattern as OwningModuleRef
Tamas Berghammer via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
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
More information about the llvm-commits
mailing list