[PATCH] D70500: [WebAssembly] Enable use of wasm-opt and LTO-enabled system libraries

[Dan Gohman via Phabricator via cfe-commits]

sunfish marked 2 inline comments as done.
sunfish added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/WebAssembly.cpp:105
+        OOpt = "0";
+      else if (A->getOption().matches(options::OPT_O))
+        OOpt = A->getValue();
----------------
dschuff wrote:
> This chain is slightly confusing. I assume this `getValue()` captures the number in `-O3`, `-O2`, etc? But why then do we need special-casing for 0 and 4 above?
> 
> For that matter, we should probably not run wasm-opt at all if the opt-level is 0, right?
This isn't a wasm thing; O0 and O4 [are special](https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Driver/Options.td#L393). See also [here](https://github.com/llvm/llvm-project/blob/master/clang/lib/Driver/ToolChains/Cuda.cpp#L368) and [here](https://github.com/llvm/llvm-project/blob/master/clang/lib/Driver/ToolChains/HIP.cpp#L97) for other code which does similar things. The wasm-opt version here is actually simpler because we don't need to special-case Os or Oz.

The `if (Arg *A = Args.getLastArg(options::OPT_O_Group)) {` guards against the case where no -O option is given, but you're right, we shouldn't run wasm-opt if -O0 is given. I'll update the patch.


================
Comment at: clang/test/Driver/wasm-toolchain-lto.c:6
+// LINK_OPT_KNOWN: clang{{.*}}" "-cc1" {{.*}} "-o" "[[temp:[^"]*]]"
+// LINK_OPT_KNOWN: wasm-ld{{.*}}" "-L/foo/lib/wasm32-wasi/llvm-lto/
----------------
sbc100 wrote:
> Include the final path segment in the expectation?
It's a git revision, so it'd be constantly changing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70500/new/

https://reviews.llvm.org/D70500