This is an archive of the discontinued LLVM Phabricator instance.

Revamp test-suite documentation
ClosedPublic

Authored by MatzeB on Aug 29 2018, 3:56 PM.

Details

Summary
  • 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.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB created this revision.Aug 29 2018, 3:56 PM
MatzeB edited reviewers, added: paquette; removed: jpaquette.Aug 29 2018, 3:56 PM

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
606 ↗(On Diff #163218)

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.

thegameg accepted this revision.Aug 30 2018, 5:03 AM

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.

This revision is now accepted and ready to land.Aug 30 2018, 5:03 AM
kristof.beyls accepted this revision.Aug 30 2018, 5:50 AM

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.
However, in this case, I can also see the argument to not do that and keep documentation more concise.

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.
I started thinking that when seeing that in docs/SourceLevelDebugging.rst in this patch, you changed a pointer to this guide, which explains the cmake/lit system, and then on the next line in docs/SourceLevelDebugging.rst, there is a command line with make, using the deprecated system.
Maybe it'll be less frustrating for people coming through that link to be notified immediately that there is an old, deprecated system based on make?

Meinersbur accepted this revision.Aug 30 2018, 9:26 AM

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

tra accepted this revision.Aug 30 2018, 9:47 AM

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?
I've been using pip install lit and that appears to work.

paquette accepted this revision.Aug 30 2018, 10:06 AM

Added some style nits.

They're mostly personal though, so LGTM.

docs/CMake.rst
605 ↗(On Diff #163218)

Capitalization:

Executing the Tests

docs/SourceLevelDebugging.rst
118 ↗(On Diff #163218)

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 ↗(On Diff #163218)

"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

#. The lit test runner.

... (stuff)

#. The test suite module.

... (stuff)

#. A build directory, configured with CMake.

... (stuff)

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:
"The suite comes with tools to collect metrics such as benchmark runtime, compilation time, and code size."

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":
Running the Test Suite via LNT

Otherwise:
Running the test-suite via LNT

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:
Old documentation is available in the :doc:TestSuiteMakefileGuide.

docs/TestSuiteMakefileGuide.rst
92 ↗(On Diff #163218)

Running Different Tests

112 ↗(On Diff #163218)

Generating Test Output

138 ↗(On Diff #163218)

Writing Custom Tests for the Test Suite

or

Writing Custom Tests for the test-suite

docs/TestingGuide.rst
29 ↗(On Diff #163218)

LLVM Testing Infrastructure Organization

98–99 ↗(On Diff #163218)

Simplification:
The `test-suite` module contains a more comprehensive test suite including whole C and C++ programs.

MatzeB updated this revision to Diff 163406.Aug 30 2018, 2:14 PM
MatzeB marked 50 inline comments as done.

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
606 ↗(On Diff #163218)

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 ↗(On Diff #163218)

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 :)
So now you can just read it as a step by step instruction list (instead of a mix of requirements and steps).

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)
  • I changed the SourceLevelDebugging.rst thing to point to the deprecated makefile guide not this new one. I don't think we have an equivalent to this specialized use case in the cmake/lit version yet.
  • I'd like to keep the section at the bottom as I'd prefer people to stumble upon this and know about it unless they really have to :)
homerdin accepted this revision.Aug 30 2018, 2:55 PM

Thanks for working on this! LGTM

docs/TestSuiteGuide.md
34 ↗(On Diff #163406)

Do we want to mention that the test-suite should be able to build with other compilers?

MatzeB updated this revision to Diff 163551.Aug 31 2018, 9:42 AM
MatzeB marked an inline comment as done.
  • 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 ↗(On Diff #163406)

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.

homerdin added inline comments.Aug 31 2018, 10:37 AM
docs/TestSuiteGuide.md
34 ↗(On Diff #163406)

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.

MatzeB marked 2 inline comments as done.Aug 31 2018, 10:43 AM
MatzeB added inline comments.
docs/TestSuiteGuide.md
34 ↗(On Diff #163406)

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)

This revision was automatically updated to reflect the committed changes.