[PATCH] D125276: [TableGen] Remove the use of global Record state

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: llvm/include/llvm/TableGen/Record.h:328
 protected:
+  /// The record keeper that initialized this Init.
+  RecordKeeper &RK;
----------------
lattner wrote:
> craig.topper wrote:
> > 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.
> 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. :(
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.


================
Comment at: llvm/include/llvm/TableGen/Record.h:1816
+  /// The internal record context used during uniquing.
+  std::unique_ptr<detail::RecordContext> Context;
+
----------------
craig.topper wrote:
> Why not make the whole Context a member of RecordKeeper? Why go through a pointer?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125276/new/

https://reviews.llvm.org/D125276