- User Since
- Apr 27 2015, 11:17 AM (181 w, 4 d)
Wed, Oct 17
Great! I only had a chance to take a cursory look so far (attending the llvm dev meeting), but a few questions/comments based on a very quick look this morning.
Tue, Oct 16
Mon, Oct 15
This LGTM, but please wait for @brzycki
Code looks fine now but just realized there is no test. You can probably create one test with a function that can normally be internalized, and then try it for the 4 combinations of LTO/ThinLTO and new/old LTO with and without your option. See test/ThinLTO/X86/internalize.ll for a ThinLTO-specific internalization test that tests both the old (via llvm-lto) and the new (via llvm-lto2) LTO APIs.
Are both fixes necessary to fix the issue (the one for back propagation and the one to bail out if the entry block is cold), or is either one sufficient? The patch description only mentions the former.
The ThinLTO code is shared between the new and old LTO APIs (via thinLTOInternalizeAndPromoteInIndex), but the regular LTO internalization is not. Suggest sharing this option with lib/LTO/LTOCodeGenerator.cpp and guarding the regular LTO internalization there too to cover the old LTO API case.
It's hard to review the changes the way they are currently split up. E.g. This patch adds some fields to the index, without using them or testing them. Those changes are in a different patch or two. It would be good to isolate all the changes that create and set these new index fields and test them via dumping the index to assembly and looking for expected values. Then have another patch that uses them in the FunctionImporter. Etc.
However, Internalizer currently removes non-externally-visible COMDATs when internalizing global objects. The commit which added this behavior https://github.com/llvm-mirror/llvm/commit/4a6d99b0a96d4eb27d89c22e33981ff0344c5737 states that passes like GlobalDCE can legally remove individual COMDAT members.
Thu, Oct 11
LGTM with a minor comment fix noted below.
Wed, Oct 10
As discussed on bug, this is already fixed in new LTO API so best to be consistent, regardless of whether users should build with -fno-builtins.
Tue, Oct 9
Mon, Oct 8
Fri, Oct 5
This seems reasonable - we should be able to keep ODR non-prevailing defs around to optimize with. I assume this is a missed optimization fix and not a correctness fix (i.e. the build currently succeeds and builds a correct binary)? Question about that below.
Can you upload the patch with context? See the following for instructions:
I'll respond more on the bug (especially since I can't reproduce the actual link time error), but I think the right way to fix these types of issues is to compile with one of -ffreestanding, -fno-builtin, or -fno-builtin-log (or whatever is being overridden), so that clang doesn't translate the call to an intrinsic. Clang translates to the intrinsic assuming it is calling the builtin, which means it can assume specific semantics for the call, and there is nothing stopping the compiler from translating that intrinsic call to e.g. an inline expansion of the function (that currently happens e.g. for memcpy and some other builtins).
Tue, Oct 2
LGTM with one minor fix noted below. Thanks! I will give it a shot with our apps which use the new PM.
Mon, Oct 1
Thu, Sep 27
Wed, Sep 26
Tue, Sep 25
Mon, Sep 24
Fri, Sep 21
LGTM with a minor change suggested.
Note that if you add a line like:
Thu, Sep 20
I think this is an NFC (no functional change) cleanup, right? It would be good to add that in the description - i.e. put "[NFC]" at the end of the summary first line.
I see that there is a new Pass Manager version of your new pass, but I don't see that it is ever being enabled in the new pass manager pipeline (or tested). Are you planning to add that?
Sep 19 2018
In your summary, I think you mean it can copy metadata (not attributes)?
Can you update description to point out that this is an issue because we will import the alias as a copy of aliasee?
Sep 18 2018
Sep 17 2018
Sep 14 2018
LGTM with nit.
Sep 13 2018
Sep 7 2018
Sep 4 2018
Split into 2 paragraphs as suggested
Change to use a map to a vector of matching type id, summary pairs
Aug 31 2018
Change the TypeIdMap in summary to be indexed by GUID.
Aug 30 2018
Aug 27 2018
Aug 24 2018
Aug 23 2018
Thanks. Can you fix the same code in EmitAssemblyWithNewPassManager?
Aug 21 2018
LGTM. Thanks for the cleanup!