[lit] Add `target=<triple>` as a feature keyword

[probinson]

Summary

As proposed first in D107162 and later in discourse at
https://discourse.llvm.org/t/rfc-lits-requires-and-triples/66041

Modified a couple of lit's own tests to use this; left others as-is,
because for now triple substrings still work in UNSUPPORTED/XFAIL.

[dblaikie]

This is to detect the current/default target?

Should we instead support something more like the one in cross-project-tests, that adds a feature for every registered target? (that way a test can declare that it runs any time x86-registered-target is available, and it can specify the x86 target, so the test would run even on arm-default builds, for instance, so long as they also included x86?)

[probinson]

In D137434#3913083, @dblaikie wrote:

This is to detect the current/default target?

This is to allow checking the default target in a more principled way. Currently UNSUPPORTED/XFAIL (but not REQUIRES) can use arbitrary substrings of the default target triple as "feature keywords" and the end goal here is to replace all those with regexes against target=<triple> (thus allowing REQUIRES to check the target triple as well).

Should we instead support something more like the one in cross-project-tests, that adds a feature for every registered target? (that way a test can declare that it runs any time x86-registered-target is available, and it can specify the x86 target, so the test would run even on arm-default builds, for instance, so long as they also included x86?)

That's not what I see cross-project-tests doing; it's checking whether any of the registered targets match the _host_ because it wants to be able to build/test executable programs running on the host.

[ldionne]

This LGTM, but I think the commit should like to the Discourse thread that this was proposed in.

[dblaikie]

In D137434#3913191, @probinson wrote:

In D137434#3913083, @dblaikie wrote:

This is to detect the current/default target?

This is to allow checking the default target in a more principled way. Currently UNSUPPORTED/XFAIL (but not REQUIRES) can use arbitrary substrings of the default target triple as "feature keywords" and the end goal here is to replace all those with regexes against target=<triple> (thus allowing REQUIRES to check the target triple as well).

Should we instead support something more like the one in cross-project-tests, that adds a feature for every registered target? (that way a test can declare that it runs any time x86-registered-target is available, and it can specify the x86 target, so the test would run even on arm-default builds, for instance, so long as they also included x86?)

That's not what I see cross-project-tests doing; it's checking whether any of the registered targets match the _host_ because it wants to be able to build/test executable programs running on the host.

Not sure I follow - this code ( https://github.com/llvm/llvm-project/blob/e044796132abd9e10e21093822dc234003628fbd/cross-project-tests/lit.cfg.py#L256-L258 ) appears to be adding a <arch>-registered-target for every architecture that's enabled, not related to whether they match the host triple. Oh, and that's already in clang and used a lot there, so maybe porting that feature over to LLVM?

But registered-target is for all the registered targets (& only the architecture, not the OS, etc, in the triple) - what's the particular need for the default target (unless it's native - which is useful for executing the code).

I guess it means a test can be written generically rather than REQUIRES target + hardcoded target on the test execution, etc?

If there's a discourse thread as @ldionne mentioned, yeah, be good to have a link for context, etc.

[probinson]

In D137434#3917880, @dblaikie wrote:

In D137434#3913191, @probinson wrote:

In D137434#3913083, @dblaikie wrote:

Should we instead support something more like the one in cross-project-tests, that adds a feature for every registered target? (that way a test can declare that it runs any time x86-registered-target is available, and it can specify the x86 target, so the test would run even on arm-default builds, for instance, so long as they also included x86?)

That's not what I see cross-project-tests doing; it's checking whether any of the registered targets match the _host_ because it wants to be able to build/test executable programs running on the host.

Not sure I follow - this code ( https://github.com/llvm/llvm-project/blob/e044796132abd9e10e21093822dc234003628fbd/cross-project-tests/lit.cfg.py#L256-L258 ) appears to be adding a <arch>-registered-target for every architecture that's enabled, not related to whether they match the host triple. Oh, and that's already in clang and used a lot there, so maybe porting that feature over to LLVM?

But registered-target is for all the registered targets (& only the architecture, not the OS, etc, in the triple) - what's the particular need for the default target (unless it's native - which is useful for executing the code).

