- User Since
- Apr 27 2015, 11:17 AM (225 w, 1 d)
I had some time to work on this finally late last week. I decided the most straightforward thing was to implement the necessary interface changes to the TLI analysis to make it require a Function (without any changes yet to how that analysis operates). See D66428 that I just mailed for review. That takes care of the most widespread changes needed for this migration, and afterwards we can change the analysis to look at the function attributes and make a truly per-function TLI.
Thu, Aug 15
Thanks for fixing this. Comment on the test case below.
Wed, Aug 14
Mon, Aug 12
I believe this same fix was applied in r368281, and that this is now obsolete. (Sorry for the delayed review, I was out last week)
Fri, Aug 2
LGTM with suggestion below.
Thu, Aug 1
Wed, Jul 31
I just discovered that there was a nearly identical patch to do this a few years ago, which generated a bunch of discussion and it seems that there was an agreement not to do this (relating to not exposing internal options through the C API, but I haven't parsed it in detail yet). See https://reviews.llvm.org/D19015
Can you add a test case?
Ping - @pcc any more comments?
Tue, Jul 30
Mon, Jul 29
Wed, Jul 24
Suggest sending this change with patch that uses and tests it.
Tue, Jul 23
Jul 19 2019
Going with a different clang-side approach (see D65009).
Jul 16 2019
Jul 15 2019
Jul 12 2019
LGTM with the comment fix from @MaskRay
Jul 11 2019
LGTM, but please wait for @pcc to make sure he is satisfied.
I think it would be good to port the file type deduction to clang (possibly as a follow on), to make these consistent.
Jul 8 2019
Jul 3 2019
LGTM. Confirmed this fixes my issue. FTR I am building on a 64-bit platform.
Jul 2 2019
@pcc can you take a look? The same failure just popped up in the new devirt test I added today (test added in r364960, workaround added in r364997).
Address review comment, and update test for recent auto hide summary changes
LGTM with one minor comment nit below. Thanks!
Jun 29 2019
Jun 28 2019
Thanks! I think this makes sense, and since this is now the same as what is done in the old LTO API (ThinLTOCodeGenerator.cpp), thinking it would be better to extract into a common helper in LTO.cpp that is called from both places (that way if we find a way to improve the sorting, e.g. some of the other metrics @mehdi_amini mentioned, it only has to be changed in one place).
I am getting failures when trying to link these tests, due to missing symbols, e.g.:
Jun 26 2019
ping - @pcc any more comments?
Jun 25 2019
I was looking at flags in this file and noticed there is a typo in the flag added here (condsider-local-interval-cost should be "consider-...", note the extraneous "d" in the flag). Also, there doesn't seem to be any test utilizing this flag. Should it be removed?
Jun 21 2019
Adding Wei who works on SamplePGO on our end currently.
Jun 20 2019
Thanks! A bunch of misc comments/suggestions below.
Jun 19 2019
I didn't get very far today, will finish the review tomorrow morning.
Jun 14 2019
lgtm for the LTO bits. Suggestion below for comment.
Jun 12 2019
Interesting - does this mean we just encoded it with a VBR 4 (what numrefs was to be encoded with), and everything else just got shifted down in the encoding (so immutablerefcnt got encoded as the first VBR 8 in the array at the end)? Unfortunate that there isn't a way to test for this.
May 29 2019
May 28 2019
May 23 2019
As per D61634, we are going to use function attributes instead.
May 17 2019
May 15 2019
There is discussion of using function attributes to control this instead, see https://reviews.llvm.org/D61634.
I'll hold off on making the planned changes here until the overall approach is decided.