I’ve read the code of llvm/TargetParser submodule (especially Triple.cpp file) and it seems to me pretty fragmented and complex. The enormous amount of switches, string literals, too complicated dependencies.
The idea is to refactor these classes in more OOP way to give more flexibility for development. (I mean, I suppose, the development of new target should start with implementation of some base classes, and not with inserting a hundreds of lines into hundreds different places)
Triple handling is definitely one area where adding a new target is very complicated and intrusive. Broadly speaking, this seems like a worthwhile direction to pursue. Some aspects probably can’t be separated out into target-specific files (e.g., you have to add an enumerator to the declaration of the enum), but to the extent that we reasonably can, it would be good. You might want to take a look at how the Clang driver breaks things up (clang/lib/Driver/ToolChains); it used to be just a couple of massive modules, but now is much more manageable.
If you want to show (small-ish) diffs in a post here, I suggest pasting diff output in a noformat section (delimited by triple backticks). Not huge diffs, though; Differential is better for that, and all your readers are used to it.
A triple-backtick section ends up looking like this.
Yes, I saw clang’s toolchains. It also has a bit fancy design, but it’s definitely more manageable than llvm’s triple.
I would be glad if you could look at my differential post (it’s big enough not to paste it here). I think the design of Architecture part of the Triple is pretty straightforward and can be understood even without documentation (which I gonna write later). However I would like to get some opinions and discussion.
For now I have designed and implemented only the Arch part of the Triple. That’s why some of my classes are coupled with current version of Triple enums.
This will be changed as soon as I design and implement the other parts of the Triple.
In looking through the patch, I’m having a difficult time seeing how the OOP structure you are proposing is meant to simplify the existing logic. Since you provide just the machinery, and no actual implementation of the complex logic you’re trying to replace, it’s hard to compare how the replacement you provide would be improving the current situation.
Based on the structure I’ve seen so far, I wonder if OOP really is the best way to simplify this logic. Several virtual functions look like they can be replaced with making the base class keep a struct of properties of the variant, and if everything can be so arranged, then it would probably make more sense to drive this logic via a tablegen file that generates the data table rather than a nest of virtual functions to achieve the same result. I would strongly implore you to consider if tablegen could be used to generate this information, and to use tablegen instead if it can, for that same tablegen file could then be used to automatically produce sorely lacking user documentation in a way that cannot be done for C++ class implementations.
On more minor code style points, LLVM generally prefers to use “curiously recurring template parameters” to implement visitors instead of virtual functions, LLVM must be buildable and work without the use of RTTI (it has its own system to provide something similar where necessary), and you will want to move the definitions of virtual destructors out of header files to make dynamic linking work correctly.
First of all, using virtual functions and classic inheritance is easier for understanding than CRTP. I see no significant reason to use CRTP. Since this part of code is not as hot as AST or MC, I suppose it would be almost impossible to notice any performance issues.
The second thing, the reason I used virtual functions instead of some enums or fields is to make classes as stateless as possible. Which leads to easier debugging and testing.
The third thing, I used RTTI because the operator== in IArchitecture class uses typeinfo as the only thing it can determine equality, since the class is stateless at general. The RTTI is used in only one particular module and can not lead to significant increase of the size of the binaries.
Moreover, since there is no heavy template magic in the module, the monomorphisation is not needed, so there is no any binary size explosions because of it. I think we can make two deals with one shot, and get full-functional RTTI and all benefits of virtual functions with the same performance and binary size.
I would say, many of code restrictions in llvm project are heavily outdated and ancient. Now we have powerful computers, we don’t need to fight for each microsecond. Another thing is about pragma once and so far. You say ‘llvm have to be built with any compiler’, but if you’d take a look at the list of compilers, which can build c++17, you could see that ALL of them support pragma once.
In conclusion, I think now we have abilities to write better code for people to read and maintain. The policies have been made to help us maintain the code, but not to restrict the evolution of it.
P.S. I going to design and draw some diagrams in nearest future.
If you want to evolve the developer guide, it is here: LLVM Programmer’s Manual — LLVM 17.0.0git documentation
You can submit a dedicated RFC for this, but I’d keep this a separate discussion because trying to go against it right now isn’t gonna help your change getting through.
D149941 adds new code and the abstractions appear to add more complexity in my view. llvm/lib/TargetParser/Triple.cpp has some complex parts (e.g. normalize), but I think they can be improved incrementally.
I think that whatever solution you come up with should be measured against the existing implementation both in terms of performance and size. To give some context, LLDB and related tools use llvm::Triple quite extensively. Mostly we rely on llvm::Triple::normalize and comparing portions of triples against enumeration values (or even asking if they were specified in the first place). I’m not sure how many times a compiler need to parse triples or compare their elements when compiling one TU, but the number of triples LLDB may need to parse could be as many as the number of images in a loaded program, sometimes more. For a better idea of how LLDB uses llvm::Triple today, you can look at the ArchSpec class at lldb/source/Utility/ArchSpec.cpp. [EDIT: I re-read parts of LLDB and there are plenty of situations where the number of times we have to parse triples is still pretty low, but I still think we should consider the performance and size implications]
Additionally, while LLDB usually runs on “powerful computers”, lldb-server may run in more constrained environments. What may be a small amount of time on my laptop could end up being noticeable on an older mobile phone or some small development hardware. lldb-server is also already quite large (last I saw, somewhere in the 40MB range when built release+stripped). If you increase that size significantly you may receive some pushback.
I don’t want to discourage you from refactoring code for the better, but I think this context could help you make more informed decisions when weighing the tradeoffs.
Since the design is not obvious at first glance I’ve made the diagram for it. There is only Architecture-related part of the Triple, and other parts still is to be designed.
