This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Introduce OwningSPIRVModuleRef for ownership
ClosedPublic

Authored by antiagainst on Jun 11 2020, 6:55 AM.

Details

Summary

Similar to OwningModuleRef, OwningSPIRVModuleRef signals ownership
transfer clearly. This is useful for APIs like spirv::deserialize,
where a spirv::ModuleOp is returned by deserializing SPIR-V binary
module.

This addresses the ASAN error as reported in
https://bugs.llvm.org/show_bug.cgi?id=46272

Diff Detail

Event Timeline

antiagainst created this revision.Jun 11 2020, 6:55 AM
Herald added a project: Restricted Project. · View Herald Transcript

I also tried to turn the existing OwningModuleRef into a template class OwningModuleRefBase and using OwningModuleRef = OwningModuleRefBase<mlir::ModuleOp>. But it means we need now to include the Module.h header where we forward declare class OwningModuleRef. That seems more intrusive than the current approach, which means a bit code duplication. But I'm happy to go back to the template approach if you think it's better. Just let me know. :)

I was somewhat strongly against this in the past because it sets the wrong usage pattern expectations. Though the example you have here makes more sense to me than say FuncOp. That being said, I would prefer we don't duplicate this code and instead use a templated version.

\But it means we need now to include the Module.h header where we forward declare class OwningModuleRef.

That's only an issue if you have a 'using' statement. I would expect that you could use inheritance and achieve the same result. That would only break down in a situation where you wanted to templatize based on the BaseRef class, but I don't foresee such a usecase right now.

I was somewhat strongly against this in the past because it sets the wrong usage pattern expectations.

Can you clarify what you mean with "pattern expectations" here?

I was somewhat strongly against this in the past because it sets the wrong usage pattern expectations.

Can you clarify what you mean with "pattern expectations" here?

The "usage pattern expectations" I mean are that we shouldn't be using this type of API within passes, patterns, or a majority of the infra that people usually work in. IMO this is more for the top-level layer, i.e., things like parsing an IR blob. In most normal use cases, passing a builder should be preferred over manually constructing and managing the lifetime of the IR.

I was somewhat strongly against this in the past because it sets the wrong usage pattern expectations.

Can you clarify what you mean with "pattern expectations" here?

The "usage pattern expectations" I mean are that we shouldn't be using this type of API within passes, patterns, or a majority of the infra that people usually work in. IMO this is more for the top-level layer, i.e., things like parsing an IR blob. In most normal use cases, passing a builder should be preferred over manually constructing and managing the lifetime of the IR.

Oh right: builders are much better. We still have a bunch of "unsafe" APIs though, and even though aren't the most common to use I am not sure why they couldn't get more type safety?

I was somewhat strongly against this in the past because it sets the wrong usage pattern expectations. Though the example you have here makes more sense to me than say FuncOp. That being said, I would prefer we don't duplicate this code and instead use a templated version.

SG. I'll change later then.

Address comments

This is now changed to use inheritance as suggested by River. Let me know if this is okay now.

rriddle accepted this revision.Jul 6 2020, 7:23 PM
rriddle added inline comments.
mlir/include/mlir/IR/ModuleRefBase.h
21 ↗(On Diff #275884)

Can you add that OpBuilder and related functionality should be highly preferred instead, and this should only be used in situations where existing solutions are not viable.

24 ↗(On Diff #275884)

nit: I would likely just rename to OwningOpRefBase instead.

mlir/lib/Dialect/SPIRV/Serialization/Deserializer.cpp
1750

nit: return deserializer.collect.getValueOr(nullptr);

This revision is now accepted and ready to land.Jul 6 2020, 7:23 PM
antiagainst marked 3 inline comments as done.

Address comments: renamed class and file and add comments

Fix accidentially removed code

Rename variables too

This revision was automatically updated to reflect the committed changes.