This is an archive of the discontinued LLVM Phabricator instance.

[llvm] [lit] Add target-x86* features
ClosedPublic

Authored by mgorny on Apr 9 2019, 10:47 AM.

Details

Summary

Add a 'target-x86' and 'target-x86_64' feature sthat indicates that
the default target is 32-bit or 64-bit x86, appropriately. Combined
with 'native' feature, we're going to use this to control x86-specific
LLDB native process tests.

Diff Detail

Repository
rL LLVM

Event Timeline

mgorny created this revision.Apr 9 2019, 10:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2019, 10:47 AM
Herald added a subscriber: delcypher. · View Herald Transcript

When I requested this, I expected you would go and add something to the lldb lit config files. However, the way you've chosen to implement that seems to complement nicely the existing features in the llvm files, so I think it may go in here as well. But, I'd like for someone from the llvm side to sign off on this as well.

The one question I have is whether we really want to bundle x86 and x86_64 into one feature. It seems to me that if you were to extend the motivating test only slightly (e.g. to include %xmm8-15), then you'd need to differentiate between the 32 and 64 bit variants. OTOH, if these are separate features, then you can always say REQUIRES: x86 || x86_64 for tests that work on both.

phosek accepted this revision.Apr 9 2019, 11:30 PM

LGTM

This revision is now accepted and ready to land.Apr 9 2019, 11:30 PM

When I requested this, I expected you would go and add something to the lldb lit config files. However, the way you've chosen to implement that seems to complement nicely the existing features in the llvm files, so I think it may go in here as well. But, I'd like for someone from the llvm side to sign off on this as well.

The one question I have is whether we really want to bundle x86 and x86_64 into one feature. It seems to me that if you were to extend the motivating test only slightly (e.g. to include %xmm8-15), then you'd need to differentiate between the 32 and 64 bit variants. OTOH, if these are separate features, then you can always say REQUIRES: x86 || x86_64 for tests that work on both.

Looks like we raced with @labath, I agree with his response that you can also use REQUIRES: x86 || x86_64 to achieve the same, so the question is how many of these cases you're going to have in lldb, if it's just handful then it's probably not worth introducing a new feature, but if it's going to be a significant number then a new feature is reasonable.

craig.topper added inline comments.
llvm/utils/lit/lit/llvm/config.py
100 ↗(On Diff #194364)

Don't 32-bit x86 triples use i386 or i686 rather than "x86"?

mgorny marked 2 inline comments as done.Apr 10 2019, 1:08 AM
mgorny added inline comments.
llvm/utils/lit/lit/llvm/config.py
100 ↗(On Diff #194364)

Yes, you are correct. I don't know what I was thinking yesterday but I'll also split it into x86/x86_64 as well whle at it..

mgorny updated this revision to Diff 194456.Apr 10 2019, 1:12 AM
mgorny marked an inline comment as done.
mgorny retitled this revision from [llvm] [lit] Add target-x86 feature to [llvm] [lit] Add target-x86* features.
mgorny edited the summary of this revision. (Show Details)
mgorny added a reviewer: craig.topper.
labath accepted this revision.Apr 11 2019, 7:46 AM
This revision was automatically updated to reflect the committed changes.