Page MenuHomePhabricator

[NFC] Extract unifyTargetFeatures
ClosedPublic

Authored by yaxunl on Jun 25 2020, 9:55 AM.

Details

Summary

extract unifyTargetFeatures to be used by lld.

Diff Detail

Event Timeline

yaxunl created this revision.Jun 25 2020, 9:55 AM
tra added inline comments.Jun 25 2020, 10:44 AM
clang/lib/Driver/ToolChains/CommonArgs.cpp
93

I don't think assert should be used for something that may be specified to the user on command line.

97

Would it be simpler to just iterate over features backwards and skip the features we've already seen?

clang/lib/Driver/ToolChains/CommonArgs.h
120

Returning a new std::vector<StringRef> would make it more convenient to use and there's less complexity -- no need for the caller to create a temporary vector and the function itself can just return the temporary vector it collects the items in.

for (auto Feature : unifyTargetFeatures(Features)) {
  ...
}
yaxunl marked 4 inline comments as done.Jun 25 2020, 11:38 AM
yaxunl added inline comments.
clang/lib/Driver/ToolChains/CommonArgs.cpp
93

The entries in this array is not directly from command line.

It is either put in by clang code, or translated from -mxxx or -mno-xxx options whereas the translated entry should always have + or - prefix. So if things go wrong, it is due to developers.

97

That may save a little when there are repeating entries but you need to reverse the obtained vector to maintain the order. Since most cases there are just a few unique features, reversing may ends up slower.

yaxunl updated this revision to Diff 273473.Jun 25 2020, 11:53 AM
yaxunl marked 2 inline comments as done.

return the unified features.

tra marked an inline comment as done.Jun 25 2020, 12:19 PM
tra added inline comments.
clang/lib/Driver/ToolChains/CommonArgs.cpp
93

While clang may be the primary user of those options, features can be specified by the user, too.
The general rule of thumb is that invalid external input should produce diagnostics, not compiler crashes.
Asserts are for programming errors. Granted, when the inputs may be generated by our code the boundary is blurred, but in this case I would err on the side of not crashing.

97

Up to you. Performance does not matter here, but simpler code with improved readability is always a plus.

yaxunl marked 3 inline comments as done.Jun 25 2020, 1:21 PM
yaxunl added inline comments.
clang/lib/Driver/ToolChains/CommonArgs.cpp
93

user may use -target-feature to directly specify features and they may forget +/- there but that is a -cc1 option, whereas this function is used in tool and will never be used in -cc1. The features in this vector can only come from clang driver, where the code is supposed to always add +/- to the entries. How could user inputs can cause +/- not added?

tra accepted this revision.Jun 25 2020, 2:01 PM
tra added inline comments.
clang/lib/Driver/ToolChains/CommonArgs.cpp
93
This revision is now accepted and ready to land.Jun 25 2020, 2:01 PM
This revision was automatically updated to reflect the committed changes.
yaxunl marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptJun 25 2020, 8:45 PM