From 1abfd4ab2358a3bbaf4bd420fc75eb1cf348d067 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 17 Apr 2020 16:04:59 -0400 Subject: [PATCH 1/2] Issue #71248: attempt to recover perf by removing `exports_all_green` flag. (My hypothesis is that my use of this flag was an overly conservative generalization of PR 67020.) --- src/librustc_codegen_llvm/back/lto.rs | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/librustc_codegen_llvm/back/lto.rs b/src/librustc_codegen_llvm/back/lto.rs index e21cdee961d..29912c56b38 100644 --- a/src/librustc_codegen_llvm/back/lto.rs +++ b/src/librustc_codegen_llvm/back/lto.rs @@ -500,11 +500,11 @@ fn thin_lto( let module_name = module_name_to_str(module_name); // If (1.) the module hasn't changed, and (2.) none of the modules - // it imports from nor exports to have changed, *and* (3.) the - // import and export sets themselves have not changed from the - // previous compile when it was last ThinLTO'ed, then we can re-use - // the post-ThinLTO version of the module. Otherwise, freshly - // perform LTO optimization. + // it imports from have changed, *and* (3.) the import and export + // sets themselves have not changed from the previous compile when + // it was last ThinLTO'ed, then we can re-use the post-ThinLTO + // version of the module. Otherwise, freshly perform LTO + // optimization. // // (Note that globally, the export set is just the inverse of the // import set.) @@ -531,13 +531,8 @@ fn thin_lto( let imports_all_green = curr_imports .iter() .all(|imported_module| green_modules.contains_key(imported_module)); - let exports_all_green = curr_exports - .iter() - .all(|exported_module| green_modules.contains_key(exported_module)); - if imports_all_green && equivalent_as_sets(prev_imports, curr_imports) - && exports_all_green && equivalent_as_sets(prev_exports, curr_exports) { let work_product = green_modules[module_name].clone(); From 19e5a654dcef599bd2cb82eb90a9dfbc8b42d300 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 20 Apr 2020 10:33:27 -0400 Subject: [PATCH 2/2] Expand comment to justify not checking if all the exports are green. --- src/librustc_codegen_llvm/back/lto.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/librustc_codegen_llvm/back/lto.rs b/src/librustc_codegen_llvm/back/lto.rs index 29912c56b38..7292492a0c0 100644 --- a/src/librustc_codegen_llvm/back/lto.rs +++ b/src/librustc_codegen_llvm/back/lto.rs @@ -509,6 +509,23 @@ fn thin_lto( // (Note that globally, the export set is just the inverse of the // import set.) // + // For further justification of why the above is necessary and sufficient, + // see the LLVM blog post on ThinLTO: + // + // http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html + // + // which states the following: + // + // ```quote + // any particular ThinLTO backend must be redone iff: + // + // 1. The corresponding (primary) module’s bitcode changed + // 2. The list of imports into or exports from the module changed + // 3. The bitcode for any module being imported from has changed + // 4. Any global analysis result affecting either the primary module + // or anything it imports has changed. + // ``` + // // This strategy means we can always save the computed imports as // canon: when we reuse the post-ThinLTO version, condition (3.) // ensures that the current import set is the same as the previous