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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
