[PATCH] D51465: Revamp test-suite documentation

Thu Aug 30 14:16:32 PDT 2018

MatzeB added inline comments.

================
Comment at: docs/CMake.rst:606
+Executing the tests
+===================

----------------
rengolin wrote:
> 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.
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...)

================
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:

----------------
paquette wrote:
> "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."
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...

================
Comment at: docs/TestSuiteGuide.rst:12
+
+To run the test suite, you need the following:
+
----------------
paquette wrote:
> 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.
> 
> 
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).

================
Comment at: docs/TestSuiteGuide.rst:19-20
+
+        % svn co http://llvm.org/svn/llvm-project/llvm/trunk llvm
+        % sudo pip install llvm/utils/lit
+        # You can also install the tool manually if lit is not available:
----------------
tra wrote:
> kristof.beyls wrote:
> > 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.
> Is installing lit directly from llvm a hard requirement?
> I've been using `pip install lit` and that appears to work.
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 :)

================
Comment at: docs/TestSuiteGuide.rst:19-20
+
+        % svn co http://llvm.org/svn/llvm-project/llvm/trunk llvm
+        % sudo pip install llvm/utils/lit
+        # You can also install the tool manually if lit is not available:
----------------
MatzeB wrote:
> tra wrote:
> > kristof.beyls wrote:
> > > 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.
> > Is installing lit directly from llvm a hard requirement?
> > I've been using `pip install lit` and that appears to work.
> 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 :)
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.

================
Comment at: docs/TestSuiteGuide.rst:45
+
+#. Build the benchmarks:
+
----------------
paquette wrote:
> 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..."
As above, I hope it makes now sense as a step by step introduction.

================
Comment at: docs/TestSuiteGuide.rst:55
+        Scanning dependencies of target fpcmp-host
+        [  0%] [TEST_SUITE_HOST_CC] Building host executable fpcmp
+        [  0%] Built target fpcmp-host
----------------
Meinersbur wrote:
> There will be no more `TEST_SUITE_HOST_CC` after D51080.
See my comment there.

================
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
----------------
paquette wrote:
> "a number" isn't necessary.
> 
> The ``test-suite`` module contains programs that can be compiled and executed.
I took the recommendation and also dropped "module".

================
Comment at: docs/TestSuiteGuide.rst:113
+
+-  ``External/``
+
----------------
thegameg wrote:
> We could have a link to `External Suites` somewhere here.
added.

================
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.
+
----------------
paquette wrote:
> Which other means?
Dropped the sentence since we have a link to whole section now which explains things.

================
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
----------------
paquette wrote:
> 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.
I changed most of them to `test-suite` now.

================
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>`_
----------------
paquette wrote:
> Second sentence can be simplified.
> 
> "These flags are also passed to C++ compiler and linker invocations."
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.

================
Comment at: docs/TestSuiteGuide.rst:288
+        % test-suite/utils/compare.py base0.json base1.json base2.json vs exp0.json exp1.json exp2.json
+
+
----------------
kristof.beyls wrote:
> 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?
I added a section about LNT immediately below this.

================
Comment at: docs/TestSuiteGuide.rst:309-316
+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
+the ``TEST_SUITE_SUBDIRS`` variable:
+
+    .. code-block:: bash
+
----------------
paquette wrote:
> 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...
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.)

================
Comment at: docs/TestSuiteGuide.rst:344
+
+Note: The ``TEST_SUITE_RUN_TYPE`` setting only affects the SPEC benchmark suite.
+
----------------
paquette wrote:
> Meinersbur wrote:
> > ... SPEC benchmark suite**s**
> Should this be
> 
> ```
> .. NOTE::
> ```
> like on line 402?
I don't think this particular one needs the emphasis of a whole block. I removed the "Note:" now.

================
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.
+
----------------
paquette wrote:
> don't need to capitalize "however"
used lowercase and dropped "however".

================
Comment at: docs/TestSuiteGuide.rst:402-407
+.. NOTE::
+    The test-suite comes with a set of Makefiles that are considered deprecated.
+    They do not support newer testing modes like `Bitcode` or `Microbenchmarks`
+    and are harder to use.
+
+The old documentation can be found in the :doc:`TestSuiteMakefileGuide`.
----------------
kristof.beyls wrote:
> 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?
- 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 :)

Repository:
  rL LLVM

https://reviews.llvm.org/D51465