Auto merge of #60262 - michaelwoerister:pgo-preinlining-pass, r=alexcrichton

PGO: Add a run-make test that makes sure that PGO profiling data is used by the compiler during optimizations.

From the tests comment section:
```
# This test makes sure that PGO profiling data leads to cold functions being
# marked as `cold` and hot functions with `inlinehint`.
# The test program contains an `if` were actual execution only ever takes the
# `else` branch. Accordingly, we expect the function that is never called to
# be marked as cold.
```

r? @alexcrichton

src/bootstrap/test.rs

@@ -1231,6 +1231,28 @@ impl Step for Compiletest {
                 if let Some(ar) = builder.ar(target) {
                     cmd.arg("--ar").arg(ar);
                 }
+
+                // The llvm/bin directory contains many useful cross-platform
+                // tools. Pass the path to run-make tests so they can use them.
+                let llvm_bin_path = llvm_config.parent()
+                    .expect("Expected llvm-config to be contained in directory");
+                assert!(llvm_bin_path.is_dir());
+                cmd.arg("--llvm-bin-dir").arg(llvm_bin_path);
+
+                // If LLD is available, add it to the PATH
+                if builder.config.lld_enabled {
+                    let lld_install_root = builder.ensure(native::Lld {
+                        target: builder.config.build,
+                    });
+
+                    let lld_bin_path = lld_install_root.join("bin");
+
+                    let old_path = env::var_os("PATH").unwrap_or_default();
+                    let new_path = env::join_paths(std::iter::once(lld_bin_path)
+                        .chain(env::split_paths(&old_path)))
+                        .expect("Could not add LLD bin path to PATH");
+                    cmd.env("PATH", new_path);
+                }
             }
         }
 

src/bootstrap/tool.rs

@@ -9,7 +9,6 @@ use crate::Compiler;
 use crate::builder::{Step, RunConfig, ShouldRun, Builder};
 use crate::util::{exe, add_lib_path};
 use crate::compile;
-use crate::native;
 use crate::channel::GitInfo;
 use crate::channel;
 use crate::cache::Interned;
@@ -698,56 +697,6 @@ impl<'a> Builder<'a> {
             }
         }
 
-        // Add the llvm/bin directory to PATH since it contains lots of
-        // useful, platform-independent tools
-        if tool.uses_llvm_tools() && !self.config.dry_run {
-            let mut additional_paths = vec![];
-
-            if let Some(llvm_bin_path) = self.llvm_bin_path() {
-                additional_paths.push(llvm_bin_path);
-            }
-
-            // If LLD is available, add that too.
-            if self.config.lld_enabled {
-                let lld_install_root = self.ensure(native::Lld {
-                    target: self.config.build,
-                });
-
-                let lld_bin_path = lld_install_root.join("bin");
-                additional_paths.push(lld_bin_path);
-            }
-
-            if host.contains("windows") {
-                // On Windows, PATH and the dynamic library path are the same,
-                // so we just add the LLVM bin path to lib_path
-                lib_paths.extend(additional_paths);
-            } else {
-                let old_path = env::var_os("PATH").unwrap_or_default();
-                let new_path = env::join_paths(additional_paths.into_iter()
-                        .chain(env::split_paths(&old_path)))
-                    .expect("Could not add LLVM bin path to PATH");
-                cmd.env("PATH", new_path);
-            }
-        }
-
         add_lib_path(lib_paths, cmd);
     }
-
-    fn llvm_bin_path(&self) -> Option<PathBuf> {
-        if self.config.llvm_enabled() {
-            let llvm_config = self.ensure(native::Llvm {
-                target: self.config.build,
-                emscripten: false,
-            });
-
-            // Add the llvm/bin directory to PATH since it contains lots of
-            // useful, platform-independent tools
-            let llvm_bin_path = llvm_config.parent()
-                .expect("Expected llvm-config to be contained in directory");
-            assert!(llvm_bin_path.is_dir());
-            Some(llvm_bin_path.to_path_buf())
-        } else {
-            None
-        }
-    }
 }

src/test/run-make-fulldeps/cross-lang-lto-clang/Makefile

