- Remove duplication: Both TestingGuide and TestSuiteMakefileGuide would give a similar overview over the test-suite.
- Present cmake/lit as the default/normal way of running the test-suite:
- Move information about the cmake/lit testsuite into the new TestSuiteGuide.rst file. Mark the remaining information in TestSuiteMakefilesGuide.rst as deprecated.
- General simplification and shorting of language.
- Remove paragraphs about tests known to fail as everything should pass nowadays.
- Remove paragraph about zlib requirement; it's not required anymore since we copied a zlib source snapshot into the test-suite.
- Remove paragraph about comparison with "native compiler". Correctness is always checked against reference outputs nowadays.
- Change cmake/lit quickstart section to recommend pip for installing lit and use CMAKE_C_COMPILER and a cache file in the example as that is what most people will end up doing anyway. Also a section about compare.py to quickstart.
- Document Bitcode and MicroBenchmarks directories.
- Add section about commonly used cmake configuration options.
- Add section about showing and comparing result files via compare.py.
- Add section about using external benchmark suites.
- Add section about using custom benchmark suites.
- Add section about profile guided optimization.
- Add section about cross-compilation and running on external devices.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
AWESOME!
Very detailed, using the modern infrastructure and helpful even to those not using clang or wanting to run external benchmarks.
I haven't reviewed the whole text (non-English speaker), nor I ran the commands to validate the setup (I'm assuming you did, extensively!:), but this looks right on the money. Thank you!
Feel free to ping me again if no one takes any interest in this, but for the record, overall, LGTM.
Thanks,
--renato
docs/CMake.rst | ||
---|---|---|
613 | Do we want at least a hint here that there are more tests? A link to the new page with a short paragraph would do. |
Thanks a lot Matthias, this looks really great! I hopefully did not miss anything, it LGTM!
docs/TestSuiteGuide.rst | ||
---|---|---|
113 ↗ | (On Diff #163218) | We could have a link to External Suites somewhere here. |
I am as excited by this as the other reviewers.
Thank you very much for this, Matthias!
I only have a few nitpicks, see comments inline. Feel free to ignore my nitpicks - they're mostly personal opinion on my side, not essential.
Thanks!
Kristof
docs/TestSuiteGuide.rst | ||
---|---|---|
19–20 ↗ | (On Diff #163218) | I tend to prefer documentation that shows how to set up a virtualenv, so that copy pasting commands from the documentation doesn't change your system set up by default. I'm happy either way - just thought I'd share my thought. |
33 ↗ | (On Diff #163218) | s/Use/use/ |
134 ↗ | (On Diff #163218) | s/option/cmake option/ ? |
288 ↗ | (On Diff #163218) | Maybe a pointer to LNT might be worthwhile here as a system that knows how to drive running the test-suite, collecting results, storing in a database, and analyzing it further? |
402–407 ↗ | (On Diff #163218) | I wonder if it wouldn't be better to have this note at the start. |
Thanks for investing time into the documentation
docs/TestSuiteGuide.rst | ||
---|---|---|
55 ↗ | (On Diff #163218) | There will be no more TEST_SUITE_HOST_CC after D51080. |
224 ↗ | (On Diff #163218) | The file cachefile.cmake does not exist. Maybe use an existing example such as Release.cmake or use a wildcard/placeholder such as <cachefile>.cmake? |
309–316 ↗ | (On Diff #163218) | Maybe it is worth pointing out that one cannot point into arbitrary subdirs of SingleSource/MultSource/MicroBenchmarks/Bitcode since these have custom lit.local.cfg on their level. (CTMark unfortunately skips MultiSource/lit.local.cfg) |
344 ↗ | (On Diff #163218) | ... SPEC benchmark suites |
Looks great. Thank you for updating the docs.
docs/TestSuiteGuide.rst | ||
---|---|---|
19–20 ↗ | (On Diff #163218) | Is installing lit directly from llvm a hard requirement? |
Added some style nits.
They're mostly personal though, so LGTM.
docs/CMake.rst | ||
---|---|---|
612–613 | Capitalization: Executing the Tests | |
docs/SourceLevelDebugging.rst | ||
118–120 | We should make it clear that when we say "test-suite" later in the docs, we're talking about this. :doc:LLVM test suite <TestSuiteMakefileGuide> (test-suite) Also we should choose one of "test-suite" and "test suite" and stick to it throughout the document. | |
119 | "to test the optimizer's handling" Also maybe active voice would be better here? "The LLVM test suite provides a framework for testing how the optimizer handles debugging information." | |
docs/TestSuiteGuide.rst | ||
12 ↗ | (On Diff #163218) | This is kind of awkward given that the list below is partially a list of necessary things, and partially a list of instructions. Might be good to revise the items in a way that makes it sound like a list of necessary things. e.g
etc. |
45 ↗ | (On Diff #163218) | I think this shouldn't be in the same list as above, because now you're not describing what you need, but rather how to run the test suite. It can probably be broken up with something like "After this, you can build and execute the test suite as follows..." |
90–91 ↗ | (On Diff #163218) | "a number" isn't necessary. The `test-suite` module contains programs that can be compiled and executed. |
92 ↗ | (On Diff #163218) | Avoid starting sentences with "and" if possible. This can also be simplified: |
100 ↗ | (On Diff #163218) | "such" isn't necessary |
117–118 ↗ | (On Diff #163218) | Which other means? |
132 ↗ | (On Diff #163218) | some of the programs -> some programs |
140 ↗ | (On Diff #163218) | We should be consistent when using "suite" to refer to the test suite and "test-suite". There are a few places where you use "suite", and a few where you use "test-suite". I think that just using "test-suite" everywhere would be more consistent. |
157–158 ↗ | (On Diff #163218) | Second sentence can be simplified. "These flags are also passed to C++ compiler and linker invocations." |
189 ↗ | (On Diff #163218) | Semicolon separated -> Semicolon-separated |
191 ↗ | (On Diff #163218) | Can be simplified: "Using this option does not work reliably with deeper subdirectories..." (This could probably be split into two sentences, but meh.) |
197 ↗ | (On Diff #163218) | Colon should be a period. |
209 ↗ | (On Diff #163218) | coming with -> that comes with |
226 ↗ | (On Diff #163218) | After a colon (or semicolon), you don't have to capitalize the first word. e.g The test-suite comes with several cache files: they... |
227 ↗ | (On Diff #163218) | This can be simplified: "Use a CMake cache. The test-suite comes with several CMake caches which predefine common or tricky build configurations." |
234 ↗ | (On Diff #163218) | "little" is unnecessary |
235 ↗ | (On Diff #163218) | "You should" is unnecessary. "Invoke `lit`..." |
293 ↗ | (On Diff #163218) | remove semicolon; they should only be used before a list when you have a full sentence |
298 ↗ | (On Diff #163218) | Versions of what? |
309–310 ↗ | (On Diff #163218) | Can be simplified: Custom tests must contain a `CMakeLists.txt file at the top directory. The CMakeLists.txt` file will be automatically picked up... |
322 ↗ | (On Diff #163218) | colon -> no capitalization. Should probably be a proper sentence anyway (with a period). This gives better separation of the two-item list. Right now the structure is like [statement of list + item 1][item 2] While if we split it up, it'd be more [statement of list][item 1][item 2] |
326 ↗ | (On Diff #163218) | run: Example -> run. Example: Also "Example:" probably deserves its own paragraph. (I'd normally say remove the semicolon too, since it's not a sentence. Idiomatically speaking though, people write "Example:" all the time.) |
344 ↗ | (On Diff #163218) | Should this be .. NOTE:: like on line 402? |
360 ↗ | (On Diff #163218) | Should be consistent in hyphenation of cross-compilation. Above, this, there's no hyphen twice. |
362 ↗ | (On Diff #163218) | don't need to capitalize "however" |
367 ↗ | (On Diff #163218) | Don't need "currently". |
369 ↗ | (On Diff #163218) | Capitalize SSH. Can be simplified: "Via SSH connection to an external device." |
370 ↗ | (On Diff #163218) | ssh->SSH host name -> hostname |
372–374 ↗ | (On Diff #163218) | I think that this can be split up. "After this, the lit runner can be used on the host machine. The lit runner will prefix benchmark and verification command lines with a `ssh` command." Also I think "Example:" deserves its own paragraph. |
391 ↗ | (On Diff #163218) | Hmmmm, capitalization, if we're not committed to "test-suite": Otherwise: |
394–395 ↗ | (On Diff #163218) | simplification: "The LNT tool can run the test-suite. Use this when submitting results to a LNT database." |
399 ↗ | (On Diff #163218) | Same as the other capitalization confusion. If it's "test-suite", then it's fine since that's the name... |
407 ↗ | (On Diff #163218) | Simplification: |
docs/TestSuiteMakefileGuide.rst | ||
91–92 | Running Different Tests | |
111–112 | Generating Test Output | |
137–138 | Writing Custom Tests for the Test Suite or Writing Custom Tests for the test-suite | |
docs/TestingGuide.rst | ||
29 | LLVM Testing Infrastructure Organization | |
98–100 | Simplification: |
Thanks for the detailed review comments!
- Convert document to markdown (I assume that is the recommended way to do things after rL338977).
- Addressed review feedback.
docs/CMake.rst | ||
---|---|---|
613 | There is already a link to the TestingGuide two paragraphs below which explains the differences between the unit tests, lit tests and the test-suite. (Also I my original intention with this patch was not to change any of the other documentation files; but this heading felt so misleading because the whole section was about the lit tests and not the test-suite...) | |
docs/SourceLevelDebugging.rst | ||
119 | I changed it, though again I would like to keep changes in unrelated parts of the documentation minimal. This one just needed to be changed to reference the makefile guide, as the modern cmake/lit suite has no equivalent for this testing mode yet... | |
docs/TestSuiteGuide.rst | ||
12 ↗ | (On Diff #163218) | I just went ahead and dropped the whole sentence :) |
19–20 ↗ | (On Diff #163218) | Oh, I thought we stopped distributing lit to pypi out of concerns that without update processes in place it would get out of date. Indeed pip install lit still works today, but I think I'm not gonna suggest it :) |
19–20 ↗ | (On Diff #163218) | Thinking about this again, I would expect most people just use llvm-lit from their llvm build directory. I thus present that as the first possibility now and changed the name to llvm-lit in all the examples. I present a virtualenv based setup as an alternative now. |
45 ↗ | (On Diff #163218) | As above, I hope it makes now sense as a step by step introduction. |
55 ↗ | (On Diff #163218) | See my comment there. |
90–91 ↗ | (On Diff #163218) | I took the recommendation and also dropped "module". |
113 ↗ | (On Diff #163218) | added. |
117–118 ↗ | (On Diff #163218) | Dropped the sentence since we have a link to whole section now which explains things. |
140 ↗ | (On Diff #163218) | I changed most of them to test-suite now. |
157–158 ↗ | (On Diff #163218) | Yeah I added the awkward "currently the test-suite" because I always considered this behavior strange/odd and because it is specific to the test-suite and does not happen in any cmake project... But indeed this is not the place to leak these opinions and it's unlikely that we actually change it given that it would be quite a churn on users... Will go for the simpler sentence. |
288 ↗ | (On Diff #163218) | I added a section about LNT immediately below this. |
309–316 ↗ | (On Diff #163218) | I mentioned this when describing the TEST_SUITE_SUBDIRS option. I'm not sure I want to repeat it here as this section is about using custom suites not subdirectories of the test-suite. (And I would consider CTMark lacking its own lit.local.cfg a bug. I guess we only ever used it as compile time benchmark and never tried running it. Will fix this soon.) |
344 ↗ | (On Diff #163218) | I don't think this particular one needs the emphasis of a whole block. I removed the "Note:" now. |
362 ↗ | (On Diff #163218) | used lowercase and dropped "however". |
402–407 ↗ | (On Diff #163218) |
|
Thanks for working on this! LGTM
docs/TestSuiteGuide.md | ||
---|---|---|
34 | Do we want to mention that the test-suite should be able to build with other compilers? |
- Address remaining review feedback, shorten some terminal dumps.
- I will commit this after landing D51080 (the remote section here describes the situation after that patch)
docs/TestSuiteGuide.md | ||
---|---|---|
34 | Do you know whether the test-suite actually works with non-clang compilers? I can imagine it mostly works, but I also wouldn't be surprised if some clang specific flag or extension is used in few of the benchmarks... I changed the wording to "custom compiler" but will not explicitely call out support for non-clang compilers for now. |
docs/TestSuiteGuide.md | ||
---|---|---|
34 | As you suspect it mostly works but gets some errors. Also after fixing the errors you still get some misleading performance results. Some tests use clang specific pragmas and one test just prints the reference output if you are not using clang. The language led me to believe the test-suite is only for clang. I had the impression the test-suite was not meant to be specific to clang (though its buggy on other compilers). I'm good with the wording as is and agree with not explicitly calling out support for non-clang compilers. |
docs/TestSuiteGuide.md | ||
---|---|---|
34 | I'd accept patches to the test-suite that disable clang specific benchmarks if someone cares enough to make them :) (Should be possible to do in a similar way like TEST_SUITE_BENCHMARKING_ONLY disables programs unsuitable for performance testing) |
Do we want at least a hint here that there are more tests? A link to the new page with a short paragraph would do.