- User Since
- Apr 27 2015, 11:17 AM (125 w, 4 d)
Wed, Sep 20
Tue, Sep 19
Mon, Sep 18
LGTM with one fix noted.
Thanks for working on adding this missing feature! Please add a test and include an update to the info on where cache pruning is supported in ThinLTO.rst (for examples of both, see https://reviews.llvm.org/D37607, where it was added to lld).
Wed, Sep 13
LGTM, not sure how we didn't hit this before!
Tue, Sep 12
Wed, Sep 6
Fri, Sep 1
LGTM too other than test target dependent issue Mehdi pointed out.
Tue, Aug 29
Fri, Aug 25
This looks really good, just a few minor comments. But I do have a concern about the test and it not showing the multiple node SCCs that I would expect. Can you investigate?
It would be great to have a bot that catches these.
Aug 23 2017
Getting there, thanks!
Aug 22 2017
Thanks for doing this. Some comments below, but I'd like someone else like Davide or Mehdi to review since I wrote some of this.
Aug 21 2017
Looks really close to me, just some comments on the dummy root node detection. But note I had some comments on D36311 that need to be resolved.
Aug 19 2017
Aug 18 2017
Aug 17 2017
Rather than modifying the index format, can you just set the live root flag on the corresponding FunctionSummary for personality routines when we build the per-module summaries during the compile step? I.e. here: http://llvm-cs.pcc.me.uk/lib/Analysis/ModuleSummaryAnalysis.cpp#278?
Duncan has a good point about replaceOperandWith, please address that (updating to remove patch accept).
Sorry for the delay, I was out all last week and missed this one. LGTM but with one question below about a test change.
Aug 16 2017
Aug 15 2017
Aug 8 2017
Aug 4 2017
LGTM with one last question/nit. Do you have commit access and if not do you want me to submit for you? Do you have follow-on patches that use this about ready to go for review?
Aug 3 2017
Implement Easwaran's suggestion
Overall looks good, just a few minor things below. Do you have a follow up dependent patch you can send for review that shows how one of these attributes will be utilized? I'd prefer to have this committed when we have a user for at least one of these new attributes.
Remove extraneous variable
Aug 2 2017
Fix NDEBUG build - guard declaration of value used only in DEBUG build.
Address review comments
Aug 1 2017
Jul 28 2017
Jul 27 2017
LGTM, with one more comment update noted below. Thanks!
Sorry for the delay. A few comments below. Regarding Mehdi's performance question - I don't expect this to have a major impact as we were only importing aliases in limited cases anyway. We'll see any performance effect in our nightly testing once this goes in, which should surface any surprise effects.
Jul 26 2017
Jul 25 2017
Address review comments
Jul 24 2017
Address review comments, moving handling to gold-plugin (I'd like to get
this one in asap and add the lld support in a follow up).