[docs] Add Included Suites to Test Suite Guide #128937
Conversation
This policy has never been written down, but was the intention when the GCC C Torture Suite was added. In the years since that suite was added, changes to these tests have been landed, but we want to reject (or discourage) this going forwards, and look into reverting those changes.
|
Do we think this needs signalling in the LLVM Discourse? |
Yes. This part is likely uncontroversial but it's a good time to find out if we want to apply the same to the applications in llvm-test-suite. |
llvm/docs/TestSuiteGuide.md
Outdated
| to hook into the LLVM Test Suite system. For example, these include GCC's C | ||
| and Fortran Torture test suites. |
There was a problem hiding this comment.
NIT: Slight change in wording to be a bit more accurate: we include all the gfortran tests, not just the torture tests.
| to hook into the LLVM Test Suite system. For example, these include GCC's C | |
| and Fortran Torture test suites. | |
| to hook into the LLVM Test Suite system. These include GCC's C Torture tests and gfortran's test suite. |
llvm/docs/TestSuiteGuide.md
Outdated
|
|
||
| The only changes we can accept to the tests in these suites are where we bring | ||
| in all the changes from the external sources. Users should not make individual | ||
| downstream changes to these tests. |
There was a problem hiding this comment.
I think "users" and "downstream" might be a bit ambiguous in this context. Unless I'm getting the wrong end of the stick, I think you're referring to LLVM contributors making changes and attempting to submit them to the llvm-test-suite repo (downstream from the original tests/benchmarks, but "upstream" from many people's point of view!). I assume we're not trying to pass judgement on whatever atrocities people apply to an llvm-test-suite checkout on their own machine for their downstream only purposes(!).
I have a more general comment about the policy I'll leave on the Discourse thread.
There was a problem hiding this comment.
You're right that this is ambiguous. I think the consensus in Discourse is that this needs significant rewriting anyway, but I will make sure I am clearer about what I'm referring to.
There was a problem hiding this comment.
I'm still somewhat unhappy with the guidance given here. I think that, apart from the notable exceptions mentioned (GCC torture test suite, GFortran test suite), the existing and recommended practice is to modify the vendored sources, not to try to make an upstream change and sync it back into llvm-test-suite.
My understanding is that close to all benchmarks in llvm-test-suite are basically fixed in time -- we vendor a specific version, and then never update it again. This means that we have a fixed evaluation target. For example, if someone went and updated our close to 20 year old sqlite3 amalgamation to fix a compilation issue, I'd have a problem, because my compile-time measurements would lose continuity.
Now, I think we can consider doing such updates with some care -- after all, testing twenty year old code makes llvm-test-suite not particularly representative. But this should not be a go-to solution to fix an incompatibility with modern clang. (And possibly needs some additional management, e.g. what the rust perf tests do is to explicitly version the benchmarks, and keep some around for long-term comparisons, while most get updated every three years.)
So I think what these docs should be saying, in order to document existing practice, is that the sources of the GCC torture suite and GFortran tests should not be modified, and for everything else we should be performing either cmake or source modifications, whichever makes more sense.
|
@nikic understood about the problems with updating benchmarks. I'm not sure this necessarily applies to tests, so do you think that it's at all worth splitting the guidance between tests and benchmarks, or is it the case that basically everything in the llvm-test-suite repo is a test that we also record benchmark scores for? |
This policy is derived from the discussion here: https://discourse.llvm.org/t/test-suite-clarify-included-test-suites-policy/84859