Skip to content

Conversation

jurahul
Copy link
Contributor

@jurahul jurahul commented Aug 28, 2025

Change add_lit_testsuites to accept either a set of include or a set of exclude directories and use that to specialize check-llvm-tablegen and check-llvm-filecheck targets to use a trimmed down set of dependencies. The check-llvm-filecheck trimming is just for demonstration purpose to show how > 1 testsuites can have smaller set of dependencies than the default LLVM_TEST_DEPENDS.

Note that building check-llvm-filecheck for example when not all LLVM tools have been built results in warnings like:

llvm-lit: <dir>/llvm-project/llvm/utils/lit/lit/llvm/subst.py:126: note: Did not find lli in <dir>/build/bin
llvm-lit: <dir>/llvm-project/llvm/utils/lit/lit/llvm/subst.py:126: note: Did not find llc_dwarf in <dir>/build/bin
llvm-lit: <dir>/llvm-project/llvm/utils/lit/lit/llvm/subst.py:126: note: Did not find llvm-objcopy in <dir>t/build/bin

but the lit tests run fine since all the dependencies specified do get rebuilt.

@jurahul jurahul marked this pull request as ready for review August 28, 2025 22:45
@llvmbot llvmbot added the cmake Build system in general and CMake in particular label Aug 28, 2025
@joker-eph
Copy link
Collaborator

I'm confused by the title, shouldn't it be instead "Add support to define lit test suites with an include/exclude list of folders" or something like this? I don't see the "dependency" part here.

@mshockwave
Copy link
Member

mshockwave commented Aug 29, 2025

I'm confused by the title, shouldn't it be instead "Add support to define lit test suites with an include/exclude list of folders" or something like this? I don't see the "dependency" part here.

I believe the DEPENDS fields of these newly-added add_list_testsuites are different from the original one -- they only depends on a (much) smaller set of tools.
The new EXCLUDE_DIR / INCLUDE_DIR is just a mean to the end I think, helping us to split the test suites into different portions (which have different set of dependencies)

@joker-eph
Copy link
Collaborator

joker-eph commented Aug 29, 2025

OK but it still does not seem to be "Add support to define lit test suites with ..." but "Add LLVM lit test suites with ..."?
I would think that either we describe the "added support" which is about include/exclude or about the feature which is "adding lit test suites with ".

@jurahul
Copy link
Contributor Author

jurahul commented Aug 29, 2025

Right, maybe this works:

Specialize LLVM lit test suites for TableGen and FileCheck with a smaller set of dependencies?

@jurahul jurahul changed the title [LLVM] Add support to define lit test suites with a smaller set of dependencies [LLVM] Specialize LLVM lit test suites for TableGen and FileCheck with a smaller set of dependencies Aug 29, 2025
@s-barannikov
Copy link
Contributor

but the lit tests run fine since all the dependencies specified do get rebuilt.

Or because the dependencies were found in the system.

I've once ran into this with llc missing from clang/test/lit.cfg.py (still not fixed). Spent an hour or so figuring out why the tests fail, and then another hour figuring out why llvm-lit uses llc installed in the system. (I must have done something obscure because llc is clearly in CLANG_TEST_DEPS, like deleting llc from disk or renaming the build directory before running the tests.)

These warnings should not be ignored and they can be confusing. Instead, we should specialize the list of needed tools per subdirectory (is it possible to add something to lit.local.cfg to achieve that?).

Not a blocker to me, but others may find the warnings confusing.

@jurahul
Copy link
Contributor Author

jurahul commented Aug 29, 2025

but the lit tests run fine since all the dependencies specified do get rebuilt.

Or because the dependencies were found in the system.

I've once ran into this with llc missing from clang/test/lit.cfg.py (still not fixed). Spent an hour or so figuring out why the tests fail, and then another hour figuring out why llvm-lit uses llc installed in the system. (I must have done something obscure because llc is clearly in CLANG_TEST_DEPS, like deleting llc from disk or renaming the build directory before running the tests.)

These warnings should not be ignored and they can be confusing. Instead, we should specialize the list of needed tools per subdirectory (is it possible to add something to lit.local.cfg to achieve that?).

Not a blocker to me, but others may find the warnings confusing.

Right, so assuming the list of dependencies is correct, we get a warning for the unused dependencies and its benign. If the list of dependencies itself is missing a dependency, then you're right that this will end up using the command without substitution (from somewhere else or even from the same build/bin directory, just that it hasn't been rebuilt). This does not address that (or make it worse as well). Again, assuming the trimmed list of dependencies is kept correct

@jurahul jurahul changed the title [LLVM] Specialize LLVM lit test suites for TableGen and FileCheck with a smaller set of dependencies [LLVM] Specialize test suites for TableGen and FileCheck to use smaller set of dependencies Aug 29, 2025
@s-barannikov
Copy link
Contributor

Right, so assuming the list of dependencies is correct, we get a warning for the unused dependencies and its benign.

This is a very strong assumption? The list may be incomplete from the start or become outdated (unlikely for TableGen/FileCheck, but for other subdirectories -- not impossible).

This does not address that (or make it worse as well).

I imagine one big list makes it less likely that there is something missing from it.

@jurahul
Copy link
Contributor Author

jurahul commented Aug 29, 2025

I agree that one giant list makes it less likely than several smaller lists and it's more likely that one of the smaller lists can become outdated. This was discussed on the discourse thread and was agreed as a practical but imperfect solution.

@llvm-beanz
Copy link
Collaborator

I understand the utility of what you're trying to do here, but I worry that if we extrapolate this pattern out across the test subdirectories we're going to create an unmanageable mess.

Every subdirectory under the test suite gets its own target, and if each target gets configured for its own dependency subset manually we're creating a real mess to maintain.

@mshockwave
Copy link
Member

I understand the utility of what you're trying to do here, but I worry that if we extrapolate this pattern out across the test subdirectories we're going to create an unmanageable mess.

Every subdirectory under the test suite gets its own target, and if each target gets configured for its own dependency subset manually we're creating a real mess to maintain.

For codegen and the middle-end tests like Analysis and Transforms, this (relatively short) list of dependencies doesn't change really frequently, so personally I'm not super worry about managing lists in those places. That being said, some places like MC might have a more diversified dependencies.
I think a tradeoff we can make here is to limit the granularity, for instance limiting to top-level directories immediate under test.

@jurahul
Copy link
Contributor Author

jurahul commented Aug 29, 2025

Right, it's not a perfect solution, but as @mshockwave said limiting it to top-level directories only (I can add a comment to say so) can both avoid the issue @llvm-beanz is worried about and also such specialization is likely only needed in that scenarios for coarse level separation of dependencies. So it's a combination of having the ability and some discipline on our side to limit this to top level directories.

@jurahul
Copy link
Contributor Author

jurahul commented Sep 2, 2025

@llvm-beanz will restricting specialization to top-level directories only address your concerns?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake Build system in general and CMake in particular
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants