Page MenuHomePhabricator

[lit] warn if explicitly specified test won't be run indirectly
Needs ReviewPublic

Authored by bd1976llvm on Jul 2 2020, 11:45 AM.

Details

Summary

It's relatively easy to write a lit test that won't actually be run. I did in: https://reviews.llvm.org/D82567

This patch adds a warning to avoid users doing that.

Diff Detail

Event Timeline

bd1976llvm created this revision.Jul 2 2020, 11:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2020, 11:45 AM

You said to me offline when you were discussing this that you were concerned by performance implications. I take it this isn't the approach you had then, or you changed your mind?

You said to me offline when you were discussing this that you were concerned by performance implications. I take it this isn't the approach you had then, or you changed your mind?

That was an earlier version (where I was creating a set of all possible tests and then checking that the specified test wasn't in it). Performance seems acceptable now.

thakis added inline comments.Sat, Jul 11, 7:29 PM
llvm/utils/lit/lit/discovery.py
162

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)

bd1976llvm marked an inline comment as done.Mon, Jul 20, 12:05 PM
bd1976llvm added inline comments.
llvm/utils/lit/lit/discovery.py
162

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.