When building LLVM as a (potentially dynamic) library that can be linked against
by multiple compilers, the default triple is not really meaningful.
We allow to explicitely set it to an empty string when configuring LLVM.
In this case, said "target independent" tests in the test suite that are using
the default triple are disabled by matching the newly available feature
"default_triple".
Details
Diff Detail
Event Timeline
I like this idea, but for a different reason than you're proposing it: it shows all of the tests which are depending on a notion of a "default triple", but *ought* to be fixed to run on all targets, instead.
James, I have another experimental patch that given a list of triple run multiple times these tests with a different triple every time. The implementation is a hack, I need to find a way to clean it.
I also like this tactic of permitting an empty default triple. Definitely an improvement over a silly default triple!
Does llvm-mc always need a triple? That is, would it be better to add test/tools/llvm-mc/lit.local.cfg (like you did for test/DebugInfo) instead of adding REQUIRES to each test/tools/llvm-mc/* file?
It seems weird for target-specific tests to require a default triple, but I guess those are the kinds of tests where you and jyknight are talking about iterating over some list of triples, and that seems like a good future direction.
Have you thought about moving test/DebugInfo/*.ll to a Generic subdirectory? Not saying you should or should not, but it seems possibly sub-optimal to require a default triple for *all* DebugInfo tests, even the target-specific ones. But maybe it's okay the way it is; I haven't done any particular research or thought about it much.
See also inline comments.
include/llvm/Config/config.h.cmake | ||
---|---|---|
424 | Because this is different from all the other parameters, there should be a comment so nobody will think it's a typo and try to "fix" it. | |
test/DebugInfo/lit.local.cfg | ||
4 | Please fix the missing newline. Also there's an empty first line? | |
test/Feature/optnone-llc.ll | ||
7 | I have generally started associating "test uses llc" with "REQUIRES: default_triple" but here we have a test that doesn't? It seems like the configuration you use (host=X86, default-triple empty, only-target=GPU) should fail here. That is, I'd expect llc to fall back on the host (X86-ish) triple and therefore fail. Doesn't it? | |
test/lit.cfg | ||
343 | It would be good to have a comment briefly stating why we have a default_triple feature. I'm not aware of any other place where these strings are documented. | |
346 | Given that we both misunderstood the purpose of this feature, it would be good to improve the commentary a little ("needed for JIT" or something like that). |
Created test/tools/llvm-mc/Generic/
It seems weird for target-specific tests to require a default triple, but I guess those are the kinds of tests where you and jyknight are talking about iterating over some list of triples, and that seems like a good future direction.
Do you mean the ones in test/CodeGen/PowerPC, test/CodeGen/X86, etc.?
They really either shouldn't be in these directory, or should specify a triple. This is a bit messy but I wasn't sure which triple to define for these.
Have you thought about moving test/DebugInfo/*.ll to a Generic subdirectory? Not saying you should or should not, but it seems possibly sub-optimal to require a default triple for *all* DebugInfo tests, even the target-specific ones. But maybe it's okay the way it is; I haven't done any particular research or thought about it much.
See also inline comments.
test/Feature/optnone-llc.ll | ||
---|---|---|
7 | This was a mistake (I ran the tests with a release build and was covered by the "REQUIRES: asserts") | |
test/lit.cfg | ||
346 | What about renaming it then? |
I submitted unintentionally. I added a lit.local.cfg.
It seems weird for target-specific tests to require a default triple, but I guess those are the kinds of tests where you and jyknight are talking about iterating over some list of triples, and that seems like a good future direction.
Do you mean the ones in test/CodeGen/PowerPC, test/CodeGen/X86, etc.?
They really either shouldn't be in these directory, or should specify a triple. This is a bit messy but I wasn't sure which triple to define for these.Have you thought about moving test/DebugInfo/*.ll to a Generic subdirectory? Not saying you should or should not, but it seems possibly sub-optimal to require a default triple for *all* DebugInfo tests, even the target-specific ones. But maybe it's okay the way it is; I haven't done any particular research or thought about it much.
Moved as suggested.
Thanks for the comments!
+echristo as debug-info owner, to judge whether moving the top-level DebugInfo tests to a Generic subdirectory is okay.
Two minor comment suggestions, everything else LGTM.
test/lit.cfg | ||
---|---|---|
344 | Comments should end with a period. | |
346 | I think the name is descriptive, just an example of how it's used would help clarify things. |
I'll trust that you're all going in a good direction here. Thanks for checking in!
-eric
Because this is different from all the other parameters, there should be a comment so nobody will think it's a typo and try to "fix" it.