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

[Rot127 via Phabricator via llvm-commits]

Rot127 added a comment.

In D138323#4050804 <https://reviews.llvm.org/D138323#4050804>, @rovka wrote:

> 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`.

The disadvantage with a `Printer` class per backend is that we have ~2+ files for each backend. So quite a lot.
Although this might not so bad if each backend is moved to its own directory. Just like `GlobalISel`.

> ... what happens if you only override a subset of the virtuals? Do you get random bursts of C++ code in your output?

Personally I don't see the danger of unexpected C++ output.
If an introduction document is added how to add a new language `Printer` (regardless of the design I will definitely add one), it should be clear that all methods for a backend must be overriden.

> 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.

The abstract class version was the first implementation I added here. If we agree on backend specific printer classes I would like to go back to this as well.
But with a single `Printer` for all backends it would force each new language `Printer` to override methods for other backends as well. Although it only needs one.

> (*) 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).

If each backend gets its own printer class they could simply inherit from such a `PrinterCommon` or `PrinterBase`.
So something like : `PrinterBase <---- PrinterBackendX <---- PrinterLangY`?

> 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)?

I haven't looked at it because for the Capstone use case we only needed the backends listed on top to be honest.
But I'll look into it and report back.

> I'd rather extract the printing code for this into a printer too. (The comment to `SequenceToOffsetTable.h`)

Of cause. But `SequenceToOffsetTable` is used in a yet not refactored backend. So I left it there for the time being. But I agree that classes like this should be integrated into the `Printer`.

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

Absolutely agree :)


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

https://reviews.llvm.org/D138323