remove inlined functions for faster compilation #92

andjo403 · 2017-09-17T20:19:29Z

after inlining remove functions that is not called any more
reduces compile times due to the removed functions is not further optimized.

part of solution for rust-lang/rust#44655

alexcrichton · 2017-09-17T22:11:43Z

Wow those are some awesome wins you're seeing! Would you be willing to try to send this upstream to LLVM itself as well? Unfortunately I'm less familiar with their contribution process, but it'd be great to ensure that their experiences eyes also took a look at this!

andjo403 · 2017-09-18T05:19:39Z

I'm waiting for a bugzilla account for LLVM so I can add a ticket upstream

andjo403 · 2017-09-18T06:22:55Z

llvm ticket

alexcrichton · 2017-09-18T14:06:01Z

Thanks @andjo403! Let's wait a few days and see what happens upstream there.

dcci · 2017-09-18T16:03:32Z

LLVM dev here [with a hidden passion for rust] :)
I see the rationale behind your commit. I think it makes sense, and you're probably seeing a big benefit because you have a bunch of small functions (even better if marked as static) with a single caller.
If you look at the inliner code in LLVM you'll see it gives a ridicolous bonus (grep for LastCallToStaticBonus, or something along these lines).

I wonder whether it's better running GDCE after the inliner or having the inliner itself removing dead functions after they're inlined (admittedly, I haven't looked at whether the latter is feasible).

FWIW, what's your benchmark? Can you try with big cases (like, building rust, building clang itself, maybe with LTO [this last one generaly should leave a bunch of unused function around])

alexcrichton · 2017-09-18T16:08:21Z

@dcci hello again! FWIW on rust-lang/rust#44655 it mentions a 23% improvement for the racer project which I think is relatively large, and it also mentions a 15% improvement for rustc itself which I know for a fact is quite large! I haven't tried this out myself yet, though, to verify.

dcci

See my comment.

dcci · 2017-09-18T16:08:26Z

lib/Transforms/IPO/PassManagerBuilder.cpp

@@ -478,6 +478,8 @@ void PassManagerBuilder::populateModulePassManager(
  if (Inliner) {
    MPM.add(Inliner);
    Inliner = nullptr;
+    // Removes functions that is not used due to inlining


nit: that "are" not used.

thanks will fix this

dcci · 2017-09-18T16:12:41Z

OK, thanks for your datapoints.
FWIW, GDCE is O(n), so I don't expect that taking a lot of time, but still, we should make a more careful analysis before changing the standard -O2 pipeline. If you're willing to run some tests on rust, I'm going to run something equivalent on C++ (as I mentioned earlier, I'm particularly interested in LTO).
I'm a little swamped at the moment so that my take a week or two before we get the patch in, how does that sound like as timeline?

Thanks!

alexcrichton · 2017-09-18T16:17:43Z

Sure yeah! @andjo403 if you want to poke around as well I can try to reproduce the rustc build times and also test some projects like Cargo and some simple-ish LTO cases.

andjo403 · 2017-09-18T16:54:39Z

@alexcrichton sounds good. will try some more projects was thinking of running the https://github.com/rust-lang-nursery/rustc-benchmarks compare.py but will have to install a virtualbox or something due to it is using make and I'm on windows.

alexcrichton · 2017-09-18T17:31:46Z

Some stats:

These numbers are for whole compilations, basically how long it took cargo build --release to finish, which means it's timing a whole lot more than just LLVM:

rustc itself 505s -> 423s
cargo 215s -> 163s
small async I/O server 57s -> 48s

Some other numbers

Compiling Cargo with LTO, just the final rustc invocation 106s -> 95s

Definitely seems like some possible big improvements to be had! Apparently not helping LTO a whole lot though :(

andjo403 · 2017-09-18T17:37:48Z

@dcci and @alexcrichton maybe I do not understand how LTO works but I do not see how functions can be removed from an other crate. the linkage must be not private for LTO to do something and it feels like GDCE only can remove private linkage functions.

alexcrichton · 2017-09-18T18:04:57Z

@andjo403 oh I think it's just with LTO that typically LLVM works with much larger LLVM modules (lots more code in memory all at once). In theory there's also tons of inlining and in theory greater wins to be had, but the one test I had didn't look too promising unfortunately :(

dcci · 2017-09-18T18:09:07Z

@alexcrichton Alex, I'm a little surprised of these numbers as, FWIW, LTO should be the kinda best case but not always really.
Please note that if you do LTO, the inliner runs twice. 1st time, during the per-TU pipeline (i.e. -O2 and -O3) and another time on the whole module.
I'm not sure how rust actually sets up the pipeline, but if it gets the stock from LLVM, you might consider adding the pass also to populateLTOPassManager (or something similar, I don't remember the name off the top of my head), otherwise, GDCE won't run after the inliner at link time.

alexcrichton · 2017-09-18T18:16:43Z

Aha indeed! Looks like there's a spot this PR as-is doesn't handle. I've updated that and will report back the LTO build for Cargo.

dcci · 2017-09-18T18:27:38Z

Yes, that's exactly it. You were only getting the benefit from the per-TU pipeline.

alexcrichton · 2017-09-18T18:30:59Z

Ah unfortunately that ended up having no impact at all :(

andjo403 · 2017-09-18T18:32:58Z

@alexcrichton there is already one GDCE pass added some lines below https://github.com/andjo403/llvm-1/blob/42e941e24e38c8225bad59632ce56d13949c7f38/lib/Transforms/IPO/PassManagerBuilder.cpp#L747

alexcrichton · 2017-09-19T14:46:00Z

Ah that'd explain the lack of improvements!

dcci · 2017-09-21T18:51:21Z

@alexcrichton @andjo403
So, I've been looking at this more closely. I'm not able to find any obvious regression, so I think I'm going for a slightly modified version of this patch for review (probably, we should also run another pass, GlobalOpt in tandem with GlobalDCE). Once that is done, I guess you guys can probably backport this thingy back? Of course, I'll give the correct attribution for the idea =)

alexcrichton · 2017-09-21T19:36:33Z

@dcci sounds great, and thanks again for helping out with this! If you've got a patch I can run some tests for it for sure. And yeah we'll take care of backporting for our own "fork", no need to worry about that!

andjo403 · 2017-09-21T19:48:33Z

@alexcrichton shall I close this PR then?
@dcci sounds amazing thanks for the help

dcci · 2017-10-02T19:33:12Z

FWIW, I'm working on this, but the patch exposes a series of misoptimizations in LLVM that need to be fixed. I'm working on these now and I hope to enable the patch once they're all addressed.

dcci · 2017-10-02T20:12:50Z

OK, I think the patch was placed in a slightly incorrect point in the pipeline.
Can you please re-run with the following patch applied?

Thanks!

diff --git a/lib/Transforms/IPO/PassManagerBuilder.cpp b/lib/Transforms/IPO/PassManagerBuilder.cpp
index b384629..9155d1a 100644
--- a/lib/Transforms/IPO/PassManagerBuilder.cpp
+++ b/lib/Transforms/IPO/PassManagerBuilder.cpp
@@ -499,10 +499,14 @@ void PassManagerBuilder::populateModulePassManager(
   // Start of CallGraph SCC passes.
   if (!DisableUnitAtATime)
     MPM.add(createPruneEHPass()); // Remove dead EH info
+  bool RunInliner = false;
   if (Inliner) {
     MPM.add(Inliner);
     Inliner = nullptr;
+    RunInliner = true;
   }
+
+
   if (!DisableUnitAtATime)
     MPM.add(createPostOrderFunctionAttrsLegacyPass());
   if (OptLevel > 2)
@@ -515,6 +519,18 @@ void PassManagerBuilder::populateModulePassManager(
   // pass manager that we are specifically trying to avoid. To prevent this
   // we must insert a no-op module pass to reset the pass manager.
   MPM.add(createBarrierNoopPass());
+
+  // The inliner performs some kind of dead code elimination as it goes,
+  // but there are cases that are not really caught by it. We might
+  // at some point consider teaching the inliner about them, but it
+  // is OK for now to run GlobalOpt + GlobalDCE in tandem as their
+  // benefits generally outweight the cost, making the whole pipeline
+  // faster. See PR34652.
+  if (RunInliner) {
+    MPM.add(createGlobalOptimizerPass());
+    MPM.add(createGlobalDCEPass());
+  }
+
   if (RunPartialInlining)
     MPM.add(createPartialInliningPass());

after inlining remove functions that is not called any more reduces compile times due to the removed functions is not further optimized.

andjo403 · 2017-10-02T20:53:18Z

have update this PR with the patch from @dcci so it shall be easier to test for others
I have started the compilation not sure if I will wait for the results tonight

andjo403 · 2017-10-02T21:46:33Z

@dcci I have only tested to compile race once with standard rustc and once with the patch and I get
LLVM Pass execution timing report for racer lib compilation
standard rustc Total Execution Time: 64.7500 seconds (68.0480 wall clock)
patch Total Execution Time: 35.9531 seconds (37.2969 wall clock)

and for all crates in racer the total time go from 405.71 secs to 315.5 secs
will run some more tests tomorrow

@alexcrichton can you also run some tests?

dcci · 2017-10-02T22:14:06Z

OK, I think this is fine.

dcci · 2017-10-05T18:27:54Z

Upstream rL314997 and rL314999.
Cheers.

andjo403 · 2017-10-05T18:30:40Z

@dcci thanks for all the help

alexcrichton · 2017-10-05T18:32:20Z

Thanks so much @dcci!

@andjo403 oh I actually just pushed this to the relevant branch, ensuring that it builds locally first though. @andjo403 want to send a PR to rust-lang/rust though?

andjo403 · 2017-10-05T18:34:24Z

@alexcrichton yes I want to send a PR to rust-lang/rust

cuviper · 2017-10-05T18:38:26Z

@alexcrichton looks like you only pushed 314997 -- did you intend to backport 314999 too?

alexcrichton · 2017-10-05T18:45:19Z

Oh oops, thanks @cuviper! Apparently when I fetched 314999 wasn't in the mirror yet...

andjo403 · 2017-10-05T19:17:24Z

@alexcrichton I wanted to test the the commit rust-lang/rust@8fd3c8f
before making a PR but I get:

failed to run: /home/ubuntu/rust/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /home/ubuntu/rust/src/bootstrap/Cargo.toml

alexcrichton · 2017-10-05T19:19:10Z

@andjo403 sounds suspicious! Mind gisting the full error you saw?

andjo403 · 2017-10-05T19:21:45Z

https://gist.github.com/andjo403/fd4aa056592f7ce807cc49f6ff2067c8

alexcrichton · 2017-10-05T19:23:34Z

Ah try doing git submodule update --init --recursive maybe?

andjo403 · 2017-10-05T19:25:47Z

yes solved it thanks

dcci suggested changes Sep 18, 2017

View reviewed changes

remove inlined functions for faster compilation

8e0cf18

after inlining remove functions that is not called any more reduces compile times due to the removed functions is not further optimized.

andjo403 closed this Oct 5, 2017

andjo403 deleted the remove_dead_code_after_inline branch October 5, 2017 19:26

This was referenced Mar 9, 2019

LowerCallResult regression bugfix avr-rust/llvm#10

Closed

Fix for avr-rust issue 129 avr-rust/llvm#9

Closed

remove inlined functions for faster compilation #92

remove inlined functions for faster compilation #92

Conversation

andjo403 commented Sep 17, 2017

alexcrichton commented Sep 17, 2017

andjo403 commented Sep 18, 2017

andjo403 commented Sep 18, 2017

alexcrichton commented Sep 18, 2017

dcci commented Sep 18, 2017

alexcrichton commented Sep 18, 2017

dcci left a comment

Choose a reason for hiding this comment

dcci Sep 18, 2017

Choose a reason for hiding this comment

andjo403 Sep 18, 2017

Choose a reason for hiding this comment

dcci commented Sep 18, 2017

alexcrichton commented Sep 18, 2017

andjo403 commented Sep 18, 2017

alexcrichton commented Sep 18, 2017

andjo403 commented Sep 18, 2017

alexcrichton commented Sep 18, 2017

dcci commented Sep 18, 2017

alexcrichton commented Sep 18, 2017

dcci commented Sep 18, 2017

alexcrichton commented Sep 18, 2017

andjo403 commented Sep 18, 2017 • edited Loading

alexcrichton commented Sep 19, 2017

dcci commented Sep 21, 2017

alexcrichton commented Sep 21, 2017

andjo403 commented Sep 21, 2017 • edited Loading

dcci commented Oct 2, 2017

dcci commented Oct 2, 2017

andjo403 commented Oct 2, 2017

andjo403 commented Oct 2, 2017 • edited Loading

dcci commented Oct 2, 2017

dcci commented Oct 5, 2017

andjo403 commented Oct 5, 2017 • edited Loading

alexcrichton commented Oct 5, 2017

andjo403 commented Oct 5, 2017

cuviper commented Oct 5, 2017

alexcrichton commented Oct 5, 2017

andjo403 commented Oct 5, 2017

alexcrichton commented Oct 5, 2017

andjo403 commented Oct 5, 2017

alexcrichton commented Oct 5, 2017

andjo403 commented Oct 5, 2017

andjo403 commented Sep 18, 2017 •

edited

Loading

andjo403 commented Sep 21, 2017 •

edited

Loading

andjo403 commented Oct 2, 2017 •

edited

Loading

andjo403 commented Oct 5, 2017 •

edited

Loading