Aw, but i had a list of issues, such as:
- tablegen doesn’t generate .d filesActually, it does if asked to.
Oh, thanks. Thats good to know.
I don’t know if ninja uses the .d as the source of real deps, or if it still includes those deps emitted by CMake. Will take a look.
Thanks,
Pete
ninja certainly supports reading dependencies from .d files. I think the actual problem here is that there is no way to have cmake's add_custom_command() generate the necessary ninja depfile/deps directives.
- Matthias
add_custom_command(DEPFILE) is available with CMake>=3.7 and generator is Ninja.
I have a patch for this and this is working fine with “*-tblgen -d”
This can reduce edges between generated.inc and *.td(s), though, I think this won’t be so useful to reduce turn-around build time.
Thanks, Takumi
Just a note. Support used to be two separate libraries, support and system.
These were merged (7 years ago?) due to the need for and existence of cross
dependencies between them. I would be concerned with any split that brought
back these issues where adding an include changed which library something
needed to be in.
- Michael Spencer
Was this any different than the existing cross-dependencies that exist between ADT and Support?
While it would be nice to easily use parts of support outside of llvm (as
I've done on many occasions), I'm not sure the llvm project wants to
maintain and support a generic c++ library. Given that I don't think use
outside of llvm is a great line to split on.
- Michael Spencer
Was this any different than the existing cross-dependencies that exist
between ADT and Support?
The difference between ADT and Support is just which directory the headers
are in. system and support were different .a/.so, which would be required
to break the build dependency.
- Michael Spencer
Let me try starting over without mentioning Tablegen
Aw, but i had a list of issues, such as:
- tablegen doesn’t generate .d files
- CMake is set up with far reaching deps on .td files, so changing Options.td rebuilds the backends, and adding an ARM intrinsic rebuilds X86 or vice versa
- All the tablegen backends are linked in to a single binary, so if you are hacking on tablegen and change the disassemblers backend, you rebuild the SelectionDAG code
Is Support too big, and if so should we consider breaking it up?
Yes please. Even if it didn’t have a signifiant impact on build time, its become something of a dumping ground for code which would otherwise cause layering violations if put somewhere else.
The way I see it, there’s a bunch of functionality which effectively just wraps the platform, (i.e., anything which abstracts away posix/unix vs Windows vs macOS). This is often stuff like file systems, streams, the host itself and probably a few others. Then there is obviously ADT. There are utilities which basically everyone uses like Error, Debug, Statistic and stack trace. There are utilities which only tools should use, like CommandLine, and utilities only used by a few projects right now, but maybe more in future, like Threading.
Then there are things which should now live elsewhere because perhaps other libraries exist now which didn’t before. ELF.h and others should be in libObjectFile. If they can’t be there, then we should have a very good reason why not.
BranchProbability shouldn’t be there at all but probably is due to layering, and likely the same goes for TargetRegistry. We should fix that. Same goes for ARM* which should all be in the ARM backend.
Dwarf should be in libDebugInfoDWARF.
I think the reason why it isn’t there is because lib/DebugInfo is for consuming debug info, but these definitions are useful for both reading and writing DWARF. That is not to say that they couldn’t be moved somewhere else of course (but probably not lib/DebugInfo in its current form).
– adrian
2017-05-26 17:47 GMT-07:00 Zachary Turner via llvm-dev <
llvm-dev@lists.llvm.org>:Changing a header file somewhere and having to spend 10 minutes
waiting for a build leads to a lot of wasted developer time.The real culprit here is tablegen. Can we split support and ADT into
two - the parts that tablegen depends on and the parts that it doesn't?Splitting ADT just based on tablegen usage seems dubious to me. If we
need to go this route, I'd replace as many uses of ADT data structure with
STL ones to begin with to reduce the surface.Tablegen would not need to determine WHERE to split, it would just
motivate the why.Well even the why
(note I was mentioning ADT and not Support above).It's obvious just from looking at Support's include directory though
that a lot of stuff in there doesn't belong together. A quick look over
the include directory already suggests a split into "broadly useful stuff"
and "narrowly useful stuff"I agree, Support is a mess IMO (we have target specific stuff here just
for the sake of sharing code with clang AFAIK) and I'm not sure anyone
would oppose to split it. Ideally the way I would split it would be such
that it could (at some point) be useful outside of LLVM (just like ADT), so
one main criteria could be "could this component of Support be useful
outside of LLVM (and its subprojects)".While it would be nice to easily use parts of support outside of llvm (as
I've done on many occasions), I'm not sure the llvm project wants to
maintain and support a generic c++ library.
Sorry, I'm not sure to follow you, because my impression is that we're
*already* maintaining a generic C++ library in-tree. The fact that the mess
that is libSupport today does not allow to split ADT (or other generic
utilities in libSupport) out-of-tree easily does not change that.
Given that I don't think use outside of llvm is a great line to split on.
I think on the opposite that it is a valuable line because it matches a
conceptual layer.
I’m somewhere in the middle. I don’t think we need to maintain build files, documentation, upstream buildbots etc for using LLVMSupport out of tree, but I do like the conceptual separation and I think “is this useful outside of LLVM” is a good conceptual litmus test for what should go in such a hypothetical library.
Right, to clarify: that is exactly my view as well at this point 
Dwarf should be in libDebugInfoDWARF.
I think the reason why it isn’t there is because lib/DebugInfo is for consuming debug info, but these definitions are useful for both reading and writing DWARF. That is not to say that they couldn’t be moved somewhere else of course (but probably not lib/DebugInfo in its current form).
Ah, didn’t know that. I’m sure a bunch of the others I mentioned also have similar reasons for being where they are.
Ultimately its a judgement call, but i’d say that libDebugInfoDWARF is a less bad place for this file than libSupport. Not a great reason to move it, but if someone wants to I still wouldn’t personally object.
Cheers,
Pete
Dwarf should be in libDebugInfoDWARF.
I think the reason why it isn’t there is because lib/DebugInfo is for consuming debug info, but these definitions are useful for both reading and writing DWARF. That is not to say that they couldn’t be moved somewhere else of course (but probably not lib/DebugInfo in its current form).
Ah, didn’t know that. I’m sure a bunch of the others I mentioned also have similar reasons for being where they are.
Ultimately its a judgement call, but i’d say that libDebugInfoDWARF is a less bad place for this file than libSupport. Not a great reason to move it, but if someone wants to I still wouldn’t personally object.
That would seem to have a concrete disadvantage, though - that’d make many things dependent on libDebugInfo that aren’t currently - so there’d be real changes in build time, etc building all the DWARF parsing/dumping/etc API code for users who only generate DWARF but don’t parse it.
If it were moved, it’d probably want to be a separate library that both DWARF reading libraries and DWARF writing libraries could be dependent on. (unless the reading/writing could be tightened up to the point where it was mostly a common library used for both sides)
I would probably just make a new library called ObjectDef or ObjectFormat and put ELF.h, COFF.h, DWARF.h, etc all there. Doesn’t even need to produce a .a file
Dwarf should be in libDebugInfoDWARF.
I think the reason why it isn’t there is because lib/DebugInfo is for consuming debug info, but these definitions are useful for both reading and writing DWARF. That is not to say that they couldn’t be moved somewhere else of course (but probably not lib/DebugInfo in its current form).
Ah, didn’t know that. I’m sure a bunch of the others I mentioned also have similar reasons for being where they are.
Ultimately its a judgement call, but i’d say that libDebugInfoDWARF is a less bad place for this file than libSupport. Not a great reason to move it, but if someone wants to I still wouldn’t personally object.
That would seem to have a concrete disadvantage, though - that’d make many things dependent on libDebugInfo that aren’t currently - so there’d be real changes in build time, etc building all the DWARF parsing/dumping/etc API code for users who only generate DWARF but don’t parse it.
Thats a fair point. And ultimately something we could check for before moving it (see who is using Dwarf.o but not libDebugInfoDwarf).
The other place for this particular case (and i’m not really trying that hard to move this one file, just discussing merits), would be libCodeGen or even libMC. For better or worse, everyone who emits dwarf today does it with the MC layer, even dsymutil unfortunately.
Pete
In all the comments downthread, I think there is one thing that hasn't been mentioned: doing a split like this makes tblgen evolution more difficult. If libsupport was split into “used by tblgen” and “not used by tblgen” sections, and then a new tblgen feature needs to use other parts of libsupport, they’d have to be moved into the “used by tblgen” directory.
Splitting libsupport as a whole out into its own llvm subproject has come up many times though, and does make a lot of sense.
-Chris
Fair enough, i sort of regret mentioning that specific method of splitting originally.
For the record, i think any splitting should make sense on its own merit without considering tablegen, and hopefully the end result of “tablegen eventually depends on less stuff” would happen naturally
So, here is an example of where I think a split would be really helpful.
https://reviews.llvm.org/D34667
This code would benefit vastly even from just being able to use StringRef and ArrayRef. We have other cases as well where we export some code that cannot depend on the rest of LLVM.
Thinking about it some, StringRef, ArrayRef, and various other things like STLExtras and iterator.h basically can be summarized as “things that are either already already planned for, or wouldn’t be entirely out of place in the STL itself”. For example, StringRef is std::string_view. ArrayRef is std::array_view. iterator_facade_base is a better version of std::iterator.
So I would drop my suggestion to split the libraries in such a way that it might benefit TableGen, and instead re-word my suggestion to include only classes such as StringRef, ArrayRef, and other STL-like utilities that can benefit utilities like our demangler etc that cannot depend on the rest of LLVM. If and when we ever require C++17 for building LLVM (a long ways away, obviously, but we might as well be forward thinking), we would certainly be able to use std::string_view and std::array_view in the demangler. So splitting things in a way such as this makes long term sense IMO.
Zachary Turner via llvm-dev <llvm-dev@lists.llvm.org> writes:
So, here is an example of where I think a split would be really helpful.
https://reviews.llvm.org/D34667
This code would benefit vastly even from just being able to use StringRef
and ArrayRef. We have other cases as well where we export some code that
cannot depend on the rest of LLVM.Thinking about it some, StringRef, ArrayRef, and various other things like
STLExtras and iterator.h basically can be summarized as "things that are
either already already planned for, or wouldn't be entirely out of place in
the STL itself". For example, StringRef is std::string_view. ArrayRef is
std::array_view. iterator_facade_base is a better version of
std::iterator.So I would drop my suggestion to split the libraries in such a way that it
might benefit TableGen, and instead re-word my suggestion to include only
classes such as StringRef, ArrayRef, and other STL-like utilities that can
benefit utilities like our demangler etc that cannot depend on the rest of
LLVM. If and when we ever require C++17 for building LLVM (a long ways
away, obviously, but we might as well be forward thinking), we would
certainly be able to use std::string_view and std::array_view in the
demangler. So splitting things in a way such as this makes long term sense
IMO.
+1. This way of splitting things makes a ton of sense to me. The generic
language support parts of ADT and Support would be useful in a number of
places where we're reluctant to pull something as heavy as libSupport
in.
Yes, that proposal makes sense to me: the split would be between things that are known to be subsumed into later versions of C++, and therefore are a compatibility library.
What do you think about this as an implementation approach:
This allows us to have a clean path out of these custom types, and makes it very clear that these headers are compatibility shims that go away in the future. It also makes it clear what the division is.
-Chris