This is an archive of the discontinued LLVM Phabricator instance.

Differential D22172

Added REQUIRES to TestingGuide documentation
ClosedPublic

Authored by Prazek on Jul 8 2016, 2:28 PM.

Details

Reviewers
rengolin
alexfh
probinson
wolfgangp
Commits
rG7a298c1df0e4: Added REQUIRES to TestingGuide documentation
rL274949: Added REQUIRES to TestingGuide documentation

Diff Detail

Event Timeline

Prazek updated this revision to Diff 63318.Jul 8 2016, 2:28 PM
Prazek retitled this revision from to Added REQUIRES to FileCheck documentation.
Prazek updated this object.
Prazek added reviewers: alexfh, wolfgangp, rengolin.
Prazek added a subscriber: llvm-commits.
Prazek added a subscriber: tejohnson.Jul 8 2016, 2:31 PM

I have found the features list that you were looking for :D

probinson added a subscriber: probinson.Jul 8 2016, 2:57 PM

REQUIRES is a 'lit' feature, not a FileCheck feature.
A better place to describe it would be docs/TestingGuide.rst, which already talks about RUN and XFAIL.

Prazek updated this revision to Diff 63328.Jul 8 2016, 3:11 PM

review fix

Prazek updated this revision to Diff 63329.Jul 8 2016, 3:13 PM

changed commit name

Prazek retitled this revision from Added REQUIRES to FileCheck documentation to Added REQUIRES to TestingGuide documentation.Jul 8 2016, 3:14 PM
probinson added a comment.Jul 8 2016, 3:27 PM

I think you can also use triple components in REQUIRES, which makes me wonder about 'x86_64-linux'. Is that something lit supports separately from the triple check?

docs/TestingGuide.rst
399

I think you need a colon after REQUIRES.

401

coma -> comma

402

I would rather see the two forms contrasted more directly, maybe like this:

REQUIRES means all listed requirements must be satisfied; REQUIRES-ANY means at least one must be satisfied.

429

Please sort the list alphabetically. If you prefer to keep the "not" versions with the positive versions, that's fine.

Just curious, what is 'xar'?

Prazek marked 4 inline comments as done.Jul 8 2016, 3:40 PM
Prazek added inline comments.
docs/TestingGuide.rst
429

no idea, but it comes from test/lit.cfg

if config.have_libxar:

config.available_features.add('xar')

so I guess libxar something

Prazek updated this revision to Diff 63338.Jul 8 2016, 3:47 PM
Prazek marked an inline comment as done.

post review

probinson accepted this revision.Jul 8 2016, 3:54 PM
probinson added a reviewer: probinson.

LGTM

This revision is now accepted and ready to land.Jul 8 2016, 3:54 PM
mehdi_amini added a subscriber: mehdi_amini.Jul 8 2016, 4:05 PM

I'd mention where this list is coming from, or how to regenerate it. I fear this is going to be out-of-sync to quickly.

Prazek updated this revision to Diff 63353.Jul 8 2016, 4:16 PM
Prazek marked an inline comment as done.
Prazek edited edge metadata.

LAst review

