From 318626710ee96b6ac2c59480bfc5f03048239220 Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Fri, 18 Sep 2020 18:46:56 -0300 Subject: [PATCH 1/3] Do not make local copies of inline fns in debug mode --- compiler/rustc_middle/src/mir/mono.rs | 19 +++++++------------ .../inline-always-many-cgu/Makefile | 7 ++++++- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_middle/src/mir/mono.rs b/compiler/rustc_middle/src/mir/mono.rs index 1e70f7605045e..b79c1a19a907e 100644 --- a/compiler/rustc_middle/src/mir/mono.rs +++ b/compiler/rustc_middle/src/mir/mono.rs @@ -1,7 +1,6 @@ use crate::dep_graph::{DepConstructor, DepNode, WorkProduct, WorkProductId}; use crate::ich::{NodeIdHashingMode, StableHashingContext}; use crate::ty::{subst::InternalSubsts, Instance, InstanceDef, SymbolName, TyCtxt}; -use rustc_attr::InlineAttr; use rustc_data_structures::base_n; use rustc_data_structures::fingerprint::Fingerprint; use rustc_data_structures::fx::FxHashMap; @@ -103,17 +102,13 @@ impl<'tcx> MonoItem<'tcx> { // inlined function. If we're inlining into all CGUs then we'll // be creating a local copy per CGU. if generate_cgu_internal_copies { - return InstantiationMode::LocalCopy; - } - - // Finally, if this is `#[inline(always)]` we're sure to respect - // that with an inline copy per CGU, but otherwise we'll be - // creating one copy of this `#[inline]` function which may - // conflict with upstream crates as it could be an exported - // symbol. - match tcx.codegen_fn_attrs(instance.def_id()).inline { - InlineAttr::Always => InstantiationMode::LocalCopy, - _ => InstantiationMode::GloballyShared { may_conflict: true }, + InstantiationMode::LocalCopy + } else { + // Finally, if we've reached this point, then we should optimize for + // compilation speed. In that regard, we will ignore any `#[inline]` + // annotations on the function and simply codegen it as usual. This could + // conflict with upstream crates as it could be an exported symbol. + InstantiationMode::GloballyShared { may_conflict: true } } } MonoItem::Static(..) | MonoItem::GlobalAsm(..) => { diff --git a/src/test/run-make-fulldeps/inline-always-many-cgu/Makefile b/src/test/run-make-fulldeps/inline-always-many-cgu/Makefile index 0cab955f6442b..d12a23fbbf013 100644 --- a/src/test/run-make-fulldeps/inline-always-many-cgu/Makefile +++ b/src/test/run-make-fulldeps/inline-always-many-cgu/Makefile @@ -1,7 +1,12 @@ -include ../tools.mk all: - $(RUSTC) foo.rs --emit llvm-ir -C codegen-units=2 + $(RUSTC) foo.rs --emit llvm-ir -C codegen-units=2 -C opt-level=0 + if ![cat $(TMPDIR)/*.ll | $(CGREP) -e '\bcall\b']; then \ + echo "not found call instruction when one was expected"; \ + exit 1; \ + fi + $(RUSTC) foo.rs --emit llvm-ir -C codegen-units=2 -C opt-level=1 if cat $(TMPDIR)/*.ll | $(CGREP) -e '\bcall\b'; then \ echo "found call instruction when one wasn't expected"; \ exit 1; \ From aeb3061c40593e9cfbb52a1d5c8dafb1bd3e6a18 Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Wed, 9 Dec 2020 07:18:32 -0500 Subject: [PATCH 2/3] Update two failing incremental tests for the new behavior On the nopt builders, we disable optimization by default for all tests which causes the new behavior to take effect and causes the tests to fail when they should not. By passing the `-O` flag explicitly, we will always run these tests with optimizations enabled. --- src/test/incremental/hygiene/load_cached_hygiene.rs | 9 +++++++-- src/test/incremental/remapped_paths_cc/main.rs | 9 ++++++++- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/test/incremental/hygiene/load_cached_hygiene.rs b/src/test/incremental/hygiene/load_cached_hygiene.rs index 8124141418bc3..d6a5cb993a467 100644 --- a/src/test/incremental/hygiene/load_cached_hygiene.rs +++ b/src/test/incremental/hygiene/load_cached_hygiene.rs @@ -1,5 +1,5 @@ // revisions:rpass1 rpass2 -// compile-flags: -Z query-dep-graph +// compile-flags: -Z query-dep-graph -O // aux-build:cached_hygiene.rs // This tests the folllowing scenario @@ -19,7 +19,12 @@ // the metadata. Specifically, we were not resetting `orig_id` // for an `EpxnData` generate in the current crate, which would cause // us to serialize the `ExpnId` pointing to a garbage location in -// the metadata. +// the metadata.o + +// NOTE: We're explicitly passing the `-O` optimization flag because if optimizations are not +// enabled, then rustc will ignore the `#[inline(always)]` attribute which means we do not load +// the optimized mir for the unmodified function to be loaded and so the CGU containing that +// function will be reused. #![feature(rustc_attrs)] diff --git a/src/test/incremental/remapped_paths_cc/main.rs b/src/test/incremental/remapped_paths_cc/main.rs index b01f02444eae8..735635029dac9 100644 --- a/src/test/incremental/remapped_paths_cc/main.rs +++ b/src/test/incremental/remapped_paths_cc/main.rs @@ -1,11 +1,18 @@ // revisions:rpass1 rpass2 rpass3 -// compile-flags: -Z query-dep-graph -g +// compile-flags: -Z query-dep-graph -g -O // aux-build:extern_crate.rs // ignore-asmjs wasm2js does not support source maps yet + // This test case makes sure that we detect if paths emitted into debuginfo // are changed, even when the change happens in an external crate. +// NOTE: We're explicitly passing the `-O` optimization flag because if no optimizations are +// requested, rustc will ignore the `#[inline]` attribute. This is a performance optimization for +// non-optimized builds which causes us to generate fewer copies of inlined functions when +// runtime performance doesn't matter. Without this flag, the function will go into a different +// CGU which can be reused by this crate. + #![feature(rustc_attrs)] #![rustc_partition_reused(module="main", cfg="rpass2")] From 07a59822aacaf53ef2396b6e43fa3e8ad3a3e983 Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Fri, 18 Dec 2020 07:43:55 -0500 Subject: [PATCH 3/3] Improve comment and move code up --- compiler/rustc_middle/src/mir/mono.rs | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/compiler/rustc_middle/src/mir/mono.rs b/compiler/rustc_middle/src/mir/mono.rs index b79c1a19a907e..18ef5d42084b8 100644 --- a/compiler/rustc_middle/src/mir/mono.rs +++ b/compiler/rustc_middle/src/mir/mono.rs @@ -78,14 +78,6 @@ impl<'tcx> MonoItem<'tcx> { } pub fn instantiation_mode(&self, tcx: TyCtxt<'tcx>) -> InstantiationMode { - let generate_cgu_internal_copies = tcx - .sess - .opts - .debugging_opts - .inline_in_all_cgus - .unwrap_or_else(|| tcx.sess.opts.optimize != OptLevel::No) - && !tcx.sess.link_dead_code(); - match *self { MonoItem::Fn(ref instance) => { let entry_def_id = tcx.entry_fn(LOCAL_CRATE).map(|(id, _)| id); @@ -98,9 +90,18 @@ impl<'tcx> MonoItem<'tcx> { return InstantiationMode::GloballyShared { may_conflict: false }; } + let generate_cgu_internal_copies = tcx + .sess + .opts + .debugging_opts + .inline_in_all_cgus + .unwrap_or_else(|| tcx.sess.opts.optimize != OptLevel::No) + && !tcx.sess.link_dead_code(); + // At this point we don't have explicit linkage and we're an - // inlined function. If we're inlining into all CGUs then we'll - // be creating a local copy per CGU. + // inlined function. If we should generate local copies for each CGU, + // then return `LocalCopy`, otherwise we'll just generate one copy + // and share it with all CGUs in this crate. if generate_cgu_internal_copies { InstantiationMode::LocalCopy } else {