[PATCH] D12660: Make the default triple optional by allowing an empty string
Mehdi AMINI via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 9 22:10:40 PDT 2015
joker.eph marked 4 inline comments as done.
joker.eph added a comment.
In http://reviews.llvm.org/D12660#242029, @probinson wrote:
> 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.
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.
Comment at: test/Feature/optnone-llc.ll:7
@@ -6,3 +6,3 @@
-; REQUIRES: asserts, native
+; REQUIRES: asserts
> 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?
This was a mistake (I ran the tests with a release build and was covered by the "REQUIRES: asserts")
Comment at: test/lit.cfg:346
@@ -344,1 +345,3 @@
+# Native compilation: host arch == default triple arch
# FIXME: Consider cases that target can be executed
> 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).
What about renaming it then?
I can do that in a separate patch.
More information about the llvm-commits