This is an archive of the discontinued LLVM Phabricator instance.

[TableGen] Remove the use of global Record state
ClosedPublic

Authored by rriddle on May 9 2022, 3:32 PM.

Details

Summary

This commits removes TableGens reliance on managed static global record state
by moving the RecordContext into the RecordKeeper. The RecordKeeper is now
treated similarly to a (LLVM|MLIR|etc)Context object and is passed to static
construction functions. This is an important step forward in removing TableGens
reliance on global state, and in a followup will allow for users that parse tablegen
to parse multiple tablegen files without worrying about Record lifetime.

Diff Detail

Event Timeline

rriddle created this revision.May 9 2022, 3:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2022, 3:32 PM
rriddle requested review of this revision.May 9 2022, 3:32 PM

Some context here is that I'm building language tooling for TableGen (diagnostics, go-to definition, find references, etc.) and I can't really do that effectively if TableGen relies on global record state.

craig.topper added inline comments.
llvm/include/llvm/TableGen/Record.h
328

Most Inits inherit from TypedInit which has a RecTy. Is possible for the TypedInits to use the RecordKeeper from the the Type? Then only UnsetInit needs an explicit RecordKeeper?

But I guess that means Init::getRecordKeeper would need to examine the Opc to determine where to find the RecordKeeper.

1816

Why not make the whole Context a member of RecordKeeper? Why go through a pointer?

lattner accepted this revision.May 9 2022, 11:32 PM
lattner added inline comments.
llvm/include/llvm/TableGen/Record.h
328

Agreed, it would be nice to use an approach like MLIR does with MLIRContext: since types have access to a MLIRContext, other things that are guaranteed to have types don't need to carry it around. Is this possible in tblgen?

Ah it seems that init doesn't carry a type. :(

This revision is now accepted and ready to land.May 9 2022, 11:32 PM
rriddle updated this revision to Diff 428491.May 10 2022, 1:49 PM
rriddle marked 3 inline comments as done.
rriddle added inline comments.May 10 2022, 1:50 PM
llvm/include/llvm/TableGen/Record.h
328

Given that all but one of the Inits are Typed, used the suggestion. I don't think it will be too bad in practice, and we can update if we ever add more inits.

1816

Using pImpl helps to keep the uniquers implementation details out of the public header, and also helps to reduce the number of friends of RecordKeeper. If we move the uniquer data directly into RecordKeeper, as opposed to an external struct, we would need to add ~20 friends. Given all of that, feels much cleaner to just use pImpl here.

craig.topper added inline comments.May 10 2022, 2:55 PM
llvm/utils/TableGen/WebAssemblyDisassemblerEmitter.cpp
45

I know you didn't write, this but can the reinterpret_cast be a cast<>. reinterpret_cast looks suspicious.

rriddle updated this revision to Diff 428560.May 10 2022, 9:27 PM
rriddle marked an inline comment as done.
rriddle added inline comments.May 10 2022, 9:28 PM
llvm/utils/TableGen/WebAssemblyDisassemblerEmitter.cpp
45

Thanks for pointing this out! Fixed.

+1 for doing this, thank you!

This revision was landed with ongoing or failed builds.May 11 2022, 11:55 AM
This revision was automatically updated to reflect the committed changes.
llvm/lib/TableGen/Record.cpp