[WebAssembly] Disable running `wasm-opt` on components #98373

alexcrichton · 2024-07-10T19:55:07Z

This commit adds a check that disables wasm-opt for the wasm32-wasip2 target because wasm-opt doesn't support components at this time. This also fixes a minor issue from #95208 where if wasm-opt was disabled then the linker wouldn't run at all.

This commit adds a check that disables `wasm-opt` for the `wasm32-wasip2` target because `wasm-opt` doesn't support components at this time. This also fixes a minor issue from llvm#95208 where if `wasm-opt` was disabled then the linker wouldn't run at all.

llvmbot · 2024-07-10T19:55:42Z

@llvm/pr-subscribers-clang

Author: Alex Crichton (alexcrichton)

Changes

This commit adds a check that disables wasm-opt for the wasm32-wasip2 target because wasm-opt doesn't support components at this time. This also fixes a minor issue from #95208 where if wasm-opt was disabled then the linker wouldn't run at all.

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

1 Files Affected:

(modified) clang/lib/Driver/ToolChains/WebAssembly.cpp (+47-38)

diff --git a/clang/lib/Driver/ToolChains/WebAssembly.cpp b/clang/lib/Driver/ToolChains/WebAssembly.cpp
index 60bd97e0ee987..2753645f4700f 100644
--- a/clang/lib/Driver/ToolChains/WebAssembly.cpp
+++ b/clang/lib/Driver/ToolChains/WebAssembly.cpp
@@ -61,6 +61,10 @@ std::string wasm::Linker::getLinkerPath(const ArgList &Args) const {
   return ToolChain.GetProgramPath(ToolChain.getDefaultLinker());
 }
 
+static bool IsWasip2(const llvm::Triple &TargetTriple) {
+  return TargetTriple.getOSName() == "wasip2";
+}
+
 void wasm::Linker::ConstructJob(Compilation &C, const JobAction &JA,
                                 const InputInfo &Output,
                                 const InputInfoList &Inputs,
@@ -158,46 +162,51 @@ void wasm::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   CmdArgs.push_back("-o");
   CmdArgs.push_back(Output.getFilename());
 
-  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 = {};
-      }
+  // Don't use wasm-opt by default on `wasip2` as it doesn't have support for
+  // components at this time. Retain the historical default otherwise, though,
+  // of running `wasm-opt` by default.
+  bool WasmOptDefault = !IsWasip2(ToolChain.getTriple());
+  bool RunWasmOpt = Args.hasFlag(options::OPT_wasm_opt, options::OPT_no_wasm_opt, WasmOptDefault);
+
+  // 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;
+  if (RunWasmOpt && 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");
-    }
+  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));
-        }
+  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));
       }
     }
   }
@@ -241,7 +250,7 @@ WebAssembly::WebAssembly(const Driver &D, const llvm::Triple &Triple,
 }
 
 const char *WebAssembly::getDefaultLinker() const {
-  if (getOS() == "wasip2")
+  if (IsWasip2(getTriple()))
     return "wasm-component-ld";
   return "wasm-ld";
 }

llvmbot · 2024-07-10T19:55:42Z

@llvm/pr-subscribers-clang-driver

Author: Alex Crichton (alexcrichton)

Changes

This commit adds a check that disables wasm-opt for the wasm32-wasip2 target because wasm-opt doesn't support components at this time. This also fixes a minor issue from #95208 where if wasm-opt was disabled then the linker wouldn't run at all.

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

1 Files Affected:

(modified) clang/lib/Driver/ToolChains/WebAssembly.cpp (+47-38)

diff --git a/clang/lib/Driver/ToolChains/WebAssembly.cpp b/clang/lib/Driver/ToolChains/WebAssembly.cpp
index 60bd97e0ee987..2753645f4700f 100644
--- a/clang/lib/Driver/ToolChains/WebAssembly.cpp
+++ b/clang/lib/Driver/ToolChains/WebAssembly.cpp
@@ -61,6 +61,10 @@ std::string wasm::Linker::getLinkerPath(const ArgList &Args) const {
   return ToolChain.GetProgramPath(ToolChain.getDefaultLinker());
 }
 
