[clang][Driver] Support -fms-volatile as equivalent to /volatile:ms #74790
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver Author: Eli Friedman (efriedma-quic) ChangesThe flag -fms-volatile has existed as a -cc1 flag for a while. It also technically existed as a driver flag, but didn't do anything because it wasn't wired up to anything in the driver. This patch adds -fno-ms-volatile, and makes both -fms-volatile and -fno-ms-volatile work the same way as the cl-mode flags. The defaults are unchanged (default on for x86 in cl mode, default off otherwise). Full diff: https://github.com/llvm/llvm-project/pull/74790.diff 4 Files Affected:
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 0eec2b3526376..6274ff76950d9 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -2869,9 +2869,11 @@ defm asm_blocks : BoolFOption<"asm-blocks",
LangOpts<"AsmBlocks">, Default<fms_extensions.KeyPath>,
PosFlag<SetTrue, [], [ClangOption, CC1Option]>,
NegFlag<SetFalse>>;
-def fms_volatile : Flag<["-"], "fms-volatile">, Group<f_Group>,
- Visibility<[ClangOption, CC1Option]>,
- MarshallingInfoFlag<LangOpts<"MSVolatile">>;
+defm ms_volatile : BoolFOption<"ms-volatile",
+ LangOpts<"MSVolatile">, DefaultFalse,
+ PosFlag<SetTrue, [], [ClangOption, CC1Option],
+ "Volatile loads and stores have acquire and release semantics">,
+ NegFlag<SetFalse>>;
def fmsc_version : Joined<["-"], "fmsc-version=">, Group<f_Group>,
Visibility<[ClangOption, CLOption]>,
HelpText<"Microsoft compiler version number to report in _MSC_VER (0 = don't define it (default))">;
@@ -8184,7 +8186,7 @@ def _SLASH_winsysroot : CLJoinedOrSeparate<"winsysroot">,
HelpText<"Same as \"/diasdkdir <dir>/DIA SDK\" /vctoolsdir <dir>/VC/Tools/MSVC/<vctoolsversion> \"/winsdkdir <dir>/Windows Kits/10\"">,
MetaVarName<"<dir>">;
def _SLASH_volatile_iso : Option<["/", "-"], "volatile:iso", KIND_FLAG>,
- Group<_SLASH_volatile_Group>, Flags<[NoXarchOption]>, Visibility<[CLOption]>,
+ Visibility<[CLOption]>, Alias<fno_ms_volatile>,
HelpText<"Volatile loads and stores have standard semantics">;
def _SLASH_vmb : CLFlag<"vmb">,
HelpText<"Use a best-case representation method for member pointers">;
@@ -8199,7 +8201,7 @@ def _SLASH_vmv : CLFlag<"vmv">,
HelpText<"Set the default most-general representation to "
"virtual inheritance">;
def _SLASH_volatile_ms : Option<["/", "-"], "volatile:ms", KIND_FLAG>,
- Group<_SLASH_volatile_Group>, Flags<[NoXarchOption]>, Visibility<[CLOption]>,
+ Visibility<[CLOption]>, Alias<fms_volatile>,
HelpText<"Volatile loads and stores have acquire and release semantics">;
def _SLASH_clang : CLJoined<"clang:">,
HelpText<"Pass <arg> to the clang driver">, MetaVarName<"<arg>">;
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index f02f7c841b91f..c6be0750e1c33 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -5545,6 +5545,10 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
options::OPT_fno_auto_import);
}
+ if (Args.hasFlag(options::OPT_fms_volatile, options::OPT_fno_ms_volatile,
+ Triple.isX86() && D.IsCLMode()))
+ CmdArgs.push_back("-fms-volatile");
+
// Non-PIC code defaults to -fdirect-access-external-data while PIC code
// defaults to -fno-direct-access-external-data. Pass the option if different
// from the default.
@@ -7877,18 +7881,6 @@ void Clang::AddClangCLArgs(const ArgList &Args, types::ID InputType,
CmdArgs.push_back("-P");
}
- unsigned VolatileOptionID;
- if (getToolChain().getTriple().isX86())
- VolatileOptionID = options::OPT__SLASH_volatile_ms;
- else
- VolatileOptionID = options::OPT__SLASH_volatile_iso;
-
- if (Arg *A = Args.getLastArg(options::OPT__SLASH_volatile_Group))
- VolatileOptionID = A->getOption().getID();
-
- if (VolatileOptionID == options::OPT__SLASH_volatile_ms)
- CmdArgs.push_back("-fms-volatile");
-
if (Args.hasFlag(options::OPT__SLASH_Zc_dllexportInlines_,
options::OPT__SLASH_Zc_dllexportInlines,
false)) {
diff --git a/clang/test/Driver/cl-options.c b/clang/test/Driver/cl-options.c
index 6d929b19e7e2e..6bc315b09477a 100644
--- a/clang/test/Driver/cl-options.c
+++ b/clang/test/Driver/cl-options.c
@@ -272,10 +272,12 @@
// RUN: not %clang_cl /vmg /vmm /vms -### -- %s 2>&1 | FileCheck -check-prefix=VMX %s
// VMX: '/vms' not allowed with '/vmm'
-// RUN: %clang_cl /volatile:iso -### -- %s 2>&1 | FileCheck -check-prefix=VOLATILE-ISO %s
+// RUN: %clang_cl --target=i686-pc-win32 /volatile:iso -### -- %s 2>&1 | FileCheck -check-prefix=VOLATILE-ISO %s
+// RUN: %clang_cl --target=aarch64-pc-win32 -### -- %s 2>&1 | FileCheck -check-prefix=VOLATILE-ISO %s
// VOLATILE-ISO-NOT: "-fms-volatile"
-// RUN: %clang_cl /volatile:ms -### -- %s 2>&1 | FileCheck -check-prefix=VOLATILE-MS %s
+// RUN: %clang_cl --target=aarch64-pc-win32 /volatile:ms -### -- %s 2>&1 | FileCheck -check-prefix=VOLATILE-MS %s
+// RUN: %clang_cl --target=i686-pc-win32 -### -- %s 2>&1 | FileCheck -check-prefix=VOLATILE-MS %s
// VOLATILE-MS: "-fms-volatile"
// RUN: %clang_cl /W0 -### -- %s 2>&1 | FileCheck -check-prefix=W0 %s
diff --git a/clang/test/Driver/clang_f_opts.c b/clang/test/Driver/clang_f_opts.c
index ebe8a0520bf0f..1e1489b39d643 100644
--- a/clang/test/Driver/clang_f_opts.c
+++ b/clang/test/Driver/clang_f_opts.c
@@ -611,3 +611,9 @@
// CHECK-INT-OBJEMITTER-NOT: unsupported option '-fintegrated-objemitter' for target
// RUN: not %clang -### -fno-integrated-objemitter --target=x86_64 %s 2>&1 | FileCheck -check-prefix=CHECK-NOINT-OBJEMITTER %s
// CHECK-NOINT-OBJEMITTER: unsupported option '-fno-integrated-objemitter' for target
+
+// RUN: %clang -### -target aarch64-windows-msvc %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MS-VOLATILE %s
+// RUN: %clang -### -target aarch64-windows-msvc -fms-volatile %s 2>&1 | FileCheck -check-prefix=CHECK-MS-VOLATILE %s
+// RUN: %clang -### -target aarch64-windows-msvc -fno-ms-volatile %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MS-VOLATILE %s
+// CHECK-MS-VOLATILE: -fms-volatile
+// CHECK-NO-MS-VOLATILE-NOT: -fms-volatile
|
|
@llvm/pr-subscribers-clang Author: Eli Friedman (efriedma-quic) ChangesThe flag -fms-volatile has existed as a -cc1 flag for a while. It also technically existed as a driver flag, but didn't do anything because it wasn't wired up to anything in the driver. This patch adds -fno-ms-volatile, and makes both -fms-volatile and -fno-ms-volatile work the same way as the cl-mode flags. The defaults are unchanged (default on for x86 in cl mode, default off otherwise). Full diff: https://github.com/llvm/llvm-project/pull/74790.diff 4 Files Affected:
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 0eec2b3526376..6274ff76950d9 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -2869,9 +2869,11 @@ defm asm_blocks : BoolFOption<"asm-blocks",
LangOpts<"AsmBlocks">, Default<fms_extensions.KeyPath>,
PosFlag<SetTrue, [], [ClangOption, CC1Option]>,
NegFlag<SetFalse>>;
-def fms_volatile : Flag<["-"], "fms-volatile">, Group<f_Group>,
- Visibility<[ClangOption, CC1Option]>,
- MarshallingInfoFlag<LangOpts<"MSVolatile">>;
+defm ms_volatile : BoolFOption<"ms-volatile",
+ LangOpts<"MSVolatile">, DefaultFalse,
+ PosFlag<SetTrue, [], [ClangOption, CC1Option],
+ "Volatile loads and stores have acquire and release semantics">,
+ NegFlag<SetFalse>>;
def fmsc_version : Joined<["-"], "fmsc-version=">, Group<f_Group>,
Visibility<[ClangOption, CLOption]>,
HelpText<"Microsoft compiler version number to report in _MSC_VER (0 = don't define it (default))">;
@@ -8184,7 +8186,7 @@ def _SLASH_winsysroot : CLJoinedOrSeparate<"winsysroot">,
HelpText<"Same as \"/diasdkdir <dir>/DIA SDK\" /vctoolsdir <dir>/VC/Tools/MSVC/<vctoolsversion> \"/winsdkdir <dir>/Windows Kits/10\"">,
MetaVarName<"<dir>">;
def _SLASH_volatile_iso : Option<["/", "-"], "volatile:iso", KIND_FLAG>,
- Group<_SLASH_volatile_Group>, Flags<[NoXarchOption]>, Visibility<[CLOption]>,
+ Visibility<[CLOption]>, Alias<fno_ms_volatile>,
HelpText<"Volatile loads and stores have standard semantics">;
def _SLASH_vmb : CLFlag<"vmb">,
HelpText<"Use a best-case representation method for member pointers">;
@@ -8199,7 +8201,7 @@ def _SLASH_vmv : CLFlag<"vmv">,
HelpText<"Set the default most-general representation to "
"virtual inheritance">;
def _SLASH_volatile_ms : Option<["/", "-"], "volatile:ms", KIND_FLAG>,
- Group<_SLASH_volatile_Group>, Flags<[NoXarchOption]>, Visibility<[CLOption]>,
+ Visibility<[CLOption]>, Alias<fms_volatile>,
HelpText<"Volatile loads and stores have acquire and release semantics">;
def _SLASH_clang : CLJoined<"clang:">,
HelpText<"Pass <arg> to the clang driver">, MetaVarName<"<arg>">;
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index f02f7c841b91f..c6be0750e1c33 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -5545,6 +5545,10 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
options::OPT_fno_auto_import);
}
+ if (Args.hasFlag(options::OPT_fms_volatile, options::OPT_fno_ms_volatile,
+ Triple.isX86() && D.IsCLMode()))
+ CmdArgs.push_back("-fms-volatile");
+
// Non-PIC code defaults to -fdirect-access-external-data while PIC code
// defaults to -fno-direct-access-external-data. Pass the option if different
// from the default.
@@ -7877,18 +7881,6 @@ void Clang::AddClangCLArgs(const ArgList &Args, types::ID InputType,
CmdArgs.push_back("-P");
}
- unsigned VolatileOptionID;
- if (getToolChain().getTriple().isX86())
- VolatileOptionID = options::OPT__SLASH_volatile_ms;
- else
- VolatileOptionID = options::OPT__SLASH_volatile_iso;
-
- if (Arg *A = Args.getLastArg(options::OPT__SLASH_volatile_Group))
- VolatileOptionID = A->getOption().getID();
-
- if (VolatileOptionID == options::OPT__SLASH_volatile_ms)
- CmdArgs.push_back("-fms-volatile");
-
if (Args.hasFlag(options::OPT__SLASH_Zc_dllexportInlines_,
options::OPT__SLASH_Zc_dllexportInlines,
false)) {
diff --git a/clang/test/Driver/cl-options.c b/clang/test/Driver/cl-options.c
index 6d929b19e7e2e..6bc315b09477a 100644
--- a/clang/test/Driver/cl-options.c
+++ b/clang/test/Driver/cl-options.c
@@ -272,10 +272,12 @@
// RUN: not %clang_cl /vmg /vmm /vms -### -- %s 2>&1 | FileCheck -check-prefix=VMX %s
// VMX: '/vms' not allowed with '/vmm'
-// RUN: %clang_cl /volatile:iso -### -- %s 2>&1 | FileCheck -check-prefix=VOLATILE-ISO %s
+// RUN: %clang_cl --target=i686-pc-win32 /volatile:iso -### -- %s 2>&1 | FileCheck -check-prefix=VOLATILE-ISO %s
+// RUN: %clang_cl --target=aarch64-pc-win32 -### -- %s 2>&1 | FileCheck -check-prefix=VOLATILE-ISO %s
// VOLATILE-ISO-NOT: "-fms-volatile"
-// RUN: %clang_cl /volatile:ms -### -- %s 2>&1 | FileCheck -check-prefix=VOLATILE-MS %s
+// RUN: %clang_cl --target=aarch64-pc-win32 /volatile:ms -### -- %s 2>&1 | FileCheck -check-prefix=VOLATILE-MS %s
+// RUN: %clang_cl --target=i686-pc-win32 -### -- %s 2>&1 | FileCheck -check-prefix=VOLATILE-MS %s
// VOLATILE-MS: "-fms-volatile"
// RUN: %clang_cl /W0 -### -- %s 2>&1 | FileCheck -check-prefix=W0 %s
diff --git a/clang/test/Driver/clang_f_opts.c b/clang/test/Driver/clang_f_opts.c
index ebe8a0520bf0f..1e1489b39d643 100644
--- a/clang/test/Driver/clang_f_opts.c
+++ b/clang/test/Driver/clang_f_opts.c
@@ -611,3 +611,9 @@
// CHECK-INT-OBJEMITTER-NOT: unsupported option '-fintegrated-objemitter' for target
// RUN: not %clang -### -fno-integrated-objemitter --target=x86_64 %s 2>&1 | FileCheck -check-prefix=CHECK-NOINT-OBJEMITTER %s
// CHECK-NOINT-OBJEMITTER: unsupported option '-fno-integrated-objemitter' for target
+
+// RUN: %clang -### -target aarch64-windows-msvc %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MS-VOLATILE %s
+// RUN: %clang -### -target aarch64-windows-msvc -fms-volatile %s 2>&1 | FileCheck -check-prefix=CHECK-MS-VOLATILE %s
+// RUN: %clang -### -target aarch64-windows-msvc -fno-ms-volatile %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MS-VOLATILE %s
+// CHECK-MS-VOLATILE: -fms-volatile
+// CHECK-NO-MS-VOLATILE-NOT: -fms-volatile
|
|
Current behavior: Shall we make -f[no-]ms-volatile an error for non-Windows targets? (mark it as |
rnk
reviewed
Dec 8, 2023
There was a problem hiding this comment.
I assume that /volatile:ms is a legacy behavior that non-Windows OSes likely don't want to adopt.
That is correct, it is legacy behavior. However, I think this flag has potential porting applications, similar to -fshort-wchar. I also think operating systems don't really have a say or control over what command line flags developers use. I think the more relevant question is, does the Clang project want to encourage folks to use this flag, thereby increasing our long term support burden? If this is a legacy compatibility mode, do we think we will be able to sunset it at some point, similar to -fdelayed-template-parsing?
|
I got a request for this feature from a user that ran into issues porting code from cl to clang for a baremetal target. I expect scenarios like that to continue to be relevant for a while. I don't expect anyone would want to use this for new code because current practice has strongly shifted towards using atomic types for synchronization since C++11. Like the commit message notes, this isn't changing any defaults; it's just giving users an additional tool for porting code. It's something we've already provided for many years; it just wasn't exposed outside cl-mode. And it's a pretty straightforward extension to support (it just slightly modifies IR generation), so I don't think there's much long-term support burden. |
rnk
approved these changes
Dec 12, 2023
MaskRay
approved these changes
Dec 12, 2023
clang/test/Driver/clang_f_opts.c
Outdated
| @@ -611,3 +611,9 @@ | |||
| // CHECK-INT-OBJEMITTER-NOT: unsupported option '-fintegrated-objemitter' for target | |||
| // RUN: not %clang -### -fno-integrated-objemitter --target=x86_64 %s 2>&1 | FileCheck -check-prefix=CHECK-NOINT-OBJEMITTER %s | |||
| // CHECK-NOINT-OBJEMITTER: unsupported option '-fno-integrated-objemitter' for target | |||
|
|
|||
| // RUN: %clang -### -target aarch64-windows-msvc %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MS-VOLATILE %s | |||
There was a problem hiding this comment.
--target= for new tests.
-target has been deprecated since clang 3.4.
The flag "-fms-volatile" has existed as a -cc1 flag for a while. It also technically existed as a driver flag, but didn't do anything because it wasn't wired up to anything in the driver. This patch adds -fno-ms-volatile, and makes both -fms-volatile and -fno-ms-volatile work the same way as the cl-mode flags. The defaults are unchanged (default on for x86 in cl mode, default off otherwise).
efriedma-quic
force-pushed
the
ms-volatile
branch
from
bd85822 to
476402a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The flag -fms-volatile has existed as a -cc1 flag for a while. It also technically existed as a driver flag, but didn't do anything because it wasn't wired up to anything in the driver.
This patch adds -fno-ms-volatile, and makes both -fms-volatile and -fno-ms-volatile work the same way as the cl-mode flags. The defaults are unchanged (default on for x86 in cl mode, default off otherwise).