- User Since
- Nov 4 2014, 3:46 PM (197 w, 1 d)
Tue, Jul 31
Can you add a bit more explanation other than just saying fixing PR38372?
Fri, Jul 20
For the old API, I don't think it is reasonable to expect user or linker to pass in the correct preserve list. There is likely no knowledge outside the LTO modules that these lib funcs are needed.
Wed, Jul 18
Jul 16 2018
When importing a variable definition when the ref has this bit set, it can be imported as a local copy, without promoting the name/linkage type. That will avoid the need to do any modification to the optimization passes. On the exporting side, we could do prevent the promotion if we knew that all modules importing a reference also imported the referenced variable definition. It does however look like we should be importing all non-interposable linkage constant variables that are referenced, which should be all we care about for this anyway.
Jul 10 2018
Jul 9 2018
Jul 6 2018
Add testcase using 6.0 bitcode.
Address review feedback
oops, accidentally opened a new review. closing.
In that case, we can enable verification on checked-in bitcode files as a follow-up. How about testing this by round-tripping textual IR through llvm-as and llvm-dis? You just need a function, alias, and global var with local linkage and without dso_local set, right?
Jul 2 2018
It is easier and cleaner if I just fold everything into getOSVersion.
handle *-apple-macosx target option
Rebase the commit correctly
Update patch. Use a better API.
Unfortunately, I wasn't able to write a test for this because the host triple in the configuration can either be x86_64-apple-darwin* or x86_64-apple-macosx*, but the one used passed by driver is always macosx one. I can't reliably compare those two.
Jun 29 2018
Does the iteration order of those map matters other than changing the hash? Will it change symbol resolution or importing different function?
Jun 22 2018
Jun 13 2018
Jun 11 2018
I am not working on lld but this looks good for me. I like this solution better than the alternative you mentioned.
May 23 2018
May 14 2018
I think it is easier to unregister all the signal handlers in the beginning of the llvm_shutdown() since you should be done with all the work when you call llvm_shutdown(). This can also allow safe access ManagedStatics in signal handler, which I don't know if we have more of those other than the lock you are trying to fix. The only difference might be that if an interrupt is triggered when freeing ManagedStatics, llvm signal handler will not be used.
May 10 2018
Are you planning to upstream the module pass to justify this to be part of Support library?
Apr 23 2018
Thanks for working on this Jonas. Duncan and Adrian has already covered pretty much all I want to say. A small additional comments.
Apr 19 2018
Apr 18 2018
Address review feedback
Apr 16 2018
Address review feedback
Address review feedback. Fix typos and comments.
Thanks for reviewing Erik!
Apr 10 2018
Apr 9 2018
Mar 30 2018
LGTM with some feedback inline.
Mar 21 2018
I am not trying to discuss which english word is best here. My point is simply:
- macros are evaluated during compile time
- "host"means either the platform you compiled on during compile time or the platform you run on during the runtime
- __is_host_* is not a good name, because it is misleading as it either implies "runtime" as a compile-time constant, or indicates the wrong platform.
It is not about matching command line name to builtin marco name. "target" is the platform we are compiling for, whether it is host or device or something else. It is a different concept when you talking about cross-compiling, which "target" is strictly not host and "build" or "host" doesn't matter to compiler at all.
I disagree. I think "target" is the correct name, even for cross compiling. For something compiled with -target foo, we are consistent calling it "target" during compile time. It only becomes appropriate to call it "host" during the runtime of the executable. There is no such concept as "host" when you are doing cross compiling.
Mar 19 2018
Mar 15 2018
If we all agree that dropping local_unnamed_addr is invalid, I can abandon this. Thanks for the feedback!
I might have missed something. Can you be more specific about how it is going to work?
The solution is to use canBeOmittedFromSymbolTable from include/llvm/Object/IRSymtab.h.
The symbol table is computed earlier and so the above predicate still returns 1. Your examples work with lld and ThinLTO if you want to check the implementation details.
So the original of this patch is thinLTO is promoting linkonce_odr to weak_odr. Think the following two situations:
Mar 13 2018
Additional comment, I didn't realize llvm-nm now prioritize to print bitcode symbol table, rather than the symbol table from the object file. There should be an option to choose which one to see and maybe also an option to see both.
Thanks for fixing this. LGTM.
Mar 1 2018
The idea is to prevent LTO (ConstMerge pass) from merging global constants that has the same value, which has the same side-effect as linker merging two weak symbols that has ODR violation.
Feb 28 2018
LGTM. And yes, order doesn't matter.
Feb 26 2018
The API change is fine for ld64. But you need to do the following when you update LTO C API:
- You need to declare the interface in include/llvm-c/lto.h
- You need to bump LTO_API_VERSION in that header and document the new API is available since the new LTO_API_VERSION.
- You need to update tools/lto/lto.exports. This is the export list used by ld64 and only the interface listed in that file will be exported on darwin platform.
Feb 19 2018
Michael doesn't have commit right yet so I will commit this for him.
Feb 16 2018
I am using local_unnamed_addr as an example. Maybe you are right but I think my point is still valid. That is, LTO optimization and code generation can modify the symbol table from the bitcode file. It is already the case that symbols can be internalized, removed, backend can also insert new symbols. Unless there is a rule saying LTO passes cannot modify the linkage or visibility or any other attributes to the symbols, linker cannot trust whatever the bitcode symbol table says and firmly it will not be changed for some good reason.
I think this approach is much better. Can you update the title because it is not really a version bump anymore? From my understanding, this is good enough for backwards-compatibility. Thanks!
Feb 15 2018
Generated by mistake. Abandon and I will reformat the patch.
Feb 9 2018
Move the check later so it is only check when needed.
Feb 6 2018
This indeed affects how ld64 interact with libLTO. If user didn't specify a value, ld64 will always call thinlto_codegen_set_cache_pruning_interval with interval 0. This is currently a no-op so it will use whatever the default value in libLTO. With this change, it will be force pruning.