Page MenuHomePhabricator

[mlir] Introduce OwningFuncRef following the same pattern as OwningModuleRef
Needs ReviewPublic

Authored by tberghammer on Mar 4 2020, 2:52 PM.

Details

Reviewers
rriddle
Summary

Between the creating of a function op and adding it to a module it has no
explicit owner what can lead to memory leaks. The new class is intended to be
used as a convenient RAII owner for that time.

This change generalizes the code for OwningModuleRef to support arbitrary Op
types and makes OwningModuleRef and OwningFuncRef a simple shim around the
generic class to still enable forward declaring these types.

Diff Detail

Event Timeline

tberghammer created this revision.Mar 4 2020, 2:52 PM
Herald added 1 blocking reviewer(s): rriddle. · View Herald TranscriptMar 4 2020, 2:52 PM
Herald added a project: Restricted Project. · View Herald Transcript

Fix code formatting

Add missing include

Hi Tamas, thanks for the patch. ModuleOp is a bit special in that it is the top-level IR entity. What situation are you in where you can't insert the FuncOp directly on creation?

Hi River, I have two (somewhat related) usecases for having a FuncOp without adding it to a module:

  1. I would like to write an mlir::OwingFuncRef BuildFunc(...) function what takes some internal/domain specific input (e.g. IR from the front end) and returns the newly built function. I could just pass in the ModuleOp as an argument and add the FuncOp to it but returning an OwningFuncRef feels like a nicer API
  2. My code what builds a FuncOp might fail (e.g. due to invalid input) and in that case I want to report an error without modifying the existing ModuleOp as adding a broken function to it would make the module op broken as well. My approach for it is to create an OwningFuncRef, use it to own the FuncOp during the building process (so in case of a failure it will correctly destroy it) and then in case of success I would move the FuncOp from the OwningFuncRef to the ModuleOp.

Hi River, I have two (somewhat related) usecases for having a FuncOp without adding it to a module:

  1. I would like to write an mlir::OwingFuncRef BuildFunc(...) function what takes some internal/domain specific input (e.g. IR from the front end) and returns the newly built function. I could just pass in the ModuleOp as an argument and add the FuncOp to it but returning an OwningFuncRef feels like a nicer API
  2. My code what builds a FuncOp might fail (e.g. due to invalid input) and in that case I want to report an error without modifying the existing ModuleOp as adding a broken function to it would make the module op broken as well. My approach for it is to create an OwningFuncRef, use it to own the FuncOp during the building process (so in case of a failure it will correctly destroy it) and then in case of success I would move the FuncOp from the OwningFuncRef to the ModuleOp.

I would rather not encourage this type of usage. FuncOp::create is something that I've been meaning to remove for a long time, but I keep getting distracted. It is a relic of the time when FuncOp was Function, and not represented as an Operation. You also don't necessarily need to pass in a ModuleOp, using OpBuilder::create is much more idiomatic with the rest of the infra.

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> ?

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.

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.

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.

Ah of course you're right! I was thinking about Operation::create actually :)
It is less often used directly though because of OpBuilder...