Assuming I captured the link correctly, what I was looking at is this:
https://github.com/llvm/llvm-project/blob/e044796132abd9e10e21093822dc234003628fbd/cross-project-tests/lit.cfg.py#L164

REQUIRES: native is probably a better choice for those cross-project-tests, it may be that the people who set up that directory were not aware of that feature keyword and rolled their own. I don't know. It wasn't my intent to fix things like that.

[probinson]

Patch description updated with the previous patch and discourse thread where this was discussed.

[dblaikie]

Fair enough - thanks for the link to context.

Any chance this could be put in one place/shared across all LLVM projects, rather than duplicated in subprojects (the thread mentions this is already in libcxx, for instance - be nice to put it in one place so everyone gets it? (does this patch make the libcxx feature redundant/could it be removed?))

[MaskRay]

Looks great!

[probinson]

In D137434#3918419, @dblaikie wrote:

Any chance this could be put in one place/shared across all LLVM projects, rather than duplicated in subprojects (the thread mentions this is already in libcxx, for instance - be nice to put it in one place so everyone gets it? (does this patch make the libcxx feature redundant/could it be removed?))

This patch does cause target=<triple> to be shared across all LLVM projects. The code in libcxx to define the same thing is harmlessly redundant and could be removed; I do plan to get to that eventually, if @ldionne doesn't beat me to it.

[dblaikie]

In D137434#3919854, @probinson wrote:

In D137434#3918419, @dblaikie wrote:

Any chance this could be put in one place/shared across all LLVM projects, rather than duplicated in subprojects (the thread mentions this is already in libcxx, for instance - be nice to put it in one place so everyone gets it? (does this patch make the libcxx feature redundant/could it be removed?))

This patch does cause target=<triple> to be shared across all LLVM projects. The code in libcxx to define the same thing is harmlessly redundant and could be removed; I do plan to get to that eventually, if @ldionne doesn't beat me to it.

Ah, nice!

[ldionne]

Is this blocked on anything?

[probinson]

In D137434#3928372, @ldionne wrote:

Is this blocked on anything?

Just me being swamped by other things. I hope to get this in today.

llvm/utils/lit/lit/llvm/config.py

def __init__(self, lit_config, config):
         elif platform.system() == 'SunOS':
             features.add('system-solaris')
 
         # Native compilation: host arch == default triple arch
         # Both of these values should probably be in every site config (e.g. as
         # part of the standard header.  But currently they aren't)
         host_triple = getattr(config, 'host_triple', None)
         target_triple = getattr(config, 'target_triple', None)
+        features.add('target=%s' % target_triple)
         if host_triple and host_triple == target_triple:
             features.add('native')
 
         # Sanitizers.
         sanitizers = getattr(config, 'llvm_use_sanitizer', '')
         sanitizers = frozenset(x.lower() for x in sanitizers.split(';'))
         if 'address' in sanitizers:
             features.add('asan')

llvm/utils/lit/tests/Inputs/shtest-format/lit.cfg

 import lit.formats
 config.name = 'shtest-format'
 config.suffixes = ['.txt']
 config.test_format = lit.formats.ShTest()
 config.test_source_root = None
 config.test_exec_root = None
 config.target_triple = 'x86_64-unknown-unknown'
+config.available_features.add('target=%s' % config.target_triple)
 config.available_features.add('a-present-feature')
 config.substitutions.append(('%{python}', '"%s"' % (sys.executable)))

llvm/utils/lit/tests/Inputs/shtest-format/requires-triple.txt

-# REQUIRES line that uses target triple, which doesn't work. Test should not run
-REQUIRES: x86_64
-RUN: false
+# REQUIRES line that uses target triple, which now works. Test should run
+REQUIRES: target=x86_64{{.*}}
+RUN: true

llvm/utils/lit/tests/Inputs/shtest-format/xfail-target.txt

 RUN: false
-XFAIL: x86_64
+XFAIL: target=x86_64{{.*}}