Closed by commit rL274949: Added REQUIRES to TestingGuide documentation (authored by Prazek). · Explain WhyJul 8 2016, 4:54 PM
This revision was automatically updated to reflect the committed changes.
MatzeB added a subscriber: MatzeB.Jul 8 2016, 5:16 PM
MatzeB added inline comments.
llvm/trunk/docs/TestingGuide.rst
406–434 ↗(On Diff #63357)

Should we really list all of them here? I see this list getting out of date quickly as it is not obvious to people changing lit.cfg that this list exists here. I'd recommend to only refer to lit.cfg and maybe mention 1 or 2 examples...

mehdi_amini added inline comments.Jul 8 2016, 5:18 PM
llvm/trunk/docs/TestingGuide.rst
406–434 ↗(On Diff #63357)

Agree.

Prazek added inline comments.Jul 8 2016, 5:29 PM
llvm/trunk/docs/TestingGuide.rst
406–434 ↗(On Diff #63357)

I think having list somewhere is usefull. It wasn't obvious for me where I can find the code that defines the features - and all features are splitted between different files. The argument that it get outdated fast ins invalid - just keep track of it in reviews (I am willing to be added as a subscriber to every patch that touches any lit.cfg to check it)
or find a way to generate it from code.

The list is useful. e.g. I can see that the list doesn't mention anything similar to LLVM_ENABLE_STATS that I was looking for, so I know that I should probably go different way.

mehdi_amini added inline comments.Jul 8 2016, 5:37 PM
llvm/trunk/docs/TestingGuide.rst
406–434 ↗(On Diff #63357)

The argument that it get outdated fast ins invalid - just keep track of it in reviews (I am willing to be added as a subscriber to every patch that touches any lit.cfg to check it)

Sorry but you can't just say that the argument "is invalid". Experience shows it is.

or find a way to generate it from code.

Sure, in the meantime I think we should remove it.

probinson added a comment.Jul 10 2016, 3:43 PM

A list is definitely useful. It's a reasonable point that the list evolves over time and a manually maintained list is a headache.
Maybe add a command-line option to 'lit' that will dump the REQUIRES features. Then document that option in TestingGuide.rst, instead of having the list.

Prazek added a comment.Jul 11 2016, 4:31 PM

ok, I guess you are right.

here are the fixes.
http://reviews.llvm.org/D22245

Revision Contents

PathSize
docs/
40 lines

Diff 63328

docs/TestingGuide.rst

Show First 20 Lines • Show All 381 Lines • ▼ Show 20 Lines.. code-block:: llvm
; AVX2: @test1 ; AVX2: @test1
; AVX2: vpsubusw LCPI0_0(%rip), %xmm0, %xmm0 ; AVX2: vpsubusw LCPI0_0(%rip), %xmm0, %xmm0
So, if you're testing for a behaviour that you know is platform-specific or So, if you're testing for a behaviour that you know is platform-specific or
depends on special features of sub-architectures, you must add the specific depends on special features of sub-architectures, you must add the specific
triple, test with the specific FileCheck and put it into the specific triple, test with the specific FileCheck and put it into the specific
directory that will filter out all other architectures. directory that will filter out all other architectures.
REQUIRES and REQUIRES-ANY directive
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Some tests can be enabled only in specific situation - like having
debug build. Use ``REQUIRES`` directive to specify those requirements.
.. code-block:: llvm
; This test will be only enabled in the build with asserts
; REQUIRES asserts
probinsonUnsubmitted
Done
ReplyInline Actions

I think you need a colon after REQUIRES.

probinson: I think you need a colon after REQUIRES.
You can separate requirements by a coma.
probinsonUnsubmitted
Done
ReplyInline Actions

coma -> comma

probinson: coma -> comma
If test depends on one of many requirements, use ``REQUIRES-ANY``.
probinsonUnsubmitted
Done
ReplyInline Actions

I would rather see the two forms contrasted more directly, maybe like this:

REQUIRES means all listed requirements must be satisfied; REQUIRES-ANY means at least one must be satisfied.

probinson: I would rather see the two forms contrasted more directly, maybe like this: REQUIRES means all…
List of features that can be used in ``REQUIRES`` and ``REQUIRES-ANY``:
- asserts
- shell
- can-execute
- loadable_module
- asan
- not_asan
- msan
- not_msan
- ubsan
- not_ubsan
- long_tests
- object-emission
- zlib
- nozlib
- default_triple
- x86_64-linux
- native
- ld_emu_elf32ppc
- ld_plugin
- ld64_plugin
- fma3
- system-windows
- debug_frame
- global-isel
- xar
probinsonUnsubmitted
Done
ReplyInline Actions

Please sort the list alphabetically. If you prefer to keep the "not" versions with the positive versions, that's fine.

Just curious, what is 'xar'?

probinson: Please sort the list alphabetically. If you prefer to keep the "not" versions with the…
PrazekAuthorUnsubmitted
Done
ReplyInline Actions

no idea, but it comes from test/lit.cfg

if config.have_libxar:

config.available_features.add('xar')

so I guess libxar something

Prazek: no idea, but it comes from test/lit.cfg if config.have_libxar: config.available_features.
Substitutions Substitutions
------------- -------------
Besides replacing LLVM tool names the following substitutions are performed in Besides replacing LLVM tool names the following substitutions are performed in
RUN lines: RUN lines:
``%%`` ``%%``
▲ Show 20 Lines • Show All 205 Lines • Show Last 20 Lines