[PATCH] D138323: [TableGen] RegisterInfo backend - Add abstraction layer between code generation logic and syntax output

[Diana Picus via Phabricator via llvm-commits]

rovka added a comment.

In D138323#3949485 <https://reviews.llvm.org/D138323#3949485>, @Rot127 wrote:

> In D138323#3948239 <https://reviews.llvm.org/D138323#3948239>, @0x59616e wrote:
>
>> Wouldn't this break the 'open-close principle' and 'Interface segregation principle' ?
>
> Indeed it does. How about removing the `PrinterInterface` and make the `PrinterLLVM` the base class for all extensions (and mark its methods as virtual). Other `Printers` inherit from `PrinterLLVM` and override the methods needed?
> This wouldn't be a big change, but also keep the printing logic in one place and not spread around the backends.

I too have concerns about the design here. I'd rather see smaller, more specific classes, e.g. a `RegEmitterPrinter` abstract base class which can then be inherited by a `RegEmitterCPPPrinter`(*) and other similar `RegEmitter<Lang>Printer`. Having the printer for a non-C++ language inherit from the C++ printer seems suspicious - what happens if you only override a subset of the virtuals? Do you get random bursts of C++ code in your output? Using an abstract base class seems like a better choice since then if you forget to override a method (of if one is added to the base class and the subclasses aren't updated) then things should explode in an easy-to-understand/fix way.

Speaking of random bursts of C++ code, how do you handle TableGen backends that allow C++ snippets to be embedded (e.g. for the instruction selection, assuming the docs <https://llvm.org/docs/CodeGenerator.html#x86-addressing-mode:~:text=While%20the%20system%20does%20automate%20a%20lot%2C%20it%20still%20allows%20you%20to%20write%20custom%20C%2B%2B%20code%20to%20match%20special%20cases%20if%20there%20is%20something%20that%20is%20hard%20to%20express.> are up to date)?

Would be nice to get a few more opinions about this before reworking the whole patch :)

(*) We could also have a `CommonCPPPrinter` (maybe with a better name) that contains things that are the same for all the backends, e.g. `emitNamespace` and other things like that (although I guess that can be in a later patch).



================
Comment at: llvm/utils/TableGen/Printer.h:82
+
+  virtual void emitNamespace(std::string const &Name, bool Begin,
+                             std::string const &Comment = "") const;
----------------
Nit: Would be clearer to just have `emitBeginNamespace` and `emitEndNamespace`. Ditto below for `emitIncludeToggle`.


================
Comment at: llvm/utils/TableGen/RegisterInfoEmitter.cpp:787
 
-void RegisterInfoEmitter::debugDump(raw_ostream &OS) {
+void RegisterInfoEmitter::debugDump(raw_ostream &ErrOS) {
   CodeGenRegBank &RegBank = Target.getRegBank();
----------------
Nit: This looks unrelated, you should commit it separately.


================
Comment at: llvm/utils/TableGen/SequenceToOffsetTable.h:115
+  // The output language of the table.
+  PrinterLanguage PL;
+
----------------
I'd rather extract the printing code for this into a printer too.


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

https://reviews.llvm.org/D138323