Page MenuHomePhabricator

Emit strong definition for TypeID storage in Op definition (WIP)
ClosedPublic

Authored by mehdi_amini on Jul 13 2021, 9:26 AM.

Diff Detail

Event Timeline

mehdi_amini created this revision.Jul 13 2021, 9:26 AM
mehdi_amini requested review of this revision.Jul 13 2021, 9:26 AM
lattner accepted this revision.Jul 25 2021, 3:54 PM
lattner added a subscriber: lattner.
lattner added inline comments.
mlir/include/mlir/Dialect/Affine/IR/AffineOps.td
24 ↗(On Diff #358301)

FWIW, these namespace cleanups to Affine dialect are "obvious" and can be landed separately

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

I'm thrilled to see this.

Given this is emitted into a C++ file, you could emit these as true global variables, instead of as lazily initialized things within the function. Does clang optimize the lazily initialized one away when it has no initializer? If clang handles this correctly then your approach is better, but if not, then hand holding it seems best given this is autogenerated code.

This would look something like this:

os << "    static TypeID::Storage <name_mangled>_typeid_instance;\n";
os << "template<>\n";
os << "mlir::TypeID mlir::detail::TypeIDExported::get<"
   << op.getCppNamespace() << "::" << op.getCppClassName() << ">() {\n";
os << "    return TypeID(&<name_mangled>_typeid_instance);\n";
os << "}\n";
This revision is now accepted and ready to land.Jul 25 2021, 3:54 PM
mehdi_amini added inline comments.Jul 25 2021, 5:32 PM
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
2602

Does it make a difference if the static is "file scoped" or "function scoped" when the type is trivial? (we're only interested in the address, not the value).

I was expecting that this only matters if a constructor has to be invoked.

Here is the diff between the two cases as I understand it: https://godbolt.org/z/Msf1PT43j

stellaraccident added inline comments.
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
2602

I don't think it makes a big difference. File-level static is marginally nicer should some poor downstream find themselves in the position of needing to filter symbol lists and such.

It should probably also be emitted as visibility default.

lattner added inline comments.Jul 26 2021, 9:58 AM
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
2602

Looks identical to me! Please keep it as function scoped then. Thanks again!

rriddle accepted this revision.Jul 26 2021, 3:26 PM
rriddle added inline comments.
mlir/include/mlir/Dialect/Affine/IR/AffineOps.h
375 ↗(On Diff #358301)

Unrelated change?

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

Can we have a macro or something in TypeID.h that does this? That would make it easier to generate, and would also make it easier if we transition to making this mandatory.

mehdi_amini added inline comments.Jul 26 2021, 3:51 PM
mlir/include/mlir/Dialect/Affine/IR/AffineOps.h
375 ↗(On Diff #358301)

Not really: we can't do the explicit specialization emitted by TableGen otherwise: we have to rely on the "generated files are included at the top level namespace" I think.

That said, yes I'll land the affine dialect change in a separate commit, I left them here to show that this change in ODS generation can break client who haven't switched to the new include model (i.e. not fully specified the namespace for their dialect).

Address comments and add support for Types and Attributes

mehdi_amini marked 4 inline comments as done.Jul 26 2021, 8:02 PM

Add DialectGen support as well

stellaraccident accepted this revision.Jul 27 2021, 10:34 PM
This revision was landed with ongoing or failed builds.Jul 27 2021, 10:38 PM
This revision was automatically updated to reflect the committed changes.