- User Since
- Apr 27 2015, 11:17 AM (216 w, 2 h)
Fri, Jun 14
lgtm for the LTO bits. Suggestion below for comment.
Wed, Jun 12
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.
Wed, May 29
Tue, May 28
Thu, May 23
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.
May 14 2019
May 13 2019
The parent revision now includes both clang and llvm side changes.
Rework using module flags.
May 10 2019
Working on this one again as it previously wasn't causing any issues but just now got exposed in multiple places and started causing correctness issues. Two questions:
- What should be the behavior when merging modules e.g. for LTO. I'm thinking of the following, with multiple module flags to specify different aspects:
- For the NoBuiltinFuncs I'm thinking of a module flag containing the list of strings, with AppendUnique merging behavior, so we get a superset in the merged module
- For the flag to disable all builtins, have a separate module flag with a Max merge (so we get the most conservative behavior)
- For the VectorLibrary, not sure - should it be an Error to merge modules with different libraries?
- While we get the above hammered out, would it be ok to submit this one (and companion clang change) to fix the bug (then remove when the module flags are added)?
Ping - ok if I submit this bugfix?
May 9 2019
Fix LGTM. A couple of requests for comments below. Also, please update title of patch, and add a description to the patch summary.
May 2 2019
Thanks! LGTM with one suggestion below.
May 1 2019
Apr 30 2019
Apr 29 2019
Implement review suggestions
I'm not convinced this is the right approach. Why isn't r323633 working here ("[ThinLTO] - Stop internalizing and drop non-prevailing symbols")?
ping - @pcc any more comments or can this fix go in now?
Apr 25 2019
Address review comments
I am not familiar with this code, so happy with @evgeny777 's review.
Apr 23 2019
Apr 18 2019
Apr 17 2019
LGTM with a minor fix needed below.
Apr 16 2019
Sorry for the delay, just catching up after a vacation.
Apr 8 2019
What part of the patch caused the need to change the internalize action to do promote+internalize in one go?
Apr 5 2019
Just noticed something else looking through it again, question below.
I have mostly small suggestions below. Overall, it looks pretty good and straightforward.
Apr 4 2019
Apr 3 2019
Apr 2 2019
Mar 29 2019
Hi Caroline, I was looking at this patch as I needed to reference it in another discussion, and realized there was no test committed with the patch. Looks like old review comments exist for a test/ThinLTO/X86/builtin-nostrip.ll on a prior version of the patch, but that seems to have disappeared on the version that got committed (probably just a mistake during the commit process since it was a new file). Can you grab that test off the prior diff and commit it now?
Mar 27 2019
Added a gold test so that we can test the native object case too.
Mar 26 2019
This change caused a compile time regression (that I referenced at the end of my summary in D59696). In this case, there are huge numbers of select instructions. After this change, since we now update the ModifiedDT correctly, the loop over the function in CodeGenPrepare::runOnFunction will break after each select is optimized, due to ModifiedDT being set. As mentioned in D59696, even after making the DT build lazy, there is still a large regression because of the number of times we re-walk the function.
Mar 25 2019
Mar 24 2019
Address comments, sync in NFC change r356857.
Mar 22 2019
Mar 19 2019
Sorry missed your earlier update. LGTM. Thanks for doing the measurements!
Committed icmp test in r356463.