This is an archive of the discontinued LLVM Phabricator instance.

WholeProgramDevirt: generate more detailed and accurate remarks.
ClosedPublic

Authored by krasin on Aug 8 2016, 10:50 PM.

Details

Summary

Keep track of all methods for which we have devirtualized at least
one call and then print them sorted alphabetically. That allows to
avoid duplicates and also makes the order deterministic.

Add optimization names into the remarks, so that it's easier to
understand how has each method been devirtualized.

Fix a bug when wrong methods could have been reported for
tryVirtualConstProp.

Diff Detail

Event Timeline

krasin updated this revision to Diff 67286.Aug 8 2016, 10:50 PM
krasin retitled this revision from to WholeProgramDevirt: generate more detailed and accurate remarks..
krasin updated this object.
krasin added a reviewer: mehdi_amini.
krasin updated this revision to Diff 67287.Aug 8 2016, 11:17 PM

fix a typo (simple->single); simplify a condition; sync

mehdi_amini edited edge metadata.Aug 8 2016, 11:24 PM

Hey Ivan, thanks for following up on this!
Some quick comments inlined.

lib/Transforms/IPO/WholeProgramDevirt.cpp
225

I don't think you need the explicit Twine() around the literal string.

824

I don't think we'd like to create a map when reporting is disabled though. And I don't know if we can detect that actually.

Anyway, if we'd go with a container here, why a map instead of a DenseSet<Function *>?

834–836

No need for braces here.

837

&&T? Why not const auto &T?

krasin updated this revision to Diff 67291.Aug 9 2016, 12:38 AM
krasin marked 2 inline comments as done.
krasin edited edge metadata.

Address comments.

lib/Transforms/IPO/WholeProgramDevirt.cpp
824

Yes, I share the same concern. Initially I tried to avoid having a map at all, but that produces too many duplicates, and no better than having a map.

Then I decided to go with a map, and while I also don't know a good way to check if remarks are enabled, it's certainly possible to make one. At the same time, before we go deep into this hole, I would like to actually see how much time does this pass take on the large targets (like Chrome) with and without the map. If profile shows that we considerably slowdown the pass, then we'll find a way to detect -pass-remarks=wholeprogramdevirt.

As for using DenseSet<Function*>: it does not have any guarantees about iterating order, and I would like to have a stable output. Certainly, it's possible to use DenseMap, then sort, but that feels like a premature optimization at this point.

Theoretically I could use std::set<Function*> with a comparator. That would save memory a little bit.

I am very open to your ideas here, especially, given that I am not yet native to LLVM, and don't have a map of LLVM collections types in my head, only http://llvm.org/docs/ProgrammersManual.html

837

In this particular place, no good reason. Done.

In other places, where I modify T.WasDevirt, I tried to be consistent with the rest of the code.

krasin marked 2 inline comments as done.Aug 9 2016, 12:38 AM
krasin added inline comments.Aug 9 2016, 12:41 AM
test/Transforms/WholeProgramDevirt/virtual-const-prop-check.ll
6–10

One thing that I want to include into this change, but didn't have time to work on this yet, is to also check the line numbers reported by the remarks. Just an FYI that the change will get an update into the tests.

krasin updated this revision to Diff 67417.Aug 9 2016, 2:37 PM

Add line numbers into a test

krasin marked an inline comment as done.Aug 9 2016, 2:41 PM

Mehdi,

please, take another look.

test/Transforms/WholeProgramDevirt/virtual-const-prop-check.ll
6–10

I have added debug info and checks for the line numbers into one of the tests. I think it's reasonable to only do it once, as debug metadata adds quite a bit of a low value / high noise info.

krasin marked an inline comment as done.Aug 10 2016, 6:49 PM

Mehdi,

please, take another look.

lib/Transforms/IPO/WholeProgramDevirt.cpp
824

I have some numbers to share. Whole program devirtualization pass takes 1.4 sec / 0.1% of the time when linking Chrome. This includes collecting and generating all the remarks in this CL.

Most of the time is spent on the real work; I might also collect the same stats without the patch applied, but it's probably not worth it. There's not much we can save here.

krasin added a reviewer: kcc.Aug 10 2016, 7:37 PM

I'm bothered by doing work that will be thrown away in *most* cases (i.e. -pass-remark is "rare"), this is the path of slowly but surely bloating the compiler as whole.

Talked to Chandler on IRC and he shared the same impression I don't find the argument that "the rest of the thing is too slow" to be compelling that we should do work that gets thrown away.

lib/Transforms/IPO/WholeProgramDevirt.cpp
838

One line comment for this loop.

843

const auto &?
Also one line comment before the loop.

Note: for the ThinLTO import/inline statistics, I think we are controlling this as a cl::opt.

Thank you for the hint. I will take a look at ThinLTO statistics and try to add a switch to these remarks too.

Note: I appreciate getting a useful feedback on the priorities of the LLVM project. The very same question might be answered differently in different projects. Thank you for correcting me, and please continue to do so when you feel my judgement is not aligned with the rest of the team.

krasin updated this revision to Diff 67720.Aug 11 2016, 11:49 AM
krasin marked 2 inline comments as done.

Only collect stats if remarks are enabled for the pass.

Hi Mehdi,

I have added checks to avoid doing unnecessary work. Please, take a look.

mehdi_amini accepted this revision.Aug 11 2016, 11:55 AM
mehdi_amini edited edge metadata.

LGTM.

lib/Transforms/IPO/WholeProgramDevirt.cpp
679

Interesting, I didn't know about that.

This revision is now accepted and ready to land.Aug 11 2016, 11:55 AM

Mehdi,

thank you for your review!

And huge thanks for spotting bugs in my previous CL that introduces these remarks.

lib/Transforms/IPO/WholeProgramDevirt.cpp
679

It was surprisingly quick to discover using Peter's http://llvm-cs.pcc.me.uk (or may be I was just lucky)

krasin closed this revision.Aug 11 2016, 12:16 PM