New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LLVM is compiled without optimization (OpenBSD issue) #39900

Open
semarie opened this Issue Feb 17, 2017 · 4 comments

Comments

Projects
None yet
4 participants
@semarie
Contributor

semarie commented Feb 17, 2017

At OpenBSD, for building lang/rust package, we switched from using standard LLVM version (3.9.1) to embedded LLVM version, in order to be able to update LLVM at 4.0.0.

The switch has a big performance impact: the build of rust passed from approx 2h (build with linking to external LLVM) to 12h (with 2h for building LLVM itself). And every program built with rustc had a similar performance impact (build time for cargo raise by a factor of 8).

I passed lot of time to found the issue (comparing source tree between stock LLVM-3.9.1 and src/llvm, reviewing cmake options and finally comparing compiler command-line). It appeared that LLVM embedded version is build without optimization (no -O2 in CFLAGS).

As workaround, I removed the filter out of -O options from bootstrap code, and the build time come back to something more coherent: only 4h (with 2h for LLVM itself - so it is similar to what we had before).

In order to check the LLVM comand-line at compilation, I also added cfg.define("CMAKE_VERBOSE_MAKEFILE", "ON"); to src/bootstrap/native.rs.

I dunno if the issue is only on OpenBSD or could be found on other platform. Maybe Linux adds some optimization by default on the compiler ? or it is LLVM that doesn't enable same optimizations set depending the platform ?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Feb 17, 2017

Member

Odd! AFAIK this doesn't plague the current suite of platforms as I have to imagine we would have noticed this by now. Setting the optimization though is in theory CMake's job...

Member

alexcrichton commented Feb 17, 2017

Odd! AFAIK this doesn't plague the current suite of platforms as I have to imagine we would have noticed this by now. Setting the optimization though is in theory CMake's job...

@semarie

This comment has been minimized.

Show comment
Hide comment
@semarie

semarie Feb 17, 2017

Contributor

@alexcrichton ok, I think I found it: under OpenBSD we try manage ourself optimization level. In fact, it is common that some project want -O3 (it is the default for RELEASE for cmake), but these optimization steps may actually generate incorrect code (if i386/amd64 are generally well tested, it isn't the case for all platforms we support at OpenBSD).

So we have patched cmake to not add default optimization flag, but let the user the responsibility to do it. As rustbuild filter out any optimization flag, it results LLVM to be compiled without any optimization, even if we asked rustbuild to compile C-program using it (CFLAGS had -O2).

For me, rustbuild shouldn't try to interfere with CFLAGS (a user decision). What are the drawbacks to not honor CFLAGS ? The comment on src/bootstrap/lib.rs only said build scripts knows better than user what to do:

// Filter out -O and /O (the optimization flags) that we picked up from
// gcc-rs because the build scripts will determine that for themselves.
let mut base = self.cc[target].0.args().iter()
                           .map(|s| s.to_string_lossy().into_owned())
                           .filter(|s| !s.starts_with("-O") && !s.starts_with("/O"))
                           .collect::<Vec<_>>();

Please note I don't request to change this part: I could live with a local patch in lang/rust port for OpenBSD distribution. But I would like understand the rational behind.

Contributor

semarie commented Feb 17, 2017

@alexcrichton ok, I think I found it: under OpenBSD we try manage ourself optimization level. In fact, it is common that some project want -O3 (it is the default for RELEASE for cmake), but these optimization steps may actually generate incorrect code (if i386/amd64 are generally well tested, it isn't the case for all platforms we support at OpenBSD).

So we have patched cmake to not add default optimization flag, but let the user the responsibility to do it. As rustbuild filter out any optimization flag, it results LLVM to be compiled without any optimization, even if we asked rustbuild to compile C-program using it (CFLAGS had -O2).

For me, rustbuild shouldn't try to interfere with CFLAGS (a user decision). What are the drawbacks to not honor CFLAGS ? The comment on src/bootstrap/lib.rs only said build scripts knows better than user what to do:

// Filter out -O and /O (the optimization flags) that we picked up from
// gcc-rs because the build scripts will determine that for themselves.
let mut base = self.cc[target].0.args().iter()
                           .map(|s| s.to_string_lossy().into_owned())
                           .filter(|s| !s.starts_with("-O") && !s.starts_with("/O"))
                           .collect::<Vec<_>>();

Please note I don't request to change this part: I could live with a local patch in lang/rust port for OpenBSD distribution. But I would like understand the rational behind.

@semarie semarie changed the title from LLVM is compiled without optimization to LLVM is compiled without optimization (OpenBSD issue) Feb 17, 2017

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Feb 17, 2017

Member

Ah yeah that would cause it. That snippet is there because gcc-rs will unconditionally include the opt-level in the cflags returned but don't want the default opt-level there, we'd rather use the one that LLVM assigns. The fix here would be to add a flag to have gcc-rs not pass the default opt level (in gcc-rs itself), pass that flag in rustbuild, and then remove that logic.

Member

alexcrichton commented Feb 17, 2017

Ah yeah that would cause it. That snippet is there because gcc-rs will unconditionally include the opt-level in the cflags returned but don't want the default opt-level there, we'd rather use the one that LLVM assigns. The fix here would be to add a flag to have gcc-rs not pass the default opt level (in gcc-rs itself), pass that flag in rustbuild, and then remove that logic.

bob-beck pushed a commit to openbsd/ports that referenced this issue Feb 18, 2017

Divide build time by at least three and various fixes, all from semarie@
- stop pruning optimizations coming from the environment (ie OpenBSD's
  default -O2 -pipe) when building llvm - this resulted in a very slow
llvm, and in a veeery slow rust build, and an awfully slow rust
compiler. Yay. See rust-lang/rust#39900
- only add cmake to BDEP when rustc is compiled with bundled llvm
- propagate verbose cmake flag to bundled llvm build

tested on i386 and amd64
@Mark-Simulacrum

This comment has been minimized.

Show comment
Hide comment
@Mark-Simulacrum

Mark-Simulacrum May 23, 2017

Member

So this needs two parts: One for gcc-rs, probably changing something around here and in rustbuild within this repo updating us to use those flags.

Member

Mark-Simulacrum commented May 23, 2017

So this needs two parts: One for gcc-rs, probably changing something around here and in rustbuild within this repo updating us to use those flags.

@Mark-Simulacrum Mark-Simulacrum added this to the impl period milestone Sep 15, 2017

@aturon aturon removed this from the impl period milestone Sep 15, 2017

hakrdinesh pushed a commit to hakrtech/openbsd-ports0-test that referenced this issue Jan 16, 2018

Divide build time by at least three and various fixes, all from semarie@
- stop pruning optimizations coming from the environment (ie OpenBSD's
  default -O2 -pipe) when building llvm - this resulted in a very slow
llvm, and in a veeery slow rust build, and an awfully slow rust
compiler. Yay. See rust-lang/rust#39900
- only add cmake to BDEP when rustc is compiled with bundled llvm
- propagate verbose cmake flag to bundled llvm build

tested on i386 and amd64
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment