Add flag to opt out of wasm-opt

[mh4ck-Thales]

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.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Quentin Michaud (mh4ck-Thales)

Changes

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.

---

Full diff: https://github.com/llvm/llvm-project/pull/95208.diff

3 Files Affected:

• (modified) clang/include/clang/Basic/LangOptions.h (+4)
• (modified) clang/include/clang/Driver/Options.td (+8)
• (modified) clang/lib/Driver/ToolChains/WebAssembly.cpp (+37-35)

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<dxc_Group>,
   HelpText<"Specify the target environment">,
   Values<"vulkan1.2, vulkan1.3">;
+def no_wasm_opt : Flag<["--"], "no-wasm-opt">,
+  Group<m_Group>,
+  HelpText<"Disable the wasm-opt optimizer">,
+  MarshallingInfoFlag<LangOpts<"NoWasmOpt">>;
+def wasm_opt : Flag<["--"], "wasm-opt">,
+  Group<m_Group>,
+  HelpText<"Enable the wasm-opt optimizer (default)">,
+  MarshallingInfoNegativeFlag<LangOpts<"NoWasmOpt">>;
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<Command>(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<Command>(
-            JA, *this, ResponseFileSupport::AtFileCurCP(), WasmOpt, OptArgs,
-            Inputs, Output));
+      CmdArgs.push_back("--keep-section=target_features");
+    }
+
+    C.addCommand(std::make_unique<Command>(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<Command>(
+              JA, *this, ResponseFileSupport::AtFileCurCP(), WasmOpt, OptArgs,
+              Inputs, Output));
+        }
       }
     }
   }

[sunfishcode]

This looks good to me.

[mh4ck-Thales]

Any opinion on whether to implement a warning or not ?

[github-actions]

@mh4ck-Thales Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

[sbc100]

I wonder if there is some why to avoiding adding this new option globally like this and instead limit it in scope the wasm target in some way?

[sunfishcode]

@mh4ck-Thales A warning could be useful in some situations, but I also don't know how to navigate the situation with existing users using -Werror.

@sbc100 I'm open to ideas for how to limit the scope to wasm in some way.

[pavelsavara]

I don't know who needs to hear this, but I got bitten by wasm-opt being on my PATH (while using WASI SDK 22).

I was trying to compile zlib-ng for WASIp2 and it's CMake platform detection was passing -O3 to check_type_size("void *" SIZEOF_DATA_PTR)
It worked on some machines and it broke on others. Those which had wasm-opt on PATH.

Because in WASIp2 the LLVM produces WASM component not WASM module.
The wasm-opt doesn't know how to read the component binary and fails with parse exception: surprising value (at 0:8)

When that happens during CMake detetection, that problem is not surfaced and you are looking at

-- Check size of void *
-- sizeof(void *) is  bytes
  CMake Error at C:/Dev/runtime/src/native/external/zlib-ng/CMakeLists.txt:485 (message):
    sizeof(void *) is neither 32 nor 64 bit

Until now, I was naive user of CMake.

• Maybe the --no-wasm-opt should be the default for WASM components
  • because not optimizing components is not breaking change, I guess.
• Maybe the --no-wasm-opt should be the default everywhere
• Maybe LLVM should check wasm-opt version or compatibility first ?
• Maybe wasm-component-ld should pass the module to wasm-opt before it makes component out of it ?
• Or maybe wasm-opt needs to learn how to unpack component, optimize the modules and pack it back ?
  • if wasm-opt is expected on components too

Should I open new issue for this ? Which of it and where ?

[sbc100]

I think should never not be running wasm-opt when the linker is wasm-component-ld. Thats not going to work, at least not any time soon.

How to integrate wasm-opt into wasm-component-ld is a good question. I imagine @alexcrichton has some ideas in that area.

[alexcrichton]

Ah yes that's correct, for wasm32-wasip2 the wasm-opt step should be unconditionally skipped for now. It in theory wouldn't be too hard to add support for components with wasm-opt as it's pretty easy to iterate over a binary and find the core modules within it, but for the time being I think it's best to just skip.

[alexcrichton]

I've opened #98373 to avoid running wasm-opt for the wasm32-wasip2 target by default. It also fixes a minor issue with this PR where if wasm-opt were disabled it would skip running the linker entirely.

[mh4ck-Thales]

@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.

[sunfishcode]

@mh4ck-Thales If I read the release schedule right, this flag hasn't made it into a release yet, so we still have time to make changes without having to deprecate anything.

[mh4ck-Thales]

@sunfishcode from the release schedule these changes should be picked up to release/19.x today? Do you think we still have the time to add a warning?

[pavelsavara]

I would also love to see #98373 merged. Thank you.

[sunfishcode]

@mh4ck-Thales Yes; even if we don't get it in before the release branch is created, I think it'd be reasonable to backport a patch to the release branch.