[PATCH] D83069: [lit] warn if explicitly specified test won't be run indirectly

[ben via Phabricator via llvm-commits]

bd1976llvm marked an inline comment as done.
bd1976llvm added inline comments.


================
Comment at: llvm/utils/lit/lit/discovery.py:162
+            for res in lc.test_format.getTestsInDirectory(ts, test_dir_in_suite,
+                                                          litConfig, lc):
+                if test.getFullName() == res.getFullName():
----------------
thakis wrote:
> This looks potentially slow. Did you do any perf measurements for this patch? What's lit test discovery time for some target (say, check-llvm) for this test vs without? (min-of-5, or ministat)
Sorry for the slow reply.

I didn't measure any speed difference with check-llvm; that was because check-llvm does not invoke single tests directly (which is the only code path that this change impacts). Instead, I picked a directory with a large number of tests (llvm/test/CodeGen/X86/) and added a new test that would trigger the warning. I recorded these numbers running that test:

min-of-five without this change:
```
~/u/1$ time ~/u/build/bin/llvm-lit llvm/test/CodeGen/X86/zlib-longest-match.not 
-- Testing: 1 tests, 1 workers --
PASS: LLVM :: CodeGen/X86/zlib-longest-match.not (1 of 1)

Testing Time: 0.22s
  Expected Passes: 1

real    0m0.295s
user    0m0.233s
sys 0m0.035s
```
min-of-five with this change:
```
~/u/1$ time ~/u/build/bin/llvm-lit llvm/test/CodeGen/X86/zlib-longest-match.not 
llvm-lit: discovery.py:169: warning: 'LLVM :: CodeGen/X86/zlib-longest-match.not' would not be run indirectly: change name or LIT config
-- Testing: 1 tests, 1 workers --
PASS: LLVM :: CodeGen/X86/zlib-longest-match.not (1 of 1)

Testing Time: 0.22s
  Expected Passes: 1

1 warning(s) in tests

real    0m0.335s
user    0m0.274s
sys 0m0.030s
```

I also tested on windows (where filesystem operations are often more expensive) and recorded similar numbers.

So, there is a performance impact. Personally, I think this is acceptable *unless* users are listing large numbers of tests individually on their LIT command lines. That seems unlikely (IMO). However, if that is an important use-case then I think that emitting this warning only if a single test has been specified would be reasonable.


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

https://reviews.llvm.org/D83069