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

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle requested changes to this revision.
nhaehnle added a comment.
This revision now requires changes to proceed.

I appreciate your desire to extract useful information, but I think this is fundamentally the wrong approach to take.

The interface implied by PrintLLVM is at the wrong level of abstraction: will users who want to extract information really want to extract all this information, and if they do, will they want to do it in the same order and along the same hierarchy? What works for C++ might not work for another language and for another context. Another user of the information here is unlikely to need the exact set that is provided by the existing printer. And so on. In essence, the interface is exactly the wrong way around.

The code is already structured into a "database" part (`CodeGenTarget`) and a "printer" part, and the printer is quite intrinsically tied to C++ structures in many places. Ideally, you'd just use CodeGenTarget from a new printer, and potentially augment that class with additional features you may need.

The other meta problem here is that you're changing the code to be a lot less maintainable without an easily understandable in-tree motivation. For example: let's say we land this change. Soon after, anybody who looks at the code with these changes will immediately (and correctly) conclude that all those virtual methods aren't necessary and ought to be removed. Therefore, for out-of-tree purposes I would seriously urge you to just write a new printer that re-uses CodeGenTarget.

This would be different if you had an in-tree motivation for the changes.



================
Comment at: llvm/utils/TableGen/SequenceToOffsetTable.h:115
+  // The output language of the table.
+  PrinterLanguage PL;
+
----------------
rovka wrote:
> I'd rather extract the printing code for this into a printer too.
I disagree. That's additional complexity that simply isn't needed at this stage.


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

https://reviews.llvm.org/D138323