This is an archive of the discontinued LLVM Phabricator instance.

[ODS] Extra Concrete Declarations and Definitions under Traits
ClosedPublic

Authored by amandatang on Jul 7 2023, 11:08 AM.

Details

Summary

Support extra concrete class declarations and definitions under NativeTrait that get injected into the class that specifies the trait. Extra declarations and definitions can be passed in as template arguments for NativeOpTraitNativeAttrTrait and NativeTypeTrait.

Usage examples of this feature include:

  • Creating a wrapper Trait for authoring inferReturnTypes with the OpAdaptor by specifying necessary Op specific declarations and definitions directly in the trait
  • Refactoring the InferTensorType trait

Diff Detail

Event Timeline

amandatang created this revision.Jul 7 2023, 11:08 AM
Herald added a project: Restricted Project. · View Herald Transcript
amandatang requested review of this revision.Jul 7 2023, 11:08 AM
amandatang updated this revision to Diff 538263.Jul 7 2023, 2:13 PM

clang format

amandatang updated this revision to Diff 538279.EditedJul 7 2023, 3:09 PM

Quick fix

jpienaar added inline comments.Jul 7 2023, 4:19 PM
mlir/include/mlir/IR/OpBase.td
1969

Could you document these here?

mlir/include/mlir/Interfaces/InferTypeOpInterface.h
239–240

You can remove this TODO now as not relevant anymore here

241

ArrayRef?

mlir/include/mlir/Interfaces/InferTypeOpInterface.td
223

Does this end up only inserting inferReturnTypes ?

mlir/include/mlir/TableGen/Trait.h
71

s/type/instance/ ? given you can have Type's with this too

mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
1362

This cast shouldn't be needed. The adaptor returns a TypedValue<MemRefType>, so that getType() is of type MemRefType.

Now what is true, is that nothing has verified it until here along the build path (but building with invalid inputs is not expected to work) while along verify path this happens post verifying type, so should be fine in either case.

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
858

This is going to constantly resize the string below, could that be avoided? (same in other spots)

jpienaar edited reviewers, added: okwank; removed: nicolasvasilache.Jul 7 2023, 4:22 PM
amandatang marked 6 inline comments as done.

Address comments

mlir/include/mlir/Interfaces/InferTypeOpInterface.td
223

Yes

mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
1362

The getSource() method in the Op itself returns TypedValue<MemRefType>, but the equivalent method in the adaptor returns a ValueT, which requires a cast.

Update documentation

jpienaar accepted this revision.Jul 11 2023, 9:20 AM

Looks good overall, thanks

mlir/docs/Traits.md
107

s/properties/fields/ ? (at least seems from TableGen reference that's the more common name)

110

They will be formatted too with variables being replaced

mlir/include/mlir/IR/AttrTypeBase.td
45

I thought I have asked about this, but why not have the same template args on NativeTrait too?

That way you cold just do NativeTrait<name, "Type", extraTypeDeclaration, extraTypeDefinition> here.

This revision is now accepted and ready to land.Jul 11 2023, 9:20 AM
amandatang marked 3 inline comments as done.

Address final comments

This revision was automatically updated to reflect the committed changes.