- User Since
- Apr 27 2015, 11:17 AM (299 w, 4 d)
Fri, Jan 15
Agree, these should just be removed. but I also don't see any uses of disable-lto-vectorization in a test, can that and the corresponding DisableVectorization parameter be nuked as well?
Thu, Jan 14
Use linkonce_odr vtables to illustrate issue with Symbol exportDynamic
Rebase and use ";;" instead of ";" for comments.
Wed, Jan 13
Tue, Jan 12
Haven't looked at the detailed changes yet, but a couple of high level comments/questions below.
Thanks, I'll take a look hopefully today. As I just mentioned in the bug, ThinLTOCodeGenerator has the same issue - will you be migrating that as well?
Thu, Jan 7
Thu, Dec 31
LGTM, I've been bitten by this in new tests as well. We can enable it when needed.
Wed, Dec 30
Couple minor suggestions copied from review for D92900. Also, I think the macho test will fail right now since it references some symbols that don't exist there?
Update per previous comment
I implemented @MaskRay's suggestion and added a bit to convey whether a symbol is exported to the dynamic linker (via --export-dynamic[-symbol=] or --dynamic-list=), and use that to prevent the LTO visibility upgrade for WPD. I added support to both lld and gold plugin, and associated tests. Note that I couldn't use includeInDynsym in lld because that is not set for linkonce_odr symbols that were thus have canBeOmittedFromSymbolTable set (since any referencing module must have it's own copy) - we still want to block the LTO visibility upgrade for those symbols to avoid WPD. So I am using a slightly different interface that more directly checks whether export-dynamic is in effect.
Dec 21 2020
Dec 17 2020
Dec 16 2020
Sorry, completely missed this earlier. Is this fixing a bug? What is the effect on behavior?
Dec 9 2020
From your patch description:
Imported functions and variables get the visibility from the prevailing module.
LGTM with a minor fix: Patch description has a typo, mentions -fno-pass-manager, I think this should be -fno-new-pass-manager?
Dec 8 2020
Dec 7 2020
Subsumed by D92804
Dec 4 2020
Dec 3 2020
Dec 2 2020
Dec 1 2020
lgtm. Thanks for fixing this, and that's a nice cleanup on the existing handling! One request - could you submit the change to the current handling and associated test changes first, with the symver handling/test as a follow on?
Nov 30 2020
Thinking through this some more, I think I was making it too hard. I think we should just be able to collect the symvers out of the inline asm when we do the IR linking. However, not in the place you've modified below, as I note there. Instead, take a look at IRLinker::run(), where it appends the module inline asm strings together if !IsPerformingImport. I think here you could add a case where IsPerformingImport, and just invoke ModuleSymbolTable::CollectAsmSymvers here and directly add any symver you find to the DstM (probably do all this in a helper function). I was thinking that we might need to carry it via module metadata to avoid parsing it during IR linking, but I'm now not sure that's necessary - I don't think it should add significant overhead. Let me know if you run into issues with that approach.
lgtm with a couple of minor nits noted below that you can fix before submitting
Nov 24 2020
While I investigate alternative mechanisms to handle --export-dynamic, I've sent D92060 to add a --no- version of the option, to simplify working around issues when --lto-whole-program-visibility is specified broadly.
Adding another reviewer - @wmi can you take a look? This is a straightforward compile time fix.
Nov 23 2020
Nov 20 2020
From a ThinLTO perspective, no specific concerns as the buildModuleSimplificationPipeline is invoked in both the pre and post LTO link pipelines, so they both get an equivalent change. But there is an issue for regular LTO, noted below.
Nov 19 2020
Beef up test as suggested to check the full path via an assert
Ping - @gkistanova can you take a look and also let me know if there is a way to test the LTO builder change?
Nov 18 2020
Fix test to avoid using /tmp
Add the test
Thanks for adding the Driver test. I was thinking of something to test the CompilerInvocation changes, similar to your test using opt, that ensures the option has the desired behavior when invoked via clang. Looks like there is an existing test clang/test/Frontend/optimization-remark-with-hotness.c that perhaps could be extended or leveraged?
Nov 17 2020
Thanks, generally this seems to be good cleanup. Question on one of the changes below though.
Largely LGTM, except that the clang changes are missing a corresponding test.