llvm/utils/lit/tests/shtest-format.py

 # CHECK-NEXT: line 1: failed test output on stdout
 # CHECK-NEXT: line 2: failed test output on stdout
 
 # CHECK: UNRESOLVED: shtest-format :: no-test-line.txt
 # CHECK: PASS: shtest-format :: pass.txt
 # CHECK: UNSUPPORTED: shtest-format :: requires-missing.txt
 # CHECK: PASS: shtest-format :: requires-present.txt
 # CHECK: UNRESOLVED: shtest-format :: requires-star.txt
-# CHECK: UNSUPPORTED: shtest-format :: requires-triple.txt
+# CHECK: PASS: shtest-format :: requires-triple.txt
 # CHECK: PASS: shtest-format :: unsupported-expr-false.txt
 # CHECK: UNSUPPORTED: shtest-format :: unsupported-expr-true.txt
 # CHECK: UNRESOLVED: shtest-format :: unsupported-star.txt
 # CHECK: UNSUPPORTED: shtest-format :: unsupported_dir/some-test.txt
 # CHECK: PASS: shtest-format :: xfail-expr-false.txt
 # CHECK: XFAIL: shtest-format :: xfail-expr-true.txt
 # CHECK: XFAIL: shtest-format :: xfail-feature.txt
 # CHECK: XFAIL: shtest-format :: xfail-target.txt

 # CHECK: shtest-format :: external_shell/fail_with_bad_encoding.txt
 # CHECK: shtest-format :: external_shell/fail_with_control_chars.txt
 # CHECK: shtest-format :: fail.txt
 
 # CHECK: Unexpectedly Passed Tests (1)
 # CHECK: shtest-format :: xpass.txt
 
 # CHECK: Testing Time:
-# CHECK: Unsupported        : 4
-# CHECK: Passed             : 6
+# CHECK: Unsupported        : 3
+# CHECK: Passed             : 7
 # CHECK: Expectedly Failed  : 4
 # CHECK: Unresolved         : 3
 # CHECK: Failed             : 4
 # CHECK: Unexpectedly Passed: 1
 
 
 # XUNIT: <?xml version="1.0" encoding="UTF-8"?>
 # XUNIT-NEXT: <testsuites time="{{[0-9.]+}}">
-# XUNIT-NEXT: <testsuite name="shtest-format" tests="22" failures="8" skipped="4">
+# XUNIT-NEXT: <testsuite name="shtest-format" tests="22" failures="8" skipped="3">
 
 # XUNIT: <testcase classname="shtest-format.external_shell" name="fail.txt" time="{{[0-9]+\.[0-9]+}}">
 # XUNIT-NEXT: <failure{{[ ]*}}>
 # XUNIT: </failure>
 # XUNIT-NEXT: </testcase>
 
 
 # XUNIT: <testcase classname="shtest-format.external_shell" name="fail_with_bad_encoding.txt" time="{{[0-9]+\.[0-9]+}}">

 
 # XUNIT: <testcase classname="shtest-format.shtest-format" name="requires-present.txt" time="{{[0-9]+\.[0-9]+}}"/>
 
 # XUNIT: <testcase classname="shtest-format.shtest-format" name="requires-star.txt" time="{{[0-9]+\.[0-9]+}}">
 # XUNIT-NEXT: <failure{{[ ]*}}>
 # XUNIT: </failure>
 # XUNIT-NEXT: </testcase>
 
-
-# XUNIT: <testcase classname="shtest-format.shtest-format" name="requires-triple.txt" time="{{[0-9]+\.[0-9]+}}">
-# XUNIT-NEXT:<skipped message="Missing required feature(s): x86_64"/>
+# XUNIT: <testcase classname="shtest-format.shtest-format" name="requires-triple.txt" time="{{[0-9]+\.[0-9]+}}"/>
 
 # XUNIT: <testcase classname="shtest-format.shtest-format" name="unsupported-expr-false.txt" time="{{[0-9]+\.[0-9]+}}"/>
 
 # XUNIT: <testcase classname="shtest-format.shtest-format" name="unsupported-expr-true.txt" time="{{[0-9]+\.[0-9]+}}">
 # XUNIT-NEXT:<skipped message="Unsupported configuration"/>
 
 # XUNIT: <testcase classname="shtest-format.shtest-format" name="unsupported-star.txt" time="{{[0-9]+\.[0-9]+}}">
 # XUNIT-NEXT: <failure{{[ ]*}}>