sunfish marked an inline comment as done.
sunfish added inline comments.
================
Comment at: clang/lib/Driver/ToolChains/WebAssembly.cpp:96
+ if (Arg *A = Args.getLastArg(options::OPT_O_Group)) {
+ if (const char *WasmOptPath = getenv("WASM_OPT")) {
+ StringRef OOpt = "s";
----------------
dschuff wrote:
> sunfish wrote:
> > dschuff wrote:
> > > sunfish wrote:
> > > > dschuff wrote:
> > > > > sunfish wrote:
> > > > > > dschuff wrote:
> > > > > > > What would you think about adding a way to pass arguments through
> > > > > > > to wasm-opt on the command line, like we have for the linker,
> > > > > > > assembler, etc? Something like `-Wo,-O2` (or `-Ww` or whatever;
> > > > > > > analogous to `-Wl` and `-Wa`). That seems nicer than an env var,
> > > > > > > although it doesn't solve the problem of controlling whether to
> > > > > > > run the optimizer in the first place.
> > > > > > My guess here is that we don't need clang to have an option for
> > > > > > passing additional flags -- if people want to do extra special
> > > > > > things with wasm-opt on clang's output they can just run wasm-opt
> > > > > > directly themselves. Does that sound reasonable?
> > > > > Maybe. But I still don't like the use of an env var for this kind of
> > > > > behavior-effecting (i.e. non-debugging) use case. It's hard enough
> > > > > to get reproducible and hermetic build behavior as it is, I
> > > > > definitely wouldn't want to worry about the environment affecting the
> > > > > output in addition to all the random flags, included files, etc.
> > > > If we did -Wo,-O2 or so, we'd still need to be able to find the
> > > > wasm-opt program to be able to run it. We could just search for it in
> > > > PATH, but that's also a little dicey from a hermetic build perspective.
> > > >
> > > > I borrowed "WASM_OPT" from
> > > > [cargo-wasi](https://github.com/bytecodealliance/cargo-wasi). I'm also
> > > > not a fan of environment variables in general, but this way does have
> > > > the advantage that people can set it once, and not have to modify their
> > > > Makefiles to add new flags. Users can think of it as just being part of
> > > > -O2 and friends.
> > > >
> > > What's the usual way to locate things like external assemblers?
> > > Presumably we could use the same mechanism for wasm-opt?
> > It checks the COMPILER_PATH environment variable and -B command-line flags,
> > which I'm not sure we should use here, but it looks like it does fall back
> > to checking PATH at the end.
> >
> > So, what would you think of just checking PATH for wasm-opt?
> I suspect we'll end up with -B flags if/when people start building
> interesting or nontrivial toolchains with clang (or we try to make emscripten
> more standardish), but I'm fine with leaving that out for now. Checking PATH
> for wasm-opt seems fine to me to locate the binary. Did you have that in mind
> as also the way to determine whether or not to run wasm-opt (i.e. run if it's
> there, don't if it's not)? That seems slightly error-prone in the sense that
> there would be a silent behavior change if something went wrong (e.g.
> wasm-opt goes missing) but in a world where most clang users are using
> wasm-opt then using wasm-opt by default seems reasonable; so that seems fine
> as a way to start out.
>
> I do think we will eventually want some way to modify the behavior of
> wasm-opt though. For that matter wasm-opt might at some point become able to
> optimize object files (allowing faster links at the cost of less
> LTO-optimized binaries; we'd run a reduced set of passes post-link or none at
> all). In that case there'd also have to be further changes if we want builtin
> support for that.
Yes, we'd just run wasm-opt if it's in the PATH. See the now updated patch. And
yeah, this means if your wasm-opt disappears, you silently won't get as much
optimization, which is a little unfortunate, but it is the most convenient for
the common use case.
And yeah, we can always add more options in the future :-).
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
