[PATCH] D12660: Make the default triple optional by allowing an empty string

[Mehdi AMINI via llvm-commits]

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?


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.



================
Comment at: test/Feature/optnone-llc.ll:7
@@ -6,3 +6,3 @@
 
-; REQUIRES: asserts, native
+; REQUIRES: asserts
 
----------------
probinson wrote:
> 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
----------------
probinson wrote:
> 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.


http://reviews.llvm.org/D12660