-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[Driver] Move CommonArgs to a location visible by the Frontend Drivers #142800
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
918b853
28c4762
aef6882
db4482d
a6535b1
ef34cdf
121c41a
8b69907
2691029
d33a5f9
ca89f42
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ | |
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
#include "CommonArgs.h" | ||
#include "clang/Driver/CommonArgs.h" | ||
#include "Arch/AArch64.h" | ||
#include "Arch/ARM.h" | ||
#include "Arch/CSKY.h" | ||
|
@@ -3167,3 +3167,29 @@ void tools::handleInterchangeLoopsArgs(const ArgList &Args, | |
options::OPT_fno_loop_interchange, EnableInterchange)) | ||
CmdArgs.push_back("-floop-interchange"); | ||
} | ||
|
||
std::optional<StringRef> tools::ParseMPreferVectorWidthOption( | ||
clang::DiagnosticsEngine &Diags, const llvm::opt::ArgList &Args, | ||
ArgStringList &CmdArgs, bool isCompilerDriver) { | ||
// If this was invoked by the Compiler Driver, we pass through the option | ||
// as-is. Otherwise, if this is the Frontend Driver, we want just the value. | ||
StringRef Out = isCompilerDriver ? "-mprefer-vector-width=" : ""; | ||
|
||
Arg *A = Args.getLastArg(clang::driver::options::OPT_mprefer_vector_width_EQ); | ||
if (!A) | ||
return std::nullopt; | ||
|
||
StringRef Value = A->getValue(); | ||
unsigned Width __attribute((uninitialized)); | ||
|
||
// Only "none" and Integer values are accepted by | ||
// -mprefer-vector-width=<value>. | ||
if (Value != "none" && Value.getAsInteger(10, Width)) { | ||
Diags.Report(clang::diag::err_drv_invalid_value) | ||
<< A->getOption().getName() << Value; | ||
return std::nullopt; | ||
} | ||
|
||
CmdArgs.push_back(Args.MakeArgString(Out + Value)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: maybe the function should just return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The way this function behaves is consistent with the way options are handled in this part of the code. We could consider renaming the function There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that this aligns with how other functions work. At the same time, it seems awkward to me that we push just the value into the args-list, when the caller is a frontend driver. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is awkward. I was hoping someone with more C++ expertise would have a trick to streamline it. I chose this solution so that the Compiler Driver side would be as undisturbed as possible. I also don't feel strongly that it needs to change for an initial implementation. It's something we could build out in the future as we learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function could take an That way the compiler driver can pass the list and ignore the result and the frontend can just use the result. Not sure if it would works with all options, I am not familiar with the drivers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That would work. It would disturb the Compiler Driver a bit to avoid warnings. We'd need to explicitly discard the results when unwanted. E.g.:
I wonder if we could come up with a Frontend Driver mechanism to discard everything up to and including the
Same. I suggest that we delay this decision until there's a better view of the entire problem. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Argh! Sorry, I somehow missed the real issue here. Sorry, it must have been one of those days.
Just to clarify, by warnings you mean warnings when building
Is the real issue here that we need to validate the argument values? It is a fairly common pattern in, e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Correct. The unused result would be a warning.
Yes, that's what we're trying to avoid. Both
I like that!! I'll wait to see if there are other ideas before updating the PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I take that back. This solution is pretty slick. Sharing an update now... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It may be better to always return just the value here, instead of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The alternative would be to not add to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's a good point as well. There are plenty of Let me code that up and we can see if we like it... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just pushed a new patch. I like it a lot. See what you think... We were able to get rid of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ))) this is what I suggested 2 days ago Thank you, Cameron! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes. As I said 1 day ago, I am not sure how I missed the obvious the first time around. :-) |
||
return Value; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a docstring for this function