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

[Paul Robinson via llvm-commits]

probinson added a comment.

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.


================
Comment at: include/llvm/Config/config.h.cmake:424
@@ -423,3 +423,3 @@
 /* Target triple LLVM will generate code for by default */
-#cmakedefine LLVM_DEFAULT_TARGET_TRIPLE "${LLVM_DEFAULT_TARGET_TRIPLE}"
+#define LLVM_DEFAULT_TARGET_TRIPLE "${LLVM_DEFAULT_TARGET_TRIPLE}"
 
----------------
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.

================
Comment at: test/DebugInfo/lit.local.cfg:4
@@ +3,1 @@
+    config.unsupported = True
\ No newline at end of file

----------------
Please fix the missing newline. Also there's an empty first line?


================
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?


================
Comment at: test/lit.cfg:343
@@ -342,2 +342,3 @@
 
-# Native compilation: host arch == target arch and native backend built-in
+if config.target_triple:
+    config.available_features.add("default_triple")
----------------
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.

================
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).


http://reviews.llvm.org/D12660