[PATCH] D51465: Revamp test-suite documentation
Jessica Paquette via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 30 10:06:24 PDT 2018
paquette accepted this revision.
paquette added a comment.
Added some style nits.
They're mostly personal though, so LGTM.
================
Comment at: docs/CMake.rst:605
-Executing the test suite
-========================
+Executing the tests
+===================
----------------
Capitalization:
Executing the Tests
================
Comment at: docs/SourceLevelDebugging.rst:118
-The :ref:`LLVM test suite <test-suite-quickstart>` provides a framework to test
-optimizer's handling of debugging information. It can be run like this:
+The :doc:`LLVM test suite <TestSuiteMakefileGuide>` provides a framework to
+test optimizer's handling of debugging information. It can be run like this:
----------------
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.
================
Comment at: docs/SourceLevelDebugging.rst:119
+The :doc:`LLVM test suite <TestSuiteMakefileGuide>` provides a framework to
+test optimizer's handling of debugging information. It can be run like this:
----------------
"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."
================
Comment at: docs/TestSuiteGuide.rst:12
+
+To run the test suite, you need the following:
+
----------------
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.
================
Comment at: docs/TestSuiteGuide.rst:45
+
+#. Build the benchmarks:
+
----------------
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..."
================
Comment at: docs/TestSuiteGuide.rst:90-91
+
+The ``test-suite`` module contains a number of programs that can be compiled
+and executed. The programs come with reference outputs so that their
+correctness can be checked. And the suite comes with tools to measure aspects
----------------
"a number" isn't necessary.
The ``test-suite`` module contains programs that can be compiled and executed.
================
Comment at: docs/TestSuiteGuide.rst:92
+and executed. The programs come with reference outputs so that their
+correctness can be checked. And the suite comes with tools to measure aspects
+like benchmark runtime, compilation time or code size.
----------------
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."
================
Comment at: docs/TestSuiteGuide.rst:100
+ Contains test programs that are only a single source file in size. A
+ subdirectory may contain several such programs.
+
----------------
"such" isn't necessary
================
Comment at: docs/TestSuiteGuide.rst:117-118
+ distributed with the test-suite. The most prominent members of this
+ directory are the SPEC CPU benchmark suites. The user can supply the sources
+ via other means.
+
----------------
Which other means?
================
Comment at: docs/TestSuiteGuide.rst:132
+
+Every program can work as a correctness test. However some of the programs are
+unsuitable for performance measurements. Enabling the
----------------
some of the programs -> some programs
================
Comment at: docs/TestSuiteGuide.rst:140
+
+The test-suite comes with a number of configuration options to customize
+building and running the benchmarks. CMake can print a list of available
----------------
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.
================
Comment at: docs/TestSuiteGuide.rst:157-158
+
+ Specify extra flags to be passed to C compiler invocations. Currently the
+ test-suite passes these flags to C++ compiler and linker invocations too.
+ See `<https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_FLAGS.html>`_
----------------
Second sentence can be simplified.
"These flags are also passed to C++ compiler and linker invocations."
================
Comment at: docs/TestSuiteGuide.rst:189
+
+ Semicolon separated list of directories to include. This can be used to
+ only build parts of the test-suite or to include external suites.
----------------
Semicolon separated -> Semicolon-separated
================
Comment at: docs/TestSuiteGuide.rst:191
+ only build parts of the test-suite or to include external suites.
+ Note that using this option does not work reliably with deeper
+ subdirectories as it skips intermediate ``CMakeLists.txt`` files which may
----------------
Can be simplified:
"Using this option does not work reliably with deeper subdirectories..."
(This could probably be split into two sentences, but meh.)
================
Comment at: docs/TestSuiteGuide.rst:197
+
+ Collect internal LLVM statistics: Appends ``-save-stats=obj`` to the
+ compiler command lines and makes the lit runner collect and merge the
----------------
Colon should be a period.
================
Comment at: docs/TestSuiteGuide.rst:209
+ Use the ``perf`` tool for time measurement instead of the ``timeit`` tool
+ coming with the suite. The ``perf`` is usually available on linux systems.
+
----------------
coming with -> that comes with
================
Comment at: docs/TestSuiteGuide.rst:226
+
+ Use a CMake cache. The test-suite comes with several cache files: They
+ group a set of configuration options to provide a convenient way to enable
----------------
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...
================
Comment at: docs/TestSuiteGuide.rst:227
+ Use a CMake cache. The test-suite comes with several cache files: They
+ group a set of configuration options to provide a convenient way to enable
+ common or tricky build configurations.
----------------
This can be simplified:
"Use a CMake cache. The test-suite comes with several CMake caches which predefine common or tricky build configurations."
================
Comment at: docs/TestSuiteGuide.rst:234
+
+The test-suite comes with a little script called ``compare.py`` to display and
+compare result files. You should invoke ``lit`` with the ``-o filename.json``
----------------
"little" is unnecessary
================
Comment at: docs/TestSuiteGuide.rst:235
+The test-suite comes with a little script called ``compare.py`` to display and
+compare result files. You should invoke ``lit`` with the ``-o filename.json``
+flag to produce a result file. Example usage:
----------------
"You should" is unnecessary.
"Invoke ``lit``..."
================
Comment at: docs/TestSuiteGuide.rst:293
+
+External suites such as SPEC can be enabled by either:
+
----------------
remove semicolon; they should only be used before a list when you have a full sentence
================
Comment at: docs/TestSuiteGuide.rst:298
+
+You can find further information like the expected versions in the respective
+README files such as ``test-suite/External/SPEC/README``.
----------------
Versions of what?
================
Comment at: docs/TestSuiteGuide.rst:309-310
+
+You can build custom suites using the test-suite infrastructure. They must have
+a ``CMakeLists.txt`` file at the top directory and will be picked up
+automatically if placed into a subdirectory of the test-suite or when setting
----------------
Meinersbur wrote:
> 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)
Can be simplified:
Custom tests must contain a ``CMakeLists.txt`` file at the top directory. The ``CMakeLists.txt`` file will be automatically picked up...
================
Comment at: docs/TestSuiteGuide.rst:322
+
+Profile guided optimization requires to compile and run the suite twice: First
+the benchmark should be compiled with profile generation instrumentation
----------------
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]
================
Comment at: docs/TestSuiteGuide.rst:326
+runner will merge the profile files using ``llvm-profdata`` so they can be used
+by the second compilation run: Example:
+
----------------
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.)
================
Comment at: docs/TestSuiteGuide.rst:344
+
+Note: The ``TEST_SUITE_RUN_TYPE`` setting only affects the SPEC benchmark suite.
+
----------------
Meinersbur wrote:
> ... SPEC benchmark suite**s**
Should this be
```
.. NOTE::
```
like on line 402?
================
Comment at: docs/TestSuiteGuide.rst:360
+
+Cross-compilation from macOS to iOS is possible with the
+``test-suite/cmake/caches/target-target-*-iphoneos-internal.cmake`` CMake cache
----------------
Should be consistent in hyphenation of cross-compilation. Above, this, there's no hyphen twice.
================
Comment at: docs/TestSuiteGuide.rst:362
+``test-suite/cmake/caches/target-target-*-iphoneos-internal.cmake`` CMake cache
+files; However this requires an internal iOS SDK.
+
----------------
don't need to capitalize "however"
================
Comment at: docs/TestSuiteGuide.rst:367
+
+There are currently two ways to run the tests in a cross compilation setting:
+
----------------
Don't need "currently".
================
Comment at: docs/TestSuiteGuide.rst:369
+
+- You can use an ssh connection to the external device: The
+ ``TEST_SUITE_REMOTE_HOST`` option should be set to the ssh host name. After
----------------
Capitalize SSH.
Can be simplified:
"Via SSH connection to an external device."
================
Comment at: docs/TestSuiteGuide.rst:370
+- You can use an ssh connection to the external device: The
+ ``TEST_SUITE_REMOTE_HOST`` option should be set to the ssh host name. After
+ compilation the executables and data files need to be transferred to the
----------------
ssh->SSH
host name -> hostname
================
Comment at: docs/TestSuiteGuide.rst:372-374
+ device. This is typically done via the ``rsync`` make target. The lit runner
+ can then be used on the host machine and will prefix the benchmark and
+ verification command lines with a ``ssh`` command. Example:
----------------
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.
================
Comment at: docs/TestSuiteGuide.rst:391
+
+Running the test suite via LNT
+==============================
----------------
Hmmmm, capitalization, if we're not committed to "test-suite":
Running the Test Suite via LNT
Otherwise:
Running the test-suite via LNT
================
Comment at: docs/TestSuiteGuide.rst:394-395
+
+The LNT tool comes with a mode to run the test-suite. This should be used
+when submitting test results to an LNT database.
+
----------------
simplification:
"The LNT tool can run the test-suite. Use this when submitting results to a LNT database."
================
Comment at: docs/TestSuiteGuide.rst:399
+
+Running the test suite via Makefiles (deprecated)
+=================================================
----------------
Same as the other capitalization confusion. If it's "test-suite", then it's fine since that's the name...
================
Comment at: docs/TestSuiteGuide.rst:407
+
+The old documentation can be found in the :doc:`TestSuiteMakefileGuide`.
----------------
Simplification:
Old documentation is available in the :doc:`TestSuiteMakefileGuide`.
================
Comment at: docs/TestSuiteMakefileGuide.rst:92
Running different tests
+=======================
----------------
Running Different Tests
================
Comment at: docs/TestSuiteMakefileGuide.rst:112
Generating test output
+======================
----------------
Generating Test Output
================
Comment at: docs/TestSuiteMakefileGuide.rst:138
Writing custom tests for the test suite
+=======================================
----------------
Writing Custom Tests for the Test Suite
or
Writing Custom Tests for the test-suite
================
Comment at: docs/TestingGuide.rst:29
-If you intend to run the :ref:`test-suite <test-suite-overview>`, you will also
-need a development version of zlib (zlib1g-dev is known to work on several Linux
-distributions).
-
LLVM testing infrastructure organization
========================================
----------------
LLVM Testing Infrastructure Organization
================
Comment at: docs/TestingGuide.rst:98-99
-The more comprehensive test suite that includes whole programs in C and C++
-is in the ``test-suite`` module. See :ref:`test-suite Quickstart
-<test-suite-quickstart>` for more information on running these tests.
+The more comprehensive test suite that includes whole programs in C and C++ is
+in the ``test-suite`` module. See :doc:`TestSuiteGuide` for details.
----------------
Simplification:
The ``test-suite`` module contains a more comprehensive test suite including whole C and C++ programs.
Repository:
rL LLVM
https://reviews.llvm.org/D51465
More information about the llvm-commits
mailing list