- User Since
- Nov 4 2014, 3:46 PM (219 w, 5 d)
Thu, Jan 17
Not sure about gn part and if that has any impact. Otherwise, LGTM.
Fri, Jan 11
Fix the comment
I was planning to add a test but I am not sure how to check the file type of temporary files.
Mon, Jan 7
Dec 17 2018
Thanks for the clarifications. Would it be possible to utilize ThinBackendProc for this instead of a new CodeGenManager class? I.e. make a new derived version that does the index file write and spawns the local processes? The advantage is that it would start converging the implementations. And I think this could aid in refactoring suggested below to avoid duplication. Another advantage is that both LTO API's would have access to all backend implementations (in process, write indexes and exit, write indexes and use local processes, etc).
Other than a small design choice commented inline, LGTM.
Dec 13 2018
See comments inline.
Dec 12 2018
I think I missed something before. LGTM now!
Don't we need IR support for this as well? sdk version is not in the triple so it is going to get lost when building from IR. Maybe use a metadata node?
Thanks for taking a look. This patch is adding the customization points for thinLTO legacy API, which the code generator constructs clang invocations to do code generation. There is no dependency on any build system here and it only has a prove of concept codegen manager which invokes clang directly and collect the result back. You can replace this codegen manager with any protocol that is needed to talk to build system to run clang codegen.
XPC is the way to send information between process on Darwin, which is probably what we are going to use to talk to build system. If interested, I can post a patch which have example how to construct XPC communications, but there isn’t a build system you can use to listen on the other side to run the job yet.
When I say there are no code change for linker, I really mean there is no need to change a single line of code (maybe we need to add an API to select codegen manager in the future). ld64 really has a different approach using C API, which it tries to map the object file output back to the bitcode it gets as input. Terminating and relaunching the linker might has unexpected semantic changes for LTO. In the long run, maybe ld64 needs to design a new set of APIs to use the new C++ APIs but this is out of scope of this patch.
Ping. Is there any comments of implementing C API in this approach?
Dec 11 2018
Dec 10 2018
This is upstreamed by Saleem already
Nov 29 2018
Nov 26 2018
I did some experiment and put some more thought into this patch. Not hashing PreservedSymbol passed to the C API is fine because the reason you mentioned but freestanding/no-builtin bits stills need to be handled separately in current status. That should be a different discussion so this patch is LGTM.
Nov 16 2018
I wonder freestanding is still an issue after r316819. There is a function attribute for that now so it should be respected if the module is built with -ffreestanding and it should create different hash. I will do some digging to see if I can find the original report and how they setup the build. @mehdi_amini, do you remember the original problem?
Nov 14 2018
Steven - you had a suggestion around callers to getMDNodeFwdRefOrNull, but after this change there will be no more callers to that. So should I go ahead and submit this one, then remove that interface completely in a follow up? Or do it in this patch?
My thought is that this patch should make bitcode reader more robust when lazy loading, which is always a good thing. If this is a performance regression, the regression is coming from how debug information is generated. If there are no such forward-ref in the metadata, there is no slowdown with this patch. We can investigate performance regression post commit if needed.
Nov 13 2018
I reverted this in r346768.
Nov 12 2018
This commit causes our internal bots failed to bootstrap clang. The error we are getting is:
"gCrashRecoveryEnabled (.llvm.1401930837577591816)", referenced from: clang::ParseAST(clang::Sema&, bool, bool) in 286.thinlto.o
I have a bit trouble to reproduce the problem but it is 100% reproducible on our bots. I suspect the issue is ThinLTO cache reuse because if I clear the cache then the link will succeed. I will update if I can reproduce and pinpoint the issue.
Nov 5 2018
Oh, I see. This is not really related to the debug information quality. It is a pure metadata lazy loading problem. LGTM.
Oct 25 2018
Oct 11 2018
You need to do a release build without assertion. Local names like parameters are dropped from IR to save space/memory.
I think you just need to check the parameter name or the implementation in your test case.
Oct 9 2018
Thanks for posting this. I will start with some high level comments.
Sep 26 2018
Sep 24 2018
Thanks for doing this!
Sep 14 2018
Sep 13 2018
Also a bit clarification why ModuleToDefinedGVSummaries can have missing entries, because there can be an empty module, or a module has no GV in it. And this happens during thinLTO WebKit.
For example, in WebCore MacOS build, UnifiedSource26-mm.o has empty text and data section. Its object file only has debug info.
Fix up my previous patch.
I can't find a super clean solution for this. Here is one possible solution, which is to iterate and insert the entry before entering thread pool
Sep 7 2018
Sep 5 2018
I do prefer the current approach especially on Darwin. Some concerns of switching to use "-L + -l" are:
- clang and compiler-rt are rev-locked. Inferring the compiler-rt from resource-dir and passing to linker with the full path can prevent mistakes of mixing them up.
- This change does change linker semantics on Darwin. ld64 treats the inputs from command line with highest priority. Currently ld64 will use compiler-rt symbols before any other libraries passed in with -l flag. If clang decide to pass compiler-rt with -l flag, any other libraries (static or dynamic) that passed with -l flag will takes the priority over compiler-rt. This can lead to unexpected behavior.
Sep 4 2018
Remove the declaration in ThinLTOCodeGenerator.h?
I dont mind removing codege(). This interface exists since the first prototype and no longer exposed through C API. It seems to have the same model of optimize(), which is also not thread safe. optimize() is still used by llvm-lto but not in thread context.
I don't think we need another instance of TMBuilder. Can we just call codegenModule directly from run()? TMBuilder should be initialized already from addModule.
Aug 23 2018
I feel like this is a much tricky situation than just new and init. Following example is the same situation.
__attribute__((objc_root_class)) @interface NSObject - (void) foo; - (void) bar; @end
Jul 31 2018
Can you add a bit more explanation other than just saying fixing PR38372?
Jul 20 2018
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.
Jul 18 2018
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.