@@ -9,17 +9,17 @@ all: cpp-executable rust-executable
 
 cpp-executable:
 	$(RUSTC) -Clinker-plugin-lto=on -o $(TMPDIR)/librustlib-xlto.a -Copt-level=2 -Ccodegen-units=1 ./rustlib.rs
-	$(LD_LIB_PATH_ENVVAR)=$(REAL_LD_LIBRARY_PATH) $(CLANG) -flto=thin -fuse-ld=lld -L $(TMPDIR) -lrustlib-xlto -o $(TMPDIR)/cmain ./cmain.c -O3
+	$(CLANG) -flto=thin -fuse-ld=lld -L $(TMPDIR) -lrustlib-xlto -o $(TMPDIR)/cmain ./cmain.c -O3
 	# Make sure we don't find a call instruction to the function we expect to
 	# always be inlined.
-	$(LD_LIB_PATH_ENVVAR)=$(REAL_LD_LIBRARY_PATH) llvm-objdump -d $(TMPDIR)/cmain | $(CGREP) -v -e "call.*rust_always_inlined"
+	"$(LLVM_BIN_DIR)"/llvm-objdump -d $(TMPDIR)/cmain | $(CGREP) -v -e "call.*rust_always_inlined"
 	# As a sanity check, make sure we do find a call instruction to a
 	# non-inlined function
-	$(LD_LIB_PATH_ENVVAR)=$(REAL_LD_LIBRARY_PATH) llvm-objdump -d $(TMPDIR)/cmain | $(CGREP) -e "call.*rust_never_inlined"
+	"$(LLVM_BIN_DIR)"/llvm-objdump -d $(TMPDIR)/cmain | $(CGREP) -e "call.*rust_never_inlined"
 
 rust-executable:
-	$(LD_LIB_PATH_ENVVAR)=$(REAL_LD_LIBRARY_PATH) $(CLANG) ./clib.c -flto=thin -c -o $(TMPDIR)/clib.o -O2
+	$(CLANG) ./clib.c -flto=thin -c -o $(TMPDIR)/clib.o -O2
 	(cd $(TMPDIR); $(AR) crus ./libxyz.a ./clib.o)
 	$(RUSTC) -Clinker-plugin-lto=on -L$(TMPDIR) -Copt-level=2 -Clinker=$(CLANG) -Clink-arg=-fuse-ld=lld ./main.rs -o $(TMPDIR)/rsmain
-	$(LD_LIB_PATH_ENVVAR)=$(REAL_LD_LIBRARY_PATH) llvm-objdump -d $(TMPDIR)/rsmain | $(CGREP) -e "call.*c_never_inlined"
-	$(LD_LIB_PATH_ENVVAR)=$(REAL_LD_LIBRARY_PATH) llvm-objdump -d $(TMPDIR)/rsmain | $(CGREP) -v -e "call.*c_always_inlined"
+	"$(LLVM_BIN_DIR)"/llvm-objdump -d $(TMPDIR)/rsmain | $(CGREP) -e "call.*c_never_inlined"
+	"$(LLVM_BIN_DIR)"/llvm-objdump -d $(TMPDIR)/rsmain | $(CGREP) -v -e "call.*c_always_inlined"

src/test/run-make-fulldeps/cross-lang-lto-upstream-rlibs/Makefile

@@ -12,7 +12,7 @@ all: staticlib.rs upstream.rs
 
 	# Check No LTO
 	$(RUSTC) staticlib.rs -C linker-plugin-lto -Ccodegen-units=1 -L. -o $(TMPDIR)/staticlib.a
-	(cd $(TMPDIR); $(LD_LIB_PATH_ENVVAR)=$(REAL_LD_LIBRARY_PATH) llvm-ar x ./staticlib.a)
+	(cd $(TMPDIR); "$(LLVM_BIN_DIR)"/llvm-ar x ./staticlib.a)
 	# Make sure the upstream object file was included
 	ls $(TMPDIR)/upstream.*.rcgu.o
 
@@ -22,7 +22,7 @@ all: staticlib.rs upstream.rs
 	# Check ThinLTO
 	$(RUSTC) upstream.rs -C linker-plugin-lto -Ccodegen-units=1 -Clto=thin
 	$(RUSTC) staticlib.rs -C linker-plugin-lto -Ccodegen-units=1 -Clto=thin -L. -o $(TMPDIR)/staticlib.a
