This is an archive of the discontinued LLVM Phabricator instance.

[lit] Set the target-windows feature for any windows environment
AbandonedPublic

Authored by mstorsjo on Sep 27 2019, 5:56 AM.

Details

Reviewers
rnk
Summary

Other common targets are windows-gnu (MinGW) and windows-itanium, and occasionally windows-elf is used as well.

Diff Detail

Event Timeline

mstorsjo created this revision.Sep 27 2019, 5:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2019, 5:56 AM
Herald added a subscriber: delcypher. · View Herald Transcript
rnk added inline comments.Oct 3 2019, 4:50 PM
llvm/utils/lit/lit/llvm/config.py
96

How about -windows-[^-]*$ to handle windows-elf and the oft forgotten windows-itanium?

mstorsjo updated this revision to Diff 223144.Oct 3 2019, 11:41 PM
mstorsjo retitled this revision from [lit] Set the target-windows feature for mingw triples as well to [lit] Set the target-windows feature for any windows environment.
mstorsjo edited the summary of this revision. (Show Details)

Updated, with a slightly different form of the regex, that also allows the triple to just end at -windows.

Alternatively, we could just remove this part of the lit config altogether. There's no other OSes that have target-<os> as a feature (only system-<os> for the host where the test is running), and after D68133 and D68136, no tests actually use this feature any longer.

Updated, with a slightly different form of the regex, that also allows the triple to just end at -windows.

Alternatively, we could just remove this part of the lit config altogether. There's no other OSes that have target-<os> as a feature (only system-<os> for the host where the test is running), and after D68133 and D68136, no tests actually use this feature any longer.

In most cases, target restrictions rely on using a component of the triple. If all Windows-target triples are guaranteed to spell the component 'windows' then REQUIRES: windows works, and we don't need target-windows at all. But I was under the impression that win32 was used in the triple sometimes?

Updated, with a slightly different form of the regex, that also allows the triple to just end at -windows.

Alternatively, we could just remove this part of the lit config altogether. There's no other OSes that have target-<os> as a feature (only system-<os> for the host where the test is running), and after D68133 and D68136, no tests actually use this feature any longer.

In most cases, target restrictions rely on using a component of the triple. If all Windows-target triples are guaranteed to spell the component 'windows' then REQUIRES: windows works, and we don't need target-windows at all. But I was under the impression that win32 was used in the triple sometimes?

At least at this point, it seems to use normalized triple names. For input to tools, win32 is a synonym to windows, and "mingw32" as OS name gets normalized to "windows-gnu".

rnk added a comment.Oct 4 2019, 10:47 AM

Updated, with a slightly different form of the regex, that also allows the triple to just end at -windows.

Alternatively, we could just remove this part of the lit config altogether. There's no other OSes that have target-<os> as a feature (only system-<os> for the host where the test is running), and after D68133 and D68136, no tests actually use this feature any longer.

In most cases, target restrictions rely on using a component of the triple. If all Windows-target triples are guaranteed to spell the component 'windows' then REQUIRES: windows works, and we don't need target-windows at all. But I was under the impression that win32 was used in the triple sometimes?

At least at this point, it seems to use normalized triple names. For input to tools, win32 is a synonym to windows, and "mingw32" as OS name gets normalized to "windows-gnu".

Right, D47381 / rL339307 replaced all uses of REQUIRES: win32.

I'm going to assume that target-windows was added to deal with the common case of wanting to XFAIL a test for mingw and windows at the same time. Now that it's easy to match them as just XFAIL: windows or REQUIRES: windows, I don't think we really need this target-windows feature.

As you mention, there are two remaining tests using target-windows. If you want to update this patch to delete the target-windows feature and migrate them to just "windows", that works for me. Less code is always better.

In D68135#1694961, @rnk wrote:

As you mention, there are two remaining tests using target-windows. If you want to update this patch to delete the target-windows feature and migrate them to just "windows", that works for me. Less code is always better.

I do that in D68450 (which depends on D68449 which was ok'd).

I'm happy to be wrong about the win32 part. And eliminating 'target-windows' in favor of using just 'windows' SGTM.

I'm happy to be wrong about the win32 part. And eliminating 'target-windows' in favor of using just 'windows' SGTM.

In practice, it's removing target-windows in tests where the target availability doesn't really matter, only which host platform it runs on. All of those tests already declared REQUIRES: system-windows as well, which should be enough for those tests (this landed in D68449).

mstorsjo abandoned this revision.Oct 13 2019, 1:04 PM

Made redundant by D68450.