This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Rework the implementation of TypeID
ClosedPublic

Authored by rriddle on Mar 30 2022, 5:50 PM.

Details

Summary

This commit restructures how TypeID is implemented to ideally avoid
the current problems related to shared libraries. This is done by changing
the "implicit" fallback path to use the name of the type, instead of using
a static template variable (which breaks shared libraries). The major downside to this
is that it adds some additional initialization costs for the implicit path. Given the
use of type names for uniqueness in the fallback, we also no longer allow types
defined in anonymous namespaces to have an implicit TypeID. To simplify defining
an ID for these classes, a new MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID macro
was added to allow for explicitly defining a TypeID directly on an internal class.

To help identify when types are using the fallback, -debug-only=typeid can be
used to log which types are using implicit ids.

This change generally only requires changes to the test passes, which are all defined
in anonymous namespaces, and thus can't use the fallback any longer.

Diff Detail

Event Timeline

rriddle created this revision.Mar 30 2022, 5:50 PM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a reviewer: sjarus. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
rriddle requested review of this revision.Mar 30 2022, 5:50 PM
jpienaar added inline comments.
mlir/include/mlir/Support/TypeID.h
249

Could we prefix these by MLIR_ ? Given these are macros exported from a header file that aren't undef'd post use, it would be nicer to "namespace" it in the rework.

mehdi_amini accepted this revision.Mar 30 2022, 6:15 PM

Nice! :)

This revision is now accepted and ready to land.Mar 30 2022, 6:15 PM
rriddle updated this revision to Diff 419556.Mar 31 2022, 1:18 PM
rriddle edited the summary of this revision. (Show Details)
rriddle marked an inline comment as done.Mar 31 2022, 1:19 PM
rriddle added inline comments.
mlir/include/mlir/Support/TypeID.h
249

Yep, makes perfect sense.

stellaraccident accepted this revision.Apr 1 2022, 2:14 PM
stellaraccident added a subscriber: stellaraccident.

Thank you so much for this. I'm glad to see the best of both worlds implemented (explicit/homed and name based fallback). I had tried to sketch something not unlike this up in the past but failed to get it into a working form.

I believe this approach will be compatible with the vagaries of windows DLLs too (or can be made compatible where the previous approach could not be).

mlir/include/mlir/Support/TypeID.h
181

Would you mind extending this comment with why this specialization is only valid for fully resolved types?

310–312

A note on why the alignment is important (and why "8")? (Presumably so it can operate as part of a pointer union on all platforms).

bondhugula accepted this revision.Apr 1 2022, 6:26 PM

Nice!

mlir/include/mlir/Support/TypeID.h
168

Nit: it's -> its

bondhugula added inline comments.Apr 1 2022, 6:45 PM
mlir/include/mlir/Support/TypeID.h
195

It'll be good to have a doc comment here as well. Appreciate the detailed comments everywhere else.

rriddle updated this revision to Diff 419953.Apr 2 2022, 12:41 AM
rriddle marked 5 inline comments as done.
This revision was automatically updated to reflect the committed changes.