-	(cd $(TMPDIR); $(LD_LIB_PATH_ENVVAR)=$(REAL_LD_LIBRARY_PATH) llvm-ar x ./staticlib.a)
+	(cd $(TMPDIR); "$(LLVM_BIN_DIR)"/llvm-ar x ./staticlib.a)
 	ls $(TMPDIR)/upstream.*.rcgu.o
 
 else

src/test/run-make-fulldeps/cross-lang-lto/Makefile

@@ -10,8 +10,8 @@ ifndef IS_WINDOWS
 # -Clinker-plugin-lto.
 
 # this only succeeds for bitcode files
-ASSERT_IS_BITCODE_OBJ=($(LD_LIB_PATH_ENVVAR)=$(REAL_LD_LIBRARY_PATH) llvm-bcanalyzer $(1))
-EXTRACT_OBJS=(cd $(TMPDIR); rm -f ./*.o; $(LD_LIB_PATH_ENVVAR)=$(REAL_LD_LIBRARY_PATH) llvm-ar x $(1))
+ASSERT_IS_BITCODE_OBJ=("$(LLVM_BIN_DIR)"/llvm-bcanalyzer $(1))
+EXTRACT_OBJS=(cd $(TMPDIR); rm -f ./*.o; "$(LLVM_BIN_DIR)"/llvm-ar x $(1))
 
 BUILD_LIB=$(RUSTC) lib.rs -Copt-level=2 -Clinker-plugin-lto -Ccodegen-units=1
 BUILD_EXE=$(RUSTC) main.rs -Copt-level=2 -Clinker-plugin-lto -Ccodegen-units=1 --emit=obj

src/test/run-make-fulldeps/pgo-use/Makefile

@@ -0,0 +1,51 @@
+# needs-profiler-support
+
+-include ../tools.mk
+
+# This test makes sure that PGO profiling data leads to cold functions being
+# marked as `cold` and hot functions with `inlinehint`.
+# The test program contains an `if` were actual execution only ever takes the
+# `else` branch. Accordingly, we expect the function that is never called to
+# be marked as cold.
+#
+# The program is compiled with `-Copt-level=s` because this setting disables
+# LLVM's pre-inlining pass (i.e. a pass that does some inlining before it adds
+# the profiling instrumentation). Disabling this pass leads to rather
+# predictable IR which we need for this test to be stable.
+
+COMMON_FLAGS=-Copt-level=s -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
+
+ifeq ($(UNAME),Darwin)
+# macOS does not have the `tac` command, but `tail -r` does the same thing
+TAC := tail -r
+else
+# some other platforms don't support the `-r` flag for `tail`, so use `tac`
+TAC := tac
+endif
+
+all:
+	# Compile the test program with instrumentation
+	$(RUSTC) $(COMMON_FLAGS) -Z pgo-gen="$(TMPDIR)" main.rs
+	# Run it in order to generate some profiling data
+	$(call RUN,main some-argument) || exit 1
+	# Postprocess the profiling data so it can be used by the compiler
+	"$(LLVM_BIN_DIR)"/llvm-profdata merge \
+		-o "$(TMPDIR)"/merged.profdata \
+		"$(TMPDIR)"/default_*.profraw
+	# Compile the test program again, making use of the profiling data
+	$(RUSTC) $(COMMON_FLAGS) -Z pgo-use="$(TMPDIR)"/merged.profdata --emit=llvm-ir main.rs
+	# Check that the generate IR contains some things that we expect
+	#
+	# We feed the file into LLVM FileCheck tool *in reverse* so that we see the
+	# line with the function name before the line with the function attributes.
+	# FileCheck only supports checking that something matches on the next line,
+	# but not if something matches on the previous line.
+	$(TAC) "$(TMPDIR)"/main.ll | "$(LLVM_FILECHECK)" filecheck-patterns.txt

src/test/run-make-fulldeps/pgo-use/filecheck-patterns.txt

@@ -0,0 +1,11 @@
+# Add a check that the IR contains some expected metadata
+CHECK: !{!"ProfileFormat", !"InstrProf"}
+CHECK: !"ProfileSummary"
+
+# Make sure that the hot function is marked with `inlinehint`
+CHECK: define {{.*}} @hot_function
+CHECK-NEXT: Function Attrs:{{.*}}inlinehint
+
+# Make sure that the cold function is marked with `cold`
+CHECK: define {{.*}} @cold_function
+CHECK-NEXT: Function Attrs:{{.*}}cold

src/test/run-make-fulldeps/pgo-use/main.rs

@@ -0,0 +1,23 @@
+#[no_mangle]
+pub fn cold_function(c: u8) {
+    println!("cold {}", c);
+}
+
+#[no_mangle]
+pub fn hot_function(c: u8) {
+    std::env::set_var(format!("var{}", c), format!("hot {}", c));
+}
+
+fn main() {
+    let arg = std::env::args().skip(1).next().unwrap();
+
+    for i in 0 .. 1000_000 {
+        let some_value = arg.as_bytes()[i % arg.len()];
+        if some_value == b'!' {
+            // This branch is never taken at runtime
+            cold_function(some_value);
+        } else {
+            hot_function(some_value);
+        }
+    }
+}

src/test/run-make-fulldeps/tools.mk

@@ -47,6 +47,7 @@ DYLIB = $(TMPDIR)/$(1).dll
 STATICLIB = $(TMPDIR)/$(1).lib
 STATICLIB_GLOB = $(1)*.lib
 BIN = $(1).exe
+LLVM_FILECHECK := $(shell cygpath -u "$(LLVM_FILECHECK)")
 else
 RUN = $(TARGET_RPATH_ENV) $(RUN_BINFILE)
 FAIL = $(TARGET_RPATH_ENV) $(RUN_BINFILE) && exit 1 || exit 0

src/tools/compiletest/src/common.rs

@@ -144,6 +144,9 @@ pub struct Config {
     /// The LLVM `FileCheck` binary path.
     pub llvm_filecheck: Option<PathBuf>,
 
+    /// Path to LLVM's bin directory.
+    pub llvm_bin_dir: Option<PathBuf>,
+
     /// The valgrind path.
     pub valgrind_path: Option<String>,
 

src/tools/compiletest/src/main.rs

@@ -221,6 +221,7 @@ pub fn parse_config(args: Vec<String>) -> Config {
             "LIST",
         )
         .reqopt("", "llvm-cxxflags", "C++ flags for LLVM", "FLAGS")
+        .optopt("", "llvm-bin-dir", "Path to LLVM's `bin` directory", "PATH")
         .optopt("", "nodejs", "the name of nodejs", "PATH")
         .optopt(
             "",
@@ -306,7 +307,8 @@ pub fn parse_config(args: Vec<String>) -> Config {
         valgrind_path: matches.opt_str("valgrind-path"),
         force_valgrind: matches.opt_present("force-valgrind"),
         run_clang_based_tests_with: matches.opt_str("run-clang-based-tests-with"),
-        llvm_filecheck: matches.opt_str("llvm-filecheck").map(|s| PathBuf::from(&s)),
+        llvm_filecheck: matches.opt_str("llvm-filecheck").map(PathBuf::from),
+        llvm_bin_dir: matches.opt_str("llvm-bin-dir").map(PathBuf::from),
         src_base,
         build_base: opt_path(matches, "build-base"),
         stage_id: matches.opt_str("stage-id").unwrap(),

src/tools/compiletest/src/runtest.rs

@@ -2691,6 +2691,14 @@ impl<'test> TestCx<'test> {
             cmd.env("CLANG", clang);
         }
 
+        if let Some(ref filecheck) = self.config.llvm_filecheck {
+            cmd.env("LLVM_FILECHECK", filecheck);
+        }
+
+        if let Some(ref llvm_bin_dir) = self.config.llvm_bin_dir {
+            cmd.env("LLVM_BIN_DIR", llvm_bin_dir);
+        }
+
         // We don't want RUSTFLAGS set from the outside to interfere with
         // compiler flags set in the test cases:
         cmd.env_remove("RUSTFLAGS");