Run LLVM coverage instrumentation passes before optimization passes #83666

Amanieu · 2021-03-30T01:12:50Z

This matches the behavior of Clang and allows us to remove several
hacks which were needed to ensure functions weren't optimized away
before reaching the instrumentation pass.

Fixes #83429

cc @richkadel

r? @tmandry

This matches the behavior of Clang and allows us to remove several hacks which were needed to ensure functions weren't optimized away before reaching the instrumentation pass.

richkadel

Looks great! Thanks!

richkadel · 2021-03-30T05:24:48Z

src/test/run-make-fulldeps/coverage/lib/used_inline_crate.rs

@@ -28,12 +28,6 @@ pub fn used_inline_function() {
    }
    use_this_lib_crate();
 }


Any theories why this is no longer generating the Unexecuted instantiation? It probably doesn't have to be documented here anymore (the absence of the unexpected should be expected, so this is good), but I'm curious why it changed.

Here's a trick for keeping the diffs clean in the expected results, above. The line numbers in llvm-cov show mess things up, so I avoid changing line numbers whenever possible: Just replace these 6 deleted lines with 6 blank lines.

The Unexecuted instantiation re-appears if I undo the changes in compiler/rustc_typeck/src/collect.rs. I'm not exactly sure how that causes it to re-appear though.

Yeah, I assumed it's related somehow.

Thanks for inserting the blank lines. Makes the expected result diffs a lot easier to read!

Do you want me to remove the blank lines before merging?

Not necessary. Thanks.

richkadel · 2021-03-30T05:26:39Z

compiler/rustc_codegen_llvm/src/back/write.rs

@@ -548,6 +548,11 @@ pub(crate) unsafe fn optimize(
                    llvm::LLVMRustAddPass(fpm, find_pass("lint").unwrap());
                    continue;
                }
+                if pass_name == "insert-gcov-profiling" || pass_name == "instrprof" {
+                    // Instrumentation should be inserted before optimization.


Can we say "must" instead of "should" and reference the Issue number?

I couldn't find a suitable issue to point to so I just pointed at the Clang code.

You're updated comment works for me. Thanks!

richkadel

Sorry, I just realized that the existing tests in the rust compiler don't confirm that your fix for the inline always actually works. See my comment about how to test this. Thanks!

richkadel · 2021-03-30T06:27:52Z

compiler/rustc_typeck/src/collect.rs

-                    } else {
-                        InlineAttr::Always
-                    }
+                    InlineAttr::Always


Oh, before you make this change, we need to test that this doesn't break the brotli_decompressor

See #82875

I was not able to create a coverage test that failed in the way it fails in this issue, but by not forcing inline always, the issue was resolved.

I just tested the example from that issue. It fails on my main rustc (2021-03-24 nightly) but succeeds on my new branch. So the fix for inline always works.

That's great.

Before merging, I'd like to try your patch on some creates I had issues with.

I cherry-picked your branch commits and am testing them now.

richkadel · 2021-03-30T22:33:21Z

I'm having an issue compiling the brotli_decompressor test with anything higher than -Copt-level=1. The compiler is much slower when compiling with -Copt-level=2. (Level 3 is not much slower than 2 though.)

I don't remember ever having to wait very long for the compiler to compile and link one small program like this.

Is it possible your change could significantly slow down the optimization passes?

extern crate brotli_decompressor;
use std::io;

fn main() {
    match brotli_decompressor::BrotliDecompress(&mut io::stdin(), &mut io::stdout()) {
        Ok(_) => {}
        Err(e) => println!("Error {:?}", e),
    }
}

$ time ~/rust/build/x86_64-unknown-linux-gnu/stage1/bin/rustc -Zinstrument-coverage -Copt-level=1 brotli_covtest.rs -L ~/rust-brotli-decompressor/target/debug/ -L ~/rust-brotli-decompressor/target/debug/deps --edition=2018

real    0m1.111s
user    0m3.136s
sys     0m0.179s

$ time ~/rust/build/x86_64-unknown-linux-gnu/stage1/bin/rustc -Zinstrument-coverage -Copt-level=2 brotli_covtest.rs -L ~/rust-brotli-decompressor/target/debug/ -L ~/rust-brotli-decompressor/target/debug/deps --edition=2018

real    0m23.503s
user    0m36.125s
sys     0m0.181s

$ time ~/rust/build/x86_64-unknown-linux-gnu/stage1/bin/rustc -Zinstrument-coverage -Copt-level=3 brotli_covtest.rs -L ~/rust-brotli-decompressor/target/debug/ -L ~/rust-brotli-decompressor/target/debug/deps --edition=2018

real    0m23.822s
user    0m36.961s
sys     0m0.211s

Amanieu · 2021-03-30T23:17:58Z

I can't reproduce this:

time rustc +stage1 -Zinstrument-coverage -Copt-level=3 brotli_covtest.rs -L target/debug/ -L target/debug/deps --edition=2018                                                                                                                                                                                    
________________________________________________________
Executed in    1.45 secs    fish           external
   usr time    2.26 secs  396.00 micros    2.26 secs
   sys time    0.07 secs   46.00 micros    0.07 secs

Can you try using -Ztime-passes -Ztime-llvm-passes to see what is taking up so much time?

richkadel · 2021-03-30T23:19:43Z

yes. Also, Let me rebuild the compiler after a clean. I want to make sure I don't have some weird residual code influencing this somehow.

richkadel · 2021-03-31T01:00:12Z

I'm not seeing an issue after a clean build. Not sure what happened, but I think it was something buggy in my build. Good to go from my perspective.

@tmandry - Thumbs up from me.

Thanks Amanieu!

tmandry · 2021-03-31T01:11:35Z

Oh this is great, a big mystery is now solved. Thanks for this!

@bors r+

bors · 2021-03-31T01:11:36Z

📌 Commit cad9b6b has been approved by tmandry

bors · 2021-03-31T03:20:37Z

⌛ Testing commit cad9b6b with merge 6ff482b...

bors · 2021-03-31T05:47:16Z

☀️ Test successful - checks-actions
Approved by: tmandry
Pushing 6ff482b to master...

klensy · 2021-03-31T13:44:16Z

https://perf.rust-lang.org/compare.html?start=65b44b0320e88f5a0608126c36b02be1b840700f&end=6ff482bde5d22a3a0171edb3245327f43cf9b593&stat=max-rss

Big deviations on kessak-debug and unicode_normalization-debug with incr-full, but maybe this is random thing.

Amanieu · 2021-03-31T13:49:24Z

The changes in PR only apply if -Zinstrument-coverage is used, so this is very surprising.

klensy · 2021-03-31T13:51:42Z

Currently rust-perf site works slow for me, maybe that affects backend performance collection process too?

klensy · 2021-03-31T14:05:07Z

Checked next pr, the same thing https://perf.rust-lang.org/compare.html?start=2a32abbcdea97c6bf1d0445bc657f16c50ca103b&end=a5029ac0ab372aec515db2e718da6d7787f3d122&stat=max-rss

Maybe some issues with performance collecting, or i checking wrong commits :-(

Run LLVM coverage instrumentation passes before optimization passes

26d260b

This matches the behavior of Clang and allows us to remove several hacks which were needed to ensure functions weren't optimized away before reaching the instrumentation pass.

Amanieu added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Mar 30, 2021

rust-highfive assigned tmandry Mar 30, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 30, 2021

richkadel suggested changes Mar 30, 2021

View reviewed changes

Apply review feedback

cad9b6b

richkadel suggested changes Mar 30, 2021

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 31, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 31, 2021

bors merged commit 6ff482b into rust-lang:master Mar 31, 2021

rustbot added this to the 1.53.0 milestone Mar 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run LLVM coverage instrumentation passes before optimization passes #83666

Run LLVM coverage instrumentation passes before optimization passes #83666

Amanieu commented Mar 30, 2021

richkadel left a comment

richkadel Mar 30, 2021

Amanieu Mar 30, 2021

richkadel Mar 30, 2021

Amanieu Mar 30, 2021

richkadel Mar 30, 2021

richkadel Mar 30, 2021

Amanieu Mar 30, 2021

richkadel Mar 30, 2021

richkadel left a comment

richkadel Mar 30, 2021

Amanieu Mar 30, 2021

richkadel Mar 30, 2021

richkadel Mar 30, 2021

richkadel commented Mar 30, 2021

Amanieu commented Mar 30, 2021

richkadel commented Mar 30, 2021

richkadel commented Mar 31, 2021

tmandry commented Mar 31, 2021

bors commented Mar 31, 2021

bors commented Mar 31, 2021

bors commented Mar 31, 2021

klensy commented Mar 31, 2021

Amanieu commented Mar 31, 2021

klensy commented Mar 31, 2021

klensy commented Mar 31, 2021

Run LLVM coverage instrumentation passes before optimization passes #83666

Run LLVM coverage instrumentation passes before optimization passes #83666

Conversation

Amanieu commented Mar 30, 2021

richkadel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richkadel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richkadel commented Mar 30, 2021

Amanieu commented Mar 30, 2021

richkadel commented Mar 30, 2021

richkadel commented Mar 31, 2021

tmandry commented Mar 31, 2021

bors commented Mar 31, 2021

bors commented Mar 31, 2021

bors commented Mar 31, 2021

klensy commented Mar 31, 2021

Amanieu commented Mar 31, 2021

klensy commented Mar 31, 2021

klensy commented Mar 31, 2021