-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[X86][clang-cl] Add CL option /vlen #166375
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
Conversation
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Phoebe Wang (phoebewang) ChangesRef: https://learn.microsoft.com/en-us/cpp/build/reference/vlen?view=msvc-170 Full diff: https://github.com/llvm/llvm-project/pull/166375.diff 4 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td b/clang/include/clang/Basic/DiagnosticDriverKinds.td
index 83980e3ac35b7..e657abb86d95f 100644
--- a/clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ b/clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -207,6 +207,9 @@ def err_drv_amdgpu_ieee_without_no_honor_nans : Error<
"invalid argument '-mno-amdgpu-ieee' only allowed with relaxed NaN handling">;
def err_drv_argument_not_allowed_with : Error<
"invalid argument '%0' not allowed with '%1'">;
+def warn_drv_argument_not_allowed_with : Warning<
+ "invalid argument '%0' not allowed with '%1'">,
+ InGroup<OptionIgnored>;
def err_drv_cannot_open_randomize_layout_seed_file : Error<
"cannot read randomize layout seed file '%0'">;
def err_drv_invalid_version_number : Error<
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 11e81e032d5fc..e271444184e44 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -9218,6 +9218,10 @@ def : CLFlag<"Qscatter-">, Alias<mno_scatter>,
def _SLASH_arch : CLCompileJoined<"arch:">,
HelpText<"Set architecture for code generation">;
+def _SLASH_vlen : CLCompileJoined<"vlen">,
+ HelpText<"Set vector length for autovectorization and other optimizations">;
+def _SLASH_vlen_default : CLFlag<"vlen">,
+ HelpText<"Set default vector length for autovectorization and other optimizations">;
def _SLASH_M_Group : OptionGroup<"</M group>">, Group<cl_compile_Group>;
def _SLASH_volatile_Group : OptionGroup<"</volatile group>">,
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 30d3e5293a31b..add34c63d9c24 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -8266,6 +8266,37 @@ void Clang::AddClangCLArgs(const ArgList &Args, types::ID InputType,
<< "/kernel";
}
+ if (Args.hasArg(options::OPT__SLASH_vlen_default)) {
+ // Override /vlen= options.
+ for (const Arg *A : Args.filtered(options::OPT__SLASH_vlen))
+ A->claim();
+ // Nothing to do for AVX512F/AVX512.
+ } else if (const Arg *A = Args.getLastArg(options::OPT__SLASH_vlen)) {
+ StringRef VLen = A->getValue();
+ llvm::SmallSet<std::string, 4> Arch512 = {"AVX512F", "AVX512"};
+ llvm::SmallSet<std::string, 4> Arch256 = {"AVX", "AVX2"};
+
+ StringRef Arch = Args.getLastArgValue(options::OPT__SLASH_arch);
+ if (Arch != "") {
+ if (VLen == "=512") {
+ if (Arch512.contains(Arch.str()))
+ CmdArgs.push_back("-mprefer-vector-width=512");
+ else
+ D.Diag(diag::warn_drv_argument_not_allowed_with)
+ << "/vlen=512" << std::string("/arch:").append(Arch);
+ } else if (VLen == "=256") {
+ if (Arch512.contains(Arch.str()))
+ CmdArgs.push_back("-mprefer-vector-width=256");
+ else if (!Arch256.contains(Arch.str()))
+ D.Diag(diag::warn_drv_argument_not_allowed_with)
+ << "/vlen=256" << std::string("/arch:").append(Arch);
+ } else {
+ D.Diag(diag::warn_drv_unknown_argument_clang_cl)
+ << std::string("/vlen").append(VLen);
+ }
+ }
+ }
+
Arg *MostGeneralArg = Args.getLastArg(options::OPT__SLASH_vmg);
Arg *BestCaseArg = Args.getLastArg(options::OPT__SLASH_vmb);
if (MostGeneralArg && BestCaseArg)
diff --git a/clang/test/Driver/cl-x86-flags.c b/clang/test/Driver/cl-x86-flags.c
index 89526744c0a49..56cfcb420f4ad 100644
--- a/clang/test/Driver/cl-x86-flags.c
+++ b/clang/test/Driver/cl-x86-flags.c
@@ -133,6 +133,27 @@
// tune: "-target-cpu" "sandybridge"
// tune-SAME: "-tune-cpu" "haswell"
+// RUN: %clang_cl -m64 -arch:AVX512 -vlen=512 --target=x86_64-pc-windows -### -- 2>&1 %s | FileCheck -check-prefix=vlen512 %s
+// vlen512: "-mprefer-vector-width=512"
+
+// RUN: %clang_cl -m64 -arch:AVX512 -vlen=256 --target=x86_64-pc-windows -### -- 2>&1 %s | FileCheck -check-prefix=vlen256 %s
+// vlen256: "-mprefer-vector-width=256"
+
+// RUN: %clang_cl -m64 -arch:AVX512 -vlen=512 -vlen --target=x86_64-pc-windows -### -- 2>&1 %s | FileCheck -check-prefix=noprefer %s
+// noprefer-NOT: -mprefer-vector-width
+
+// RUN: %clang_cl -m64 -arch:AVX2 -vlen=512 --target=x86_64-pc-windows -### -- 2>&1 %s | FileCheck -check-prefix=avx2vlen512 %s
+// avx2vlen512: invalid argument '/vlen=512' not allowed with '/arch:AVX2'
+
+// RUN: %clang_cl -m64 -arch:AVX2 -vlen=256 --target=x86_64-pc-windows -### -- 2>&1 %s | FileCheck -check-prefix=avx2vlen256 %s
+// avx2vlen256-NOT: invalid argument
+
+// RUN: %clang_cl -m32 -arch:SSE2 -vlen=256 --target=i386-pc-windows -### -- 2>&1 %s | FileCheck -check-prefix=sse2vlen256 %s
+// sse2vlen256: invalid argument '/vlen=256' not allowed with '/arch:SSE2'
+
+// RUN: %clang_cl -m64 -arch:AVX -vlen256 --target=x86_64-pc-windows -### -- 2>&1 %s | FileCheck -check-prefix=avxvlen256 %s
+// avxvlen256: unknown argument ignored in clang-cl: '/vlen256'
+
void f(void) {
}
|
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions cpp,c -- clang/lib/Driver/ToolChains/Clang.cpp clang/test/Driver/cl-x86-flags.c --diff_from_common_commit
View the diff from clang-format here.diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index e20963ac2..5d3d4e741 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -8266,29 +8266,29 @@ void Clang::AddClangCLArgs(const ArgList &Args, types::ID InputType,
<< "/kernel";
}
- if (const Arg *A = Args.getLastArg(options::OPT__SLASH_vlen,
- options::OPT__SLASH_vlen_EQ_256,
- options::OPT__SLASH_vlen_EQ_512)) {
- llvm::Triple::ArchType AT = getToolChain().getArch();
- StringRef Default = AT == llvm::Triple::x86 ? "IA32" : "SSE2";
- StringRef Arch = Args.getLastArgValue(options::OPT__SLASH_arch, Default);
-
- if (A->getOption().matches(options::OPT__SLASH_vlen_EQ_512)) {
- if (Arch == "AVX512F" || Arch == "AVX512")
- CmdArgs.push_back("-mprefer-vector-width=512");
- else
- D.Diag(diag::warn_drv_argument_not_allowed_with)
- << "/vlen=512" << std::string("/arch:").append(Arch);
- }
+ if (const Arg *A = Args.getLastArg(options::OPT__SLASH_vlen,
+ options::OPT__SLASH_vlen_EQ_256,
+ options::OPT__SLASH_vlen_EQ_512)) {
+ llvm::Triple::ArchType AT = getToolChain().getArch();
+ StringRef Default = AT == llvm::Triple::x86 ? "IA32" : "SSE2";
+ StringRef Arch = Args.getLastArgValue(options::OPT__SLASH_arch, Default);
+
+ if (A->getOption().matches(options::OPT__SLASH_vlen_EQ_512)) {
+ if (Arch == "AVX512F" || Arch == "AVX512")
+ CmdArgs.push_back("-mprefer-vector-width=512");
+ else
+ D.Diag(diag::warn_drv_argument_not_allowed_with)
+ << "/vlen=512" << std::string("/arch:").append(Arch);
+ }
- if (A->getOption().matches(options::OPT__SLASH_vlen_EQ_256)) {
- if (Arch == "AVX512F" || Arch == "AVX512")
- CmdArgs.push_back("-mprefer-vector-width=256");
- else if (Arch != "AVX" && Arch != "AVX2")
- D.Diag(diag::warn_drv_argument_not_allowed_with)
- << "/vlen=256" << std::string("/arch:").append(Arch);
- }
- }
+ if (A->getOption().matches(options::OPT__SLASH_vlen_EQ_256)) {
+ if (Arch == "AVX512F" || Arch == "AVX512")
+ CmdArgs.push_back("-mprefer-vector-width=256");
+ else if (Arch != "AVX" && Arch != "AVX2")
+ D.Diag(diag::warn_drv_argument_not_allowed_with)
+ << "/vlen=256" << std::string("/arch:").append(Arch);
+ }
+ }
Arg *MostGeneralArg = Args.getLastArg(options::OPT__SLASH_vmg);
Arg *BestCaseArg = Args.getLastArg(options::OPT__SLASH_vmb);
|
Doesn't this add /vlen ? clang-cl already handles /arch |
| def _SLASH_vlen : CLCompileJoined<"vlen">, | ||
| HelpText<"Set vector length for autovectorization and other optimizations">; | ||
| def _SLASH_vlen_default : CLFlag<"vlen">, | ||
| HelpText<"Set default vector length for autovectorization and other optimizations">; |
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 think these should be _SLASH_vlen (for the default) and _SLASH_vlen_EQ for the one ending in = (and that should be included in the flag spelling.
But, since only two values are allowed, it might be simpler to do something like:
def _SLASH_vlen : CLFLag<"vlen"> ...
def _SLASH_vlen_EQ_256 : CLFLag<"vlen=256"> ...
def _SLASH_vlen_EQ_512 : CLFlag<"vlen=512"> ...
Does Clang provide good diagnostics when passing an -mprefer-vector-width that doesn't fit the target architecture?
If so, could we make _SLASH_vlen_EQ_256 an alias for -mprefer-vector-width=256 and so on, and not have to add any new driver logic at all?
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.
Good idea! No, unfortunately: https://godbolt.org/z/hhvjP8W34
| } | ||
|
|
||
| if (Args.hasArg(options::OPT__SLASH_vlen_default)) { | ||
| // Override /vlen= options. |
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.
Shouldn't the last flag take precedence? For example, in /arch:AVX512 /vlen /vlen=256, shouldn't 256 win? What does MSVC do?
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.
Yes, MSVC also takes precedence of the last flag: https://godbolt.org/z/43Ga39WhE
| StringRef Arch = Args.getLastArgValue(options::OPT__SLASH_arch); | ||
|
|
||
| if (A->getOption().matches(options::OPT__SLASH_vlen_EQ_512)) { | ||
| if (Arch512.contains(Arch.str())) |
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.
Since the set is so small, and only used here, I think just open-coding the logic would be easier:
if (Arch == "AVX512" || Arch == "AVX512F")
Same for the Arch256 set below.
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.
We will add AVX10.1, AVX10.2 soon, but I can change it for now.
| llvm::SmallSet<std::string, 4> Arch512 = {"AVX512F", "AVX512"}; | ||
| llvm::SmallSet<std::string, 4> Arch256 = {"AVX", "AVX2"}; | ||
|
|
||
| StringRef Arch = Args.getLastArgValue(options::OPT__SLASH_arch); |
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.
What if the user didn't pass /arch?
What if they passed -mavx512?
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.
Good question. MSVC has a default value for /arch. We should match it here. -mavx512xxx is a single feature, which is orthogonal to /arch. We don't need to check them.
clang/test/Driver/cl-x86-flags.c
Outdated
| // RUN: %clang_cl -m64 -arch:AVX512 -vlen=256 --target=x86_64-pc-windows -### -- 2>&1 %s | FileCheck -check-prefix=vlen256 %s | ||
| // vlen256: "-mprefer-vector-width=256" | ||
|
|
||
| // RUN: %clang_cl -m64 -arch:AVX512 -vlen=512 -vlen --target=x86_64-pc-windows -### -- 2>&1 %s | FileCheck -check-prefix=noprefer %s |
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 about novlen instead of noprefer?
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.
Done.
zmodem
left a comment
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.
lgtm
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/18645 Here is the relevant piece of the build log for the reference |
Ref: https://learn.microsoft.com/en-us/cpp/build/reference/vlen?view=msvc-170