+static bool IsWasip2(const llvm::Triple &TargetTriple) {
+  return TargetTriple.getOSName() == "wasip2";
+}
+
 void wasm::Linker::ConstructJob(Compilation &C, const JobAction &JA,
                                 const InputInfo &Output,
                                 const InputInfoList &Inputs,
@@ -158,46 +162,51 @@ void wasm::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   CmdArgs.push_back("-o");
   CmdArgs.push_back(Output.getFilename());
 
-  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 = {};
-      }
+  // Don't use wasm-opt by default on `wasip2` as it doesn't have support for
+  // components at this time. Retain the historical default otherwise, though,
+  // of running `wasm-opt` by default.
+  bool WasmOptDefault = !IsWasip2(ToolChain.getTriple());
+  bool RunWasmOpt = Args.hasFlag(options::OPT_wasm_opt, options::OPT_no_wasm_opt, WasmOptDefault);
+
+  // 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;
+  if (RunWasmOpt && 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");
-    }
+  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));
-        }
+  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));
       }
     }
   }
@@ -241,7 +250,7 @@ WebAssembly::WebAssembly(const Driver &D, const llvm::Triple &Triple,
 }
 
 const char *WebAssembly::getDefaultLinker() const {
-  if (getOS() == "wasip2")
+  if (IsWasip2(getTriple()))
     return "wasm-component-ld";
   return "wasm-ld";
 }

github-actions · 2024-07-10T19:59:02Z

✅ With the latest revision this PR passed the C/C++ code formatter.

pavelsavara · 2024-07-10T20:04:14Z

clang/lib/Driver/ToolChains/WebAssembly.cpp

@@ -61,6 +61,10 @@ std::string wasm::Linker::getLinkerPath(const ArgList &Args) const {
  return ToolChain.GetProgramPath(ToolChain.getDefaultLinker());
 }

+static bool IsWasip2(const llvm::Triple &TargetTriple) {
+  return TargetTriple.getOSName() == "wasip2";


I guess WASI preview3 and the future will be the same. Should we invert the test and only check for wasip1 ?

Sounds reasonable to me yeah 👍

sbc100

Can we write a test for this?

alexcrichton · 2024-07-10T20:24:56Z

Is there a way to "fake" a wasm-opt executable? I didn't find any existing tests for the wasm-opt integration and it's auto-disabled if wasm-opt isn't found on $PATH, which doesn't happen by default in the test suite, so unless I can set up wasm-opt to show up in $PATH as part of the test I'm not sure how to test

sbc100 · 2024-07-10T20:35:12Z

Is there a way to "fake" a wasm-opt executable? I didn't find any existing tests for the wasm-opt integration and it's auto-disabled if wasm-opt isn't found on $PATH, which doesn't happen by default in the test suite, so unless I can set up wasm-opt to show up in $PATH as part of the test I'm not sure how to test

Ah.. I guess if there are no existing tests then maybe this is something that is particularly hard to test.

mh4ck-Thales · 2024-07-11T07:10:07Z

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?

pavelsavara · 2024-07-11T08:07:27Z

could shell script named wasm-opt on PATH and doing nothing do the trick ?
Or even better it could write it's arguments into some file, so that they could be asserted in the test.

2c

mh4ck-Thales · 2024-07-11T08:54:54Z

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.

alexcrichton · 2024-07-11T14:27:38Z

Ah yeah unfortunately I think it still comes back to how to do all this in the test harness. If there are examples of this elsewhere I'd be happy to investigate and copy. I'm also not sure if shell scripts would work for Windows...

@sbc100 if you're ok with this without a test though would you be able to merge for me?

alexcrichton · 2024-07-11T14:28:14Z

would it be relevant to open an issue in the binaryen repo for the support of Wasm components?

Oh sorry forgot to respond to this as well. I'm not personally part of the binaryen repo but I suspect they would not object to having an issue to track this.

sbc100 · 2024-07-11T15:12:45Z

I think the problem with creating a dummy shell script is that it would need to work on windows too.. I guess it would be better to have test that run on some platforms that no test at all.

In any case, I'm OK with having this land as-is for now, since this is feature that is already untested.

mh4ck-Thales · 2024-07-11T15:50:45Z

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)

mh4ck-Thales · 2024-07-11T16:44:51Z

@alexcrichton sorry I thought that binaryen was under the bytecode alliance and not the webassembly org. I opened the issue here : WebAssembly/binaryen#6728 , and proposed your idea for a solution.

alexcrichton · 2024-07-12T19:27:44Z

@sbc100 oh I can't merge things myself so if you'd be ok merging for me that'd be appreciated

alexcrichton · 2024-07-19T14:52:38Z

ping @sbc100 would you be able to merge this for me?

Summary: This commit adds a check that disables `wasm-opt` for the `wasm32-wasip2` target because `wasm-opt` doesn't support components at this time. This also fixes a minor issue from #95208 where if `wasm-opt` was disabled then the linker wouldn't run at all. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251310

llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Jul 10, 2024

alexcrichton mentioned this pull request Jul 10, 2024

Add flag to opt out of wasm-opt #95208

Merged

pavelsavara reviewed Jul 10, 2024

View reviewed changes

alexcrichton added 2 commits July 10, 2024 13:09

Run clang-format

cb76384

Explicitly exclude wasip1 instead of including wasip2

92a3ba6

sbc100 approved these changes Jul 10, 2024

View reviewed changes

Fix formatting

1f27f6a

sbc100 approved these changes Jul 11, 2024

View reviewed changes

mh4ck-Thales mentioned this pull request Jul 11, 2024

Add support for Wasm components in wasm-opt WebAssembly/binaryen#6728

Open

pavelsavara mentioned this pull request Jul 23, 2024

[wasi] upgrade to WASI SDK with LLVM 19 dotnet/runtime#104773

Closed

5 tasks

sunfishcode approved these changes Jul 23, 2024

View reviewed changes

sunfishcode merged commit b4ebf2d into llvm:main Jul 23, 2024
7 checks passed

mh4ck-Thales mentioned this pull request Jul 24, 2024

Add wasm-opt warning #100321

Open

alexcrichton deleted the disable-wasm-opt-on-wasip2 branch July 29, 2024 16:10

alexcrichton mentioned this pull request Sep 5, 2024

Tests don't build locally WebAssembly/wasi-sdk#485

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WebAssembly] Disable running `wasm-opt` on components #98373

[WebAssembly] Disable running `wasm-opt` on components #98373

alexcrichton commented Jul 10, 2024

llvmbot commented Jul 10, 2024

llvmbot commented Jul 10, 2024

github-actions bot commented Jul 10, 2024 •

edited

Loading

pavelsavara Jul 10, 2024

alexcrichton Jul 10, 2024

sbc100 left a comment

alexcrichton commented Jul 10, 2024

sbc100 commented Jul 10, 2024

mh4ck-Thales commented Jul 11, 2024

pavelsavara commented Jul 11, 2024

mh4ck-Thales commented Jul 11, 2024

alexcrichton commented Jul 11, 2024

alexcrichton commented Jul 11, 2024

sbc100 commented Jul 11, 2024

mh4ck-Thales commented Jul 11, 2024

mh4ck-Thales commented Jul 11, 2024

alexcrichton commented Jul 12, 2024

alexcrichton commented Jul 19, 2024

[WebAssembly] Disable running wasm-opt on components #98373

[WebAssembly] Disable running wasm-opt on components #98373

Conversation

alexcrichton commented Jul 10, 2024

llvmbot commented Jul 10, 2024

llvmbot commented Jul 10, 2024

github-actions bot commented Jul 10, 2024 • edited Loading

pavelsavara Jul 10, 2024

Choose a reason for hiding this comment

alexcrichton Jul 10, 2024

Choose a reason for hiding this comment

sbc100 left a comment

Choose a reason for hiding this comment

alexcrichton commented Jul 10, 2024

sbc100 commented Jul 10, 2024

mh4ck-Thales commented Jul 11, 2024

pavelsavara commented Jul 11, 2024

mh4ck-Thales commented Jul 11, 2024

alexcrichton commented Jul 11, 2024

alexcrichton commented Jul 11, 2024

sbc100 commented Jul 11, 2024

mh4ck-Thales commented Jul 11, 2024

mh4ck-Thales commented Jul 11, 2024

alexcrichton commented Jul 12, 2024

alexcrichton commented Jul 19, 2024

[WebAssembly] Disable running `wasm-opt` on components #98373

[WebAssembly] Disable running `wasm-opt` on components #98373

github-actions bot commented Jul 10, 2024 •

edited

Loading