[clang] Add flag to opt out of wasm-opt (PR #95208)
https://github.com/mh4ck-Thales created
https://github.com/llvm/llvm-project/pull/95208
This PR fixes #55781 by adding the `--no-wasm-opt` and `--wasm-opt` flags in
clang to disable/enable the `wasm-opt` optimizations. The default is to enable
`wasm-opt` as before in order to not break existing workflows.
I think that adding a warning when no flag or the `--wasm-opt` flag is given
but `wasm-opt` wasn't found in the path may be relevant here. It allows people
using `wasm-opt` to be aware of if it have been used on their produced binary
or not. The only downside I see to this is that people already using the
toolchain with the `-O` and `-Werror` flags but without `wasm-opt` in the path
will see their toolchain break (with an easy fix: either adding `--no-wasm-opt`
or add `wasm-opt` to the path). I haven't implemented this here because I
haven't figured out how to add such a warning, and I don't know if this warning
should be added here or in another PR.
CC @sunfishcode that proposed in the associated issue to review this patch.
>From 8c7052d5da074eb1d754aeddc4257d33e4e299aa Mon Sep 17 00:00:00 2001
From: Quentin Michaud
Date: Tue, 11 Jun 2024 16:25:51 +0200
Subject: [PATCH] Add flag to opt out of wasm-opt (#55781)
---
clang/include/clang/Basic/LangOptions.h | 4 ++
clang/include/clang/Driver/Options.td | 8 +++
clang/lib/Driver/ToolChains/WebAssembly.cpp | 72 +++--
3 files changed, 49 insertions(+), 35 deletions(-)
diff --git a/clang/include/clang/Basic/LangOptions.h
b/clang/include/clang/Basic/LangOptions.h
index 75e88afbd9705..91f1c2f2e6239 100644
--- a/clang/include/clang/Basic/LangOptions.h
+++ b/clang/include/clang/Basic/LangOptions.h
@@ -575,6 +575,10 @@ class LangOptions : public LangOptionsBase {
// implementation on real-world examples.
std::string OpenACCMacroOverride;
+ // Indicates if the wasm-opt binary must be ignored in the case of a
+ // WebAssembly target.
+ bool NoWasmOpt = false;
+
LangOptions();
/// Set language defaults for the given input language and
diff --git a/clang/include/clang/Driver/Options.td
b/clang/include/clang/Driver/Options.td
index d44faa55c456f..22a400c9707b1 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -8740,3 +8740,11 @@ def spirv : DXCFlag<"spirv">,
def fspv_target_env_EQ : Joined<["-"], "fspv-target-env=">, Group,
HelpText<"Specify the target environment">,
Values<"vulkan1.2, vulkan1.3">;
+def no_wasm_opt : Flag<["--"], "no-wasm-opt">,
+ Group,
+ HelpText<"Disable the wasm-opt optimizer">,
+ MarshallingInfoFlag>;
+def wasm_opt : Flag<["--"], "wasm-opt">,
+ Group,
+ HelpText<"Enable the wasm-opt optimizer (default)">,
+ MarshallingInfoNegativeFlag>;
diff --git a/clang/lib/Driver/ToolChains/WebAssembly.cpp
b/clang/lib/Driver/ToolChains/WebAssembly.cpp
index 5b763df9b3329..60bd97e0ee987 100644
--- a/clang/lib/Driver/ToolChains/WebAssembly.cpp
+++ b/clang/lib/Driver/ToolChains/WebAssembly.cpp
@@ -158,44 +158,46 @@ void wasm::Linker::ConstructJob(Compilation &C, const
JobAction &JA,
CmdArgs.push_back("-o");
CmdArgs.push_back(Output.getFilename());
- // When optimizing, if wasm-opt is available, run it.
- std::string WasmOptPath;
- if (Args.getLastArg(options::OPT_O_Group)) {
-WasmOptPath = ToolChain.GetProgramPath("wasm-opt");
-if (WasmOptPath == "wasm-opt") {
- WasmOptPath = {};
+ if (Args.hasFlag(options::OPT_wasm_opt, options::OPT_no_wasm_opt, true)) {
+// When optimizing, if wasm-opt is available, run it.
+std::string WasmOptPath;
+if (Args.getLastArg(options::OPT_O_Group)) {
+ WasmOptPath = ToolChain.GetProgramPath("wasm-opt");
+ if (WasmOptPath == "wasm-opt") {
+WasmOptPath = {};
+ }
}
- }
-
- if (!WasmOptPath.empty()) {
-CmdArgs.push_back("--keep-section=target_features");
- }
- C.addCommand(std::make_unique(JA, *this,
- ResponseFileSupport::AtFileCurCP(),
- Linker, CmdArgs, Inputs, Output));
-
- if (Arg *A = Args.getLastArg(options::OPT_O_Group)) {
if (!WasmOptPath.empty()) {
- StringRef OOpt = "s";
- if (A->getOption().matches(options::OPT_O4) ||
- A->getOption().matches(options::OPT_Ofast))
-OOpt = "4";
- else if (A->getOption().matches(options::OPT_O0))
-OOpt = "0";
- else if (A->getOption().matches(options::OPT_O))
-OOpt = A->getValue();
-
- if (OOpt != "0") {
-const char *WasmOpt = Args.MakeArgString(WasmOptPath);
-ArgStringList OptArgs;
-OptArgs.push_back(Output.getFilename());
-OptArgs.push_back(Args.MakeArgString(llvm::Twine("-O") + OOpt));
-OptArgs.push_back("-o");
-OptArgs.push_back(Output.getFilename());
-C.addCommand(std::make_unique(
-JA, *this, ResponseFileSupport::AtFileCurCP(), WasmOpt, OptArgs,
-Inputs, Output));
+
[clang] Add flag to opt out of wasm-opt (PR #95208)
mh4ck-Thales wrote: Any opinion on whether to implement a warning or not ? https://github.com/llvm/llvm-project/pull/95208 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add wasm-opt warning (PR #100321)
https://github.com/mh4ck-Thales created
https://github.com/llvm/llvm-project/pull/100321
Add a warning when `wasm-opt` is requested with a flag (#95208) but is not
found in the path.
I'm using #77148 as reference on how to add a new warning but please tell me if
you can help in implementing this. Also I'm not sure in which warning group
include this, at first glance none seem relevant for this specific problem.
Regarding https://github.com/llvm/llvm-project/pull/98373 another warning might
be relevant if people target WASIp2+ and try to use wasm-opt explicitly. This
can be grouped in one warning "wasm-opt not available in this context" or
separated in 2 for clearer error messages. WDYT?
CC @sunfishcode @sbc100 @alexcrichton
>From 6a74d3a93d595592ee97120ad60a6c262a2c4d29 Mon Sep 17 00:00:00 2001
From: Quentin Michaud
Date: Wed, 24 Jul 2024 09:54:53 +0200
Subject: [PATCH] First draft of wasm-opt warning
---
clang/lib/Driver/ToolChains/WebAssembly.cpp | 3 +++
1 file changed, 3 insertions(+)
diff --git a/clang/lib/Driver/ToolChains/WebAssembly.cpp
b/clang/lib/Driver/ToolChains/WebAssembly.cpp
index 9aacda5fd5702..dc49fde378a9a 100644
--- a/clang/lib/Driver/ToolChains/WebAssembly.cpp
+++ b/clang/lib/Driver/ToolChains/WebAssembly.cpp
@@ -180,6 +180,9 @@ void wasm::Linker::ConstructJob(Compilation &C, const
JobAction &JA,
if (WasmOptPath == "wasm-opt") {
WasmOptPath = {};
}
+if (WasmOptPath.empty()) {
+ printf("warning: wasm-opt not found but was requested\n");
+}
}
if (!WasmOptPath.empty()) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add wasm-opt warning (PR #100321)
mh4ck-Thales wrote: `wasm-opt` is enabled by default for WASIp1, but can be disabled with the `--no-wasm-opt` flag. Indeed if it is explicitly enabled then an error instead of a warning would make more sense. I'm not sure what is the problem with > most developers I know of on wasm32-wasip1 don't have wasm-opt in their path > meaning that this would create many un-suppressable warnings by default for > these projects. The solution to this is just to add `--no-wasm-opt` to the compilation command and no warning will be displayed. I agree that `wasm-opt` should have been opt-in when it have been introduced into LLVM, it has already been debated in https://github.com/llvm/llvm-project/issues/55781 and the consensus is that we cannot go back on the default behavior as this would be a breaking change. A warning when `wasm-opt` is supposed to be used but is not present seems a reasonable solution to me in this situation. In the longer term, it would be nice to see if we can package and distribute `wasm-opt` alongside LLVM. Maybe it cannot be done in LLVM directly, but it could be a nice addition to wasi-sdk. https://github.com/llvm/llvm-project/pull/100321 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add wasm-opt warning (PR #100321)
https://github.com/mh4ck-Thales updated
https://github.com/llvm/llvm-project/pull/100321
>From b812b9c2a7b92edf5dab739eadff0295c2a1f631 Mon Sep 17 00:00:00 2001
From: Quentin Michaud
Date: Wed, 24 Jul 2024 09:54:53 +0200
Subject: [PATCH 1/2] First draft of wasm-opt warning
---
clang/lib/Driver/ToolChains/WebAssembly.cpp | 3 +++
1 file changed, 3 insertions(+)
diff --git a/clang/lib/Driver/ToolChains/WebAssembly.cpp
b/clang/lib/Driver/ToolChains/WebAssembly.cpp
index 9aacda5fd5702f..dc49fde378a9a7 100644
--- a/clang/lib/Driver/ToolChains/WebAssembly.cpp
+++ b/clang/lib/Driver/ToolChains/WebAssembly.cpp
@@ -180,6 +180,9 @@ void wasm::Linker::ConstructJob(Compilation &C, const
JobAction &JA,
if (WasmOptPath == "wasm-opt") {
WasmOptPath = {};
}
+if (WasmOptPath.empty()) {
+ printf("warning: wasm-opt not found but was requested\n");
+}
}
if (!WasmOptPath.empty()) {
>From a798b1fd7a7a6c7b782584021e5c32e52c3abffc Mon Sep 17 00:00:00 2001
From: Quentin Michaud
Date: Thu, 29 Aug 2024 16:31:48 +0200
Subject: [PATCH 2/2] Add errors and warnings for when wasm-opt is missing or
invoked but incompatible
---
clang/include/clang/Basic/DiagnosticDriverKinds.td | 8
clang/include/clang/Basic/DiagnosticGroups.td | 2 ++
clang/include/clang/Driver/Options.td | 1 +
clang/lib/Driver/ToolChains/WebAssembly.cpp| 14 +-
4 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td
b/clang/include/clang/Basic/DiagnosticDriverKinds.td
index ba90742fbdaabc..2a862edf8788c8 100644
--- a/clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ b/clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -826,4 +826,12 @@ def err_drv_triple_version_invalid : Error<
def warn_missing_include_dirs : Warning<
"no such include directory: '%0'">, InGroup,
DefaultIgnore;
+
+def warn_wasm_opt_not_found : Warning<
+ "wasm-opt was not found, some optimizations were not applied">,
+ InGroup;
+def err_wasm_opt_not_found_with_flag : Error<
+ "wasm-opt was explicitly requested, but was not found">;
+def err_wasm_opt_requested_but_not_supported : Error<
+ "wasm-opt was explicitly requested, but is not supported with '%0'">;
}
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td
b/clang/include/clang/Basic/DiagnosticGroups.td
index 28d315f63e5c47..fab11f47492db3 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1520,6 +1520,8 @@ in addition with the pragmas or -fmax-tokens flag to get
any warnings.
def WebAssemblyExceptionSpec : DiagGroup<"wasm-exception-spec">;
+def WebAssemblyOptimization : DiagGroup<"wasm-opt">;
+
def RTTI : DiagGroup<"rtti">;
def OpenCLCoreFeaturesDiagGroup : DiagGroup<"pedantic-core-features">;
diff --git a/clang/include/clang/Driver/Options.td
b/clang/include/clang/Driver/Options.td
index 83cf753e824845..ba6e7ab9e11d06 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -8923,3 +8923,4 @@ def wasm_opt : Flag<["--"], "wasm-opt">,
Group,
HelpText<"Enable the wasm-opt optimizer (default)">,
MarshallingInfoNegativeFlag>;
+def Wwarn_wasm_opt_not_found : Flag<["-"], "Wwarn_wasm_opt_not_found">;
diff --git a/clang/lib/Driver/ToolChains/WebAssembly.cpp
b/clang/lib/Driver/ToolChains/WebAssembly.cpp
index dc49fde378a9a7..e9820ce1d35995 100644
--- a/clang/lib/Driver/ToolChains/WebAssembly.cpp
+++ b/clang/lib/Driver/ToolChains/WebAssembly.cpp
@@ -9,6 +9,7 @@
#include "WebAssembly.h"
#include "CommonArgs.h"
#include "Gnu.h"
+#include "clang/Basic/DiagnosticDriver.h"
#include "clang/Basic/Version.h"
#include "clang/Config/config.h"
#include "clang/Driver/Compilation.h"
@@ -20,6 +21,7 @@
#include "llvm/Support/Path.h"
#include "llvm/Support/VirtualFileSystem.h"
+
using namespace clang::driver;
using namespace clang::driver::tools;
using namespace clang::driver::toolchains;
@@ -172,6 +174,11 @@ void wasm::Linker::ConstructJob(Compilation &C, const
JobAction &JA,
bool RunWasmOpt = Args.hasFlag(options::OPT_wasm_opt,
options::OPT_no_wasm_opt, WasmOptDefault);
+ if (TargetBuildsComponents(ToolChain.getTriple()) &&
+ Args.hasFlag(options::OPT_wasm_opt, options::OPT_no_wasm_opt, false)) {
+ToolChain.getDriver().Diag(diag::err_wasm_opt_requested_but_not_supported)
+<< ToolChain.getTriple().str();
+ }
// If wasm-opt is enabled and optimizations are happening look for the
// `wasm-opt` program. If it's not found auto-disable it.
std::string WasmOptPath;
@@ -181,7 +188,12 @@ void wasm::Linker::ConstructJob(Compilation &C, const
JobAction &JA,
WasmOptPath = {};
}
if (WasmOptPath.empty()) {
- printf("warning: wasm-opt not found but was requested\n");
+ if (Args.hasFlag(options::OPT_wasm_opt, options::OPT_no_wasm_opt,
+ false)) {
+
[clang] Add wasm-opt warning (PR #100321)
mh4ck-Thales wrote: I implemented a proposition on how to proceed for correctly informing users of wasm-opt problems: - a warning for when no flags are passed (the default being wasm-opt enabled for wasip1) and wasm-opt is not found - an error for when --wasm-opt is explicitly passed but wasm-opt is not found (as @alexcrichton suggested) - another error for when --wasm-opt is explicitly passed but wasm-opt is not supported (wasip2 and components) We can discuss and change this, especially for the warning "by default" that is controversial. I think that a warning is the way to go if we want to leave wasm-opt enabled as a default, otherwise people with different tooling end up with vastly differing results without knowing why, but the point of @alexcrichton on the annoyance of having a warning is relevant. Another solution could be to just print a message without using warnings? Is there a mechanism in LLVM for printing info notes? https://github.com/llvm/llvm-project/pull/100321 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add wasm-opt warning (PR #100321)
https://github.com/mh4ck-Thales ready_for_review https://github.com/llvm/llvm-project/pull/100321 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add flag to opt out of wasm-opt (PR #95208)
mh4ck-Thales wrote: @sbc100 I tried to limit the scope of this new option to Wasm using the corresponding group in `Options.td` (https://github.com/ThalesGroup/llvm-project/blob/8c7052d5da074eb1d754aeddc4257d33e4e299aa/clang/include/clang/Driver/Options.td#L222), but for some reason I was unable to make it work. @sunfishcode Is there a kind of deprecation process in LLVM, when we can warn of breaking changes ? Otherwise not sure how to handle it. https://github.com/llvm/llvm-project/pull/95208 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [WebAssembly] Disable running `wasm-opt` on components (PR #98373)
mh4ck-Thales wrote: The tests for the use of `wasm-opt` are hard to integrate into the LLVM project as `wasm-opt` is not part of the LLVM toolchain itself (which is the initial use case for the new `--no-wasm-opt` flag: as LLVM is not distributed with `wasm-opt` by default a same compilation command silently resulted of different binaries depending on the presence or not of `wasm-opt`...). I guess maybe one can hack the test suite in some way for this specific case, or maybe a suitable solution could be found to integrate `wasm-opt` into the LLVM distributions. @alexcrichton would it be relevant to open an issue in the binaryen repo for the support of Wasm components? https://github.com/llvm/llvm-project/pull/98373 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [WebAssembly] Disable running `wasm-opt` on components (PR #98373)
mh4ck-Thales wrote: A shell script would work. I don't know how easy it is to create a shell script inside the test suite and use it though, and it would be nice if this fake wasm-opt script could be unique across several tests, to ensure the consistency of those. https://github.com/llvm/llvm-project/pull/98373 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [WebAssembly] Disable running `wasm-opt` on components (PR #98373)
mh4ck-Thales wrote: Seems like bash is present on the GitHub Windows runners. I shouldn't be too hard to create a simple bash script that work both on Windows and Linux. It would add a new dependency for the test suite on Windows however (if bash is not already a dependency for Windows) https://github.com/llvm/llvm-project/pull/98373 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [WebAssembly] Disable running `wasm-opt` on components (PR #98373)
mh4ck-Thales wrote: @alexcrichton sorry I thought that binaryen was under the bytecode alliance and not the webassembly org. I opened the issue here : https://github.com/WebAssembly/binaryen/issues/6728 , and proposed your idea for a solution. https://github.com/llvm/llvm-project/pull/98373 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
