From e943426045361045286c6be55c349381b5506f0b Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Wed, 22 May 2019 12:58:56 +0200 Subject: [PATCH 1/6] Use a PathBuf instead of String for representing the pgo-use path internally. --- src/librustc/session/config.rs | 4 ++-- src/librustc_codegen_llvm/back/write.rs | 8 +++----- src/librustc_codegen_ssa/back/write.rs | 4 ++-- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/librustc/session/config.rs b/src/librustc/session/config.rs index 49cd3eff21a..0aed1bf827d 100644 --- a/src/librustc/session/config.rs +++ b/src/librustc/session/config.rs @@ -1381,7 +1381,7 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options, "insert profiling code"), pgo_gen: PgoGenerate = (PgoGenerate::Disabled, parse_pgo_generate, [TRACKED], "Generate PGO profile data, to a given file, or to the default location if it's empty."), - pgo_use: String = (String::new(), parse_string, [TRACKED], + pgo_use: Option = (None, parse_opt_pathbuf, [TRACKED], "Use PGO profile data from the given profile file."), disable_instrumentation_preinliner: bool = (false, parse_bool, [TRACKED], "Disable the instrumentation pre-inliner, useful for profiling / PGO."), @@ -2021,7 +2021,7 @@ pub fn build_session_options_and_crate_config( } } - if debugging_opts.pgo_gen.enabled() && !debugging_opts.pgo_use.is_empty() { + if debugging_opts.pgo_gen.enabled() && debugging_opts.pgo_use.is_some() { early_error( error_format, "options `-Z pgo-gen` and `-Z pgo-use` are exclusive", diff --git a/src/librustc_codegen_llvm/back/write.rs b/src/librustc_codegen_llvm/back/write.rs index 66ba95810a6..1eee9ab8c0b 100644 --- a/src/librustc_codegen_llvm/back/write.rs +++ b/src/librustc_codegen_llvm/back/write.rs @@ -721,11 +721,9 @@ pub unsafe fn with_llvm_pmb(llmod: &llvm::Module, } }; - let pgo_use_path = if config.pgo_use.is_empty() { - None - } else { - Some(CString::new(config.pgo_use.as_bytes()).unwrap()) - }; + let pgo_use_path = config.pgo_use.as_ref().map(|path_buf| { + CString::new(path_buf.to_string_lossy().as_bytes()).unwrap() + }); llvm::LLVMRustConfigurePassManagerBuilder( builder, diff --git a/src/librustc_codegen_ssa/back/write.rs b/src/librustc_codegen_ssa/back/write.rs index 1c793996c83..74c41969268 100644 --- a/src/librustc_codegen_ssa/back/write.rs +++ b/src/librustc_codegen_ssa/back/write.rs @@ -57,7 +57,7 @@ pub struct ModuleConfig { pub opt_size: Option, pub pgo_gen: PgoGenerate, - pub pgo_use: String, + pub pgo_use: Option, // Flags indicating which outputs to produce. pub emit_pre_lto_bc: bool, @@ -95,7 +95,7 @@ impl ModuleConfig { opt_size: None, pgo_gen: PgoGenerate::Disabled, - pgo_use: String::new(), + pgo_use: None, emit_no_opt_bc: false, emit_pre_lto_bc: false, From eeb7348dc3fbb0b85a7f45b5972e744085cb632d Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Wed, 22 May 2019 13:00:09 +0200 Subject: [PATCH 2/6] PGO: Check that pgo-use file actually exists. LLVM seems to only emit an easy-to-overlook warning otherwise. --- src/librustc/session/mod.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/librustc/session/mod.rs b/src/librustc/session/mod.rs index 4d47491661e..3d8092f6e00 100644 --- a/src/librustc/session/mod.rs +++ b/src/librustc/session/mod.rs @@ -1272,6 +1272,15 @@ fn validate_commandline_args_with_session_available(sess: &Session) { sess.err("Linker plugin based LTO is not supported together with \ `-C prefer-dynamic` when targeting MSVC"); } + + // Make sure that any given profiling data actually exists so LLVM can't + // decide to silently skip PGO. + if let Some(ref path) = sess.opts.debugging_opts.pgo_use { + if !path.exists() { + sess.err(&format!("File `{}` passed to `-Zpgo-use` does not exist.", + path.display())); + } + } } /// Hash value constructed out of all the `-C metadata` arguments passed to the From 30a3fad31614496e902289f88e78c8e9cab6af4f Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Wed, 22 May 2019 13:01:12 +0200 Subject: [PATCH 3/6] rustbuild: Also build compiler-rt when building LLDB. This allows clang-based run-make tests to use PGO. --- src/bootstrap/native.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bootstrap/native.rs b/src/bootstrap/native.rs index da2e03a1a08..b89cd4981a7 100644 --- a/src/bootstrap/native.rs +++ b/src/bootstrap/native.rs @@ -204,7 +204,7 @@ impl Step for Llvm { } if want_lldb { - cfg.define("LLVM_ENABLE_PROJECTS", "clang;lldb"); + cfg.define("LLVM_ENABLE_PROJECTS", "clang;lldb;compiler-rt"); // For the time being, disable code signing. cfg.define("LLDB_CODESIGN_IDENTITY", ""); cfg.define("LLDB_NO_DEBUGSERVER", "ON"); From 1de93a78147126dcb864e070734532a02dde56cc Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Mon, 20 May 2019 11:43:38 +0200 Subject: [PATCH 4/6] Add a smoketest for combining PGO with xLTO. --- .../cross-lang-lto-pgo-smoketest/Makefile | 87 +++++++++++++++++++ .../cross-lang-lto-pgo-smoketest/clib.c | 9 ++ .../cross-lang-lto-pgo-smoketest/cmain.c | 12 +++ .../cross-lang-lto-pgo-smoketest/main.rs | 11 +++ .../cross-lang-lto-pgo-smoketest/rustlib.rs | 12 +++ 5 files changed, 131 insertions(+) create mode 100644 src/test/run-make-fulldeps/cross-lang-lto-pgo-smoketest/Makefile create mode 100644 src/test/run-make-fulldeps/cross-lang-lto-pgo-smoketest/clib.c create mode 100644 src/test/run-make-fulldeps/cross-lang-lto-pgo-smoketest/cmain.c create mode 100644 src/test/run-make-fulldeps/cross-lang-lto-pgo-smoketest/main.rs create mode 100644 src/test/run-make-fulldeps/cross-lang-lto-pgo-smoketest/rustlib.rs diff --git a/src/test/run-make-fulldeps/cross-lang-lto-pgo-smoketest/Makefile b/src/test/run-make-fulldeps/cross-lang-lto-pgo-smoketest/Makefile new file mode 100644 index 00000000000..59a7d61892f --- /dev/null +++ b/src/test/run-make-fulldeps/cross-lang-lto-pgo-smoketest/Makefile @@ -0,0 +1,87 @@ +# needs-matching-clang + +# This test makes sure that cross-language inlining can be used in conjunction +# with profile-guided optimization. The test only tests that the whole workflow +# can be executed without anything crashing. It does not test whether PGO or +# xLTO have any specific effect on the generated code. + +-include ../tools.mk + +COMMON_FLAGS=-Copt-level=3 -Ccodegen-units=1 + +# LLVM doesn't support instrumenting binaries that use SEH: +# https://bugs.llvm.org/show_bug.cgi?id=41279 +# +# Things work fine with -Cpanic=abort though. +ifdef IS_MSVC +COMMON_FLAGS+= -Cpanic=abort +endif + +all: cpp-executable rust-executable + +cpp-executable: + $(RUSTC) -Clinker-plugin-lto=on \ + -Zpgo-gen="$(TMPDIR)"/cpp-profdata \ + -o "$(TMPDIR)"/librustlib-xlto.a \ + $(COMMON_FLAGS) \ + ./rustlib.rs + $(CLANG) -flto=thin \ + -fprofile-generate="$(TMPDIR)"/cpp-profdata \ + -fuse-ld=lld \ + -L "$(TMPDIR)" \ + -lrustlib-xlto \ + -o "$(TMPDIR)"/cmain \ + -O3 \ + ./cmain.c + $(TMPDIR)/cmain + # Postprocess the profiling data so it can be used by the compiler + "$(LLVM_BIN_DIR)"/llvm-profdata merge \ + -o "$(TMPDIR)"/cpp-profdata/merged.profdata \ + "$(TMPDIR)"/cpp-profdata/default_*.profraw + $(RUSTC) -Clinker-plugin-lto=on \ + -Zpgo-use="$(TMPDIR)"/cpp-profdata/merged.profdata \ + -o "$(TMPDIR)"/librustlib-xlto.a \ + $(COMMON_FLAGS) \ + ./rustlib.rs + $(CLANG) -flto=thin \ + -fprofile-use="$(TMPDIR)"/cpp-profdata/merged.profdata \ + -fuse-ld=lld \ + -L "$(TMPDIR)" \ + -lrustlib-xlto \ + -o "$(TMPDIR)"/cmain \ + -O3 \ + ./cmain.c + +rust-executable: + exit + $(CLANG) ./clib.c -fprofile-generate="$(TMPDIR)"/rs-profdata -flto=thin -c -o $(TMPDIR)/clib.o -O3 + (cd $(TMPDIR); $(AR) crus ./libxyz.a ./clib.o) + $(RUSTC) -Clinker-plugin-lto=on \ + -Zpgo-gen="$(TMPDIR)"/rs-profdata \ + -L$(TMPDIR) \ + $(COMMON_FLAGS) \ + -Clinker=$(CLANG) \ + -Clink-arg=-fuse-ld=lld \ + -o $(TMPDIR)/rsmain \ + ./main.rs + $(TMPDIR)/rsmain + # Postprocess the profiling data so it can be used by the compiler + "$(LLVM_BIN_DIR)"/llvm-profdata merge \ + -o "$(TMPDIR)"/rs-profdata/merged.profdata \ + "$(TMPDIR)"/rs-profdata/default_*.profraw + $(CLANG) ./clib.c \ + -fprofile-use="$(TMPDIR)"/rs-profdata/merged.profdata \ + -flto=thin \ + -c \ + -o $(TMPDIR)/clib.o \ + -O3 + rm "$(TMPDIR)"/libxyz.a + (cd $(TMPDIR); $(AR) crus ./libxyz.a ./clib.o) + $(RUSTC) -Clinker-plugin-lto=on \ + -Zpgo-use="$(TMPDIR)"/rs-profdata/merged.profdata \ + -L$(TMPDIR) \ + $(COMMON_FLAGS) \ + -Clinker=$(CLANG) \ + -Clink-arg=-fuse-ld=lld \ + -o $(TMPDIR)/rsmain \ + ./main.rs diff --git a/src/test/run-make-fulldeps/cross-lang-lto-pgo-smoketest/clib.c b/src/test/run-make-fulldeps/cross-lang-lto-pgo-smoketest/clib.c new file mode 100644 index 00000000000..90f33f24dc4 --- /dev/null +++ b/src/test/run-make-fulldeps/cross-lang-lto-pgo-smoketest/clib.c @@ -0,0 +1,9 @@ +#include + +uint32_t c_always_inlined() { + return 1234; +} + +__attribute__((noinline)) uint32_t c_never_inlined() { + return 12345; +} diff --git a/src/test/run-make-fulldeps/cross-lang-lto-pgo-smoketest/cmain.c b/src/test/run-make-fulldeps/cross-lang-lto-pgo-smoketest/cmain.c new file mode 100644 index 00000000000..e3f24828be3 --- /dev/null +++ b/src/test/run-make-fulldeps/cross-lang-lto-pgo-smoketest/cmain.c @@ -0,0 +1,12 @@ +#include + +// A trivial function defined in Rust, returning a constant value. This should +// always be inlined. +uint32_t rust_always_inlined(); + + +uint32_t rust_never_inlined(); + +int main(int argc, char** argv) { + return (rust_never_inlined() + rust_always_inlined()) * 0; +} diff --git a/src/test/run-make-fulldeps/cross-lang-lto-pgo-smoketest/main.rs b/src/test/run-make-fulldeps/cross-lang-lto-pgo-smoketest/main.rs new file mode 100644 index 00000000000..8129dcb85d9 --- /dev/null +++ b/src/test/run-make-fulldeps/cross-lang-lto-pgo-smoketest/main.rs @@ -0,0 +1,11 @@ +#[link(name = "xyz")] +extern "C" { + fn c_always_inlined() -> u32; + fn c_never_inlined() -> u32; +} + +fn main() { + unsafe { + println!("blub: {}", c_always_inlined() + c_never_inlined()); + } +} diff --git a/src/test/run-make-fulldeps/cross-lang-lto-pgo-smoketest/rustlib.rs b/src/test/run-make-fulldeps/cross-lang-lto-pgo-smoketest/rustlib.rs new file mode 100644 index 00000000000..8a74d74a420 --- /dev/null +++ b/src/test/run-make-fulldeps/cross-lang-lto-pgo-smoketest/rustlib.rs @@ -0,0 +1,12 @@ +#![crate_type="staticlib"] + +#[no_mangle] +pub extern "C" fn rust_always_inlined() -> u32 { + 42 +} + +#[no_mangle] +#[inline(never)] +pub extern "C" fn rust_never_inlined() -> u32 { + 421 +} From 48b9896eebff639f794f2a67532c741eb1e3b79f Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Wed, 22 May 2019 15:41:56 +0200 Subject: [PATCH 5/6] Fix unit test after pgo-use change. --- src/librustc/session/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc/session/config.rs b/src/librustc/session/config.rs index 0aed1bf827d..44b6e036557 100644 --- a/src/librustc/session/config.rs +++ b/src/librustc/session/config.rs @@ -3211,7 +3211,7 @@ mod tests { assert_ne!(reference.dep_tracking_hash(), opts.dep_tracking_hash()); opts = reference.clone(); - opts.debugging_opts.pgo_use = String::from("abc"); + opts.debugging_opts.pgo_use = Some(PathBuf::from("abc")); assert_ne!(reference.dep_tracking_hash(), opts.dep_tracking_hash()); opts = reference.clone(); From 577ea539dc4a264b480404700a2463e657c09c87 Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Mon, 27 May 2019 15:09:26 +0200 Subject: [PATCH 6/6] Only build clang_rt when RUSTBUILD_FORCE_CLANG_BASED_TESTS is set. --- src/bootstrap/native.rs | 16 +++++++++++++++- src/bootstrap/test.rs | 21 +++------------------ src/bootstrap/util.rs | 16 ++++++++++++++++ 3 files changed, 34 insertions(+), 19 deletions(-) diff --git a/src/bootstrap/native.rs b/src/bootstrap/native.rs index b89cd4981a7..bf3601cb312 100644 --- a/src/bootstrap/native.rs +++ b/src/bootstrap/native.rs @@ -203,8 +203,16 @@ impl Step for Llvm { cfg.define("LLVM_BUILD_32_BITS", "ON"); } + let mut enabled_llvm_projects = Vec::new(); + + if util::forcing_clang_based_tests() { + enabled_llvm_projects.push("clang"); + enabled_llvm_projects.push("compiler-rt"); + } + if want_lldb { - cfg.define("LLVM_ENABLE_PROJECTS", "clang;lldb;compiler-rt"); + enabled_llvm_projects.push("clang"); + enabled_llvm_projects.push("lldb"); // For the time being, disable code signing. cfg.define("LLDB_CODESIGN_IDENTITY", ""); cfg.define("LLDB_NO_DEBUGSERVER", "ON"); @@ -214,6 +222,12 @@ impl Step for Llvm { cfg.define("LLVM_ENABLE_LIBXML2", "OFF"); } + if enabled_llvm_projects.len() > 0 { + enabled_llvm_projects.sort(); + enabled_llvm_projects.dedup(); + cfg.define("LLVM_ENABLE_PROJECTS", enabled_llvm_projects.join(";")); + } + if let Some(num_linkers) = builder.config.llvm_link_jobs { if num_linkers > 0 { cfg.define("LLVM_PARALLEL_LINK_JOBS", num_linkers.to_string()); diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index 05b3ce6bc89..9d0aa09f15c 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -1143,24 +1143,9 @@ impl Step for Compiletest { } } - if let Some(var) = env::var_os("RUSTBUILD_FORCE_CLANG_BASED_TESTS") { - match &var.to_string_lossy().to_lowercase()[..] { - "1" | "yes" | "on" => { - assert!(builder.config.lldb_enabled, - "RUSTBUILD_FORCE_CLANG_BASED_TESTS needs Clang/LLDB to \ - be built."); - let clang_exe = builder.llvm_out(target).join("bin").join("clang"); - cmd.arg("--run-clang-based-tests-with").arg(clang_exe); - } - "0" | "no" | "off" => { - // Nothing to do. - } - other => { - // Let's make sure typos don't get unnoticed - panic!("Unrecognized option '{}' set in \ - RUSTBUILD_FORCE_CLANG_BASED_TESTS", other); - } - } + if util::forcing_clang_based_tests() { + let clang_exe = builder.llvm_out(target).join("bin").join("clang"); + cmd.arg("--run-clang-based-tests-with").arg(clang_exe); } // Get paths from cmd args diff --git a/src/bootstrap/util.rs b/src/bootstrap/util.rs index f22f0559265..9f684678bb0 100644 --- a/src/bootstrap/util.rs +++ b/src/bootstrap/util.rs @@ -356,3 +356,19 @@ impl CiEnv { } } } + +pub fn forcing_clang_based_tests() -> bool { + if let Some(var) = env::var_os("RUSTBUILD_FORCE_CLANG_BASED_TESTS") { + match &var.to_string_lossy().to_lowercase()[..] { + "1" | "yes" | "on" => true, + "0" | "no" | "off" => false, + other => { + // Let's make sure typos don't go unnoticed + panic!("Unrecognized option '{}' set in \ + RUSTBUILD_FORCE_CLANG_BASED_TESTS", other) + } + } + } else { + false + } +}