This is an archive of the discontinued LLVM Phabricator instance.

Make the default triple optional by allowing an empty string
ClosedPublic

Authored by mehdi_amini on Sep 4 2015, 11:04 PM.

Details

Summary

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".

Diff Detail

Event Timeline

mehdi_amini retitled this revision from to Make the default triple optional by allowing an empty string.
mehdi_amini updated this object.
mehdi_amini added a reviewer: probinson.
mehdi_amini added a subscriber: llvm-commits.

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.

probinson edited edge metadata.Sep 8 2015, 4:15 PM

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

mehdi_amini marked 4 inline comments as done.Sep 9 2015, 10:10 PM

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.

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 can do that in a separate patch.

mehdi_amini marked an inline comment as done.
mehdi_amini edited edge metadata.

Updated taking comments into account.

In D12660#243011, @joker.eph 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/

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!

add missing lit.local.cfg

+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.

echristo accepted this revision.Sep 15 2015, 12:05 PM
echristo edited edge metadata.

I'll trust that you're all going in a good direction here. Thanks for checking in!

-eric

This revision is now accepted and ready to land.Sep 15 2015, 12:05 PM
mehdi_amini closed this revision.Sep 15 2015, 10:41 PM

Thanks, r247775.