-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
[flang][driver] Mark -fcommon and -mtune as visible in Flang #68657
Conversation
490145a
to
d056372
Compare
d056372
to
b7de162
Compare
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.
Thanks for working on this! I would appreciate if you could add a bit more testing.
@@ -10,6 +10,9 @@ | |||
! Make sure that `-L' is "visible" to Flang's driver | |||
! RUN: %flang -L/ -### %s | |||
|
|||
! Make sure that `-fcommon' is "visible" to Flang's driver |
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.
This is not clear because I didn't leave any comment in this file (mea culpa, sorry), but the other flags tested here are purely driver flags (i.e. there's is nothing for Flang to do here). However, IIUC, -fcommon
will affect code-generation and that's what we should be testing here instead,
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.
So should I put the test for this option somewhere else, or should I still put it in this file?
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.
Separate file, please.
@@ -5077,6 +5077,7 @@ def module_file_info : Flag<["-"], "module-file-info">, Flags<[NoXarchOption]>, | |||
HelpText<"Provide information about a particular module file">; | |||
def mthumb : Flag<["-"], "mthumb">, Group<m_Group>; | |||
def mtune_EQ : Joined<["-"], "mtune=">, Group<m_Group>, | |||
Flags<[TargetSpecific]>, Visibility<[ClangOption, CLOption, DXCOption, FlangOption]>, |
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.
How did you infer these visibility flags? Also, could you add a test for this flag?
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.
I made a mistake then and these options were not actually confirmed. Sorry, I will close the PR.
This commit marks option -fcommon and -mtune as visible in Flang.