This is an archive of the discontinued LLVM Phabricator instance.

Mark a number of x86 only tests to require x86
ClosedPublic

Authored by JonChesterfield on Jun 27 2017, 7:01 AM.

Details

Summary

Noticed while testing for an out of tree target. There are probably more tests that should be so marked.
I'm not sure who owns these tests so I've added a few names I recognise from the recent history.

Event Timeline

probinson edited edge metadata.Jun 27 2017, 7:32 AM
probinson added a subscriber: ruiu.

According to the top-level CODE_OWNERS.TXT the overall LLD code owner is @ruiu so please add him as a reviewer.
Also all LLD patches should have llvm-commits as a subscriber.
Note that when you add the list as a subscriber after-the-fact, you should repeat the original patch description as a comment, otherwise it won't appear on the mailing list.

JonChesterfield added a subscriber: llvm-commits.

Thanks Paul. This is my first time looking at lld, the guidance is appreciated.

Mark a number of x86 only tests to require x86

Noticed while testing for an out of tree target. There are probably more tests that should be so marked.
I'm not sure who owns these tests so I've added a few names I recognise from the recent history.

davidb edited edge metadata.Jun 29 2017, 10:20 AM

A few more tests have been introduced like this. It may make more sense to restructure the tests so that they are separated by target.

ruiu edited edge metadata.Jun 29 2017, 10:38 AM

We are using x86 as a "generic" target to test features that don't depend on targets. Besides that, the number of tests that need target-specific treatment is not that many, so I don't think we want to restructure the test files per target. (Also, the fact that we are using "x86" to test generic feature means that you usually want to enable the LLVM x86 target to run these tests.)

In D34685#795468, @ruiu wrote:

(Also, the fact that we are using "x86" to test generic feature means that you usually want to enable the LLVM x86 target to run these tests.)

Are you saying, if you want to test LLD, you must enable the x86 target? That sounds backwards.

ruiu added a comment.Jun 29 2017, 10:57 AM

Yes, that's what I meant. It's not ideal, but if you want to write tests that don't depend on target, what target would you choose?

grimar added a subscriber: grimar.Jun 30 2017, 1:10 AM
In D34685#795512, @ruiu wrote:

Yes, that's what I meant. It's not ideal, but if you want to write tests that don't depend on target, what target would you choose?

LLVM also has lots of tests that are generic in principle, but end up depending on a particular target for practical reasons. However, the project does not require people to include the X86 target (or ARM target, another favorite for "generic" tests) before they can run tests at all. The project relies on the many people who *do* have those targets enabled, plus bots, to guarantee coverage. And, the ought-to-be-generic tests end up in target-specific directories, where they don't get in the way of people whose work either doesn't depend on that target or requires that they not have that target enabled. As one recent example, r295384 added a "generic" test that had to be moved to an X86 specific directory in r295389, for exactly this reason.

The people who are proposing this change do not, for whatever reason, want to include the x86 target. I don't think the LLD project should force them to include it. I know I don't personally contribute meaningfully to the LLD project, but others in my company do, and as a designated merge guru I need to keep them happy.

ruiu added a comment.Jun 30 2017, 7:45 AM

I don't know if I understand what you said correctly. I do not want to force anyone to enable the x86 target. If you disable x86, tests that use x86 as a "generic" target won't run, but it shouldn't be more than that. I'm still waiting for you to update this patch to address Rafael's comment.

In D34685#796778, @ruiu wrote:

I don't know if I understand what you said correctly. I do not want to force anyone to enable the x86 target. If you disable x86, tests that use x86 as a "generic" target won't run, but it shouldn't be more than that. I'm still waiting for you to update this patch to address Rafael's comment.

I'll update the patch. But I'm still concerned that needing to add "REQUIRES: x86" is going to be an ongoing maintenance for us as new tests are added.

Also as it currently stands, I can't see an easy way to distinguish between generic tests and actual x86 tests.

ruiu added a comment.Jun 30 2017, 9:00 AM

I'll update the patch. But I'm still concerned that needing to add "REQUIRES: x86" is going to be an ongoing maintenance for us as new tests are added.

We should start adding "REQUIRES: x86" to new tests from now on.

Also as it currently stands, I can't see an easy way to distinguish between generic tests and actual x86 tests.

You can distinguish by filename, as target-specific tests start with target name.

In D34685#796778, @ruiu wrote:

I'm still waiting for you to update this patch to address Rafael's comment.

I don't see any comment by Rafael. What are you referring to?

In D34685#796846, @ruiu wrote:

You can distinguish by filename, as target-specific tests start with target name.

Does this mean the patch for files in the ELF directory which do not start with a target name should not specify a triple?
For example, relocatable-tls.s and no-symtab.s should pass without the REQUIRES clause were the explicit triple removed.

In D34685#796778, @ruiu wrote:

I'm still waiting for you to update this patch to address Rafael's comment.

I don't see any comment by Rafael. What are you referring to?

Rafael replied via email, not on phabricator, and email replies don't always propagate. He said:

davidb added a comment.

In https://reviews.llvm.org/D34685#796778, @ruiu wrote:

I don't know if I understand what you said correctly. I do not want to force anyone to enable the x86 target. If you disable x86, tests that use x86 as a "generic" target won't run, but it shouldn't be more than that. I'm still waiting for you to update this patch to address Rafael's comment.

I'll update the patch. But I'm still concerned that needing to add "REQUIRES: x86" is going to be an ongoing maintenance for us as new tests are added.

The normal way of avoiding that is setting up a bot. Can you setup one
that tests lld without the x86 backend?

Cheers,
Rafael

ruiu added a comment.Jul 11 2017, 3:09 PM

Again, can you please update as per the Rafael's comment?

Does this mean the patch for files in the ELF directory which do not start with a target name should not specify a triple?
For example, relocatable-tls.s and no-symtab.s should pass without the REQUIRES clause were the explicit triple removed.

Here is the rule that I'm proposing.

  • If a test is testing an architecture-specific feature, add the architecture name as a prefix. For example, if a test tests x86 relocation relaxation, name it x86-relocation-relaxation.s.
  • If a test is testing an architecture-independent feature, use the x86 as a "generic" target, and don't add "x86" as a prefix.

Sorry, Jon and I have been out of office the past week.

I can't seem to update the diff again without creating a new revision? The update existing revision doesn't seem available to me...

ruiu added a comment.Jul 17 2017, 10:57 AM

Make sure you have logged in to reviews.llvm.org. Looks like even I can upload a new revision to this review request, so I think anyone who have logged in can do that.

I can't seem to do it through the Phabricator UI, or archanist which has just created a new differential... https://reviews.llvm.org/D35565

davidb updated this revision to Diff 107115.Jul 18 2017, 9:11 AM

Updating D34685: Mark a number of x86 only tests to require x86

ruiu accepted this revision.Jul 18 2017, 9:15 AM

LGTM

test/ELF/relocatable-compressed-input.s
2–3

REQUIRES: x86, zlib

This revision is now accepted and ready to land.Jul 18 2017, 9:15 AM
In D34685#813125, @ruiu wrote:

LGTM

Great. Thank you Dave for picking this up in my absence

This revision was automatically updated to reflect the committed changes.