[NFC] Drastically Reduce The Scope of the "Global LLVMContext" #31106
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There's no reason the REPL needs to generate its starting module in the global LLVMContext. Give it a local context.
The Remarks Streamer's installation seemed a bit overly complex, so simplify it in a few places: * Refactor sil-opt to install the remarks options into the SILOptions for the SILModule This reduces the parameter bloat in createSILRemarkStreamer. All of this data is freely derivable from the SILModule alone. * Refactor createSILRemarkStreamer into SILRemarkStreamer::create With the new reduction in parameters, we can hide the internal constructor and introduce a smart constructor that vends a unique pointer to clients. * setSILRemarkStreamer -> installSILRemarkStreamer Since the information to create a streamer is now entirely derivable from a module, remove a layer of abstraction and have the module directly construct a streamer for itself. * Give SILRemarkStreamer its own LLVMContext The remarks streamer just needs scratch space. It's not actually "installed" in a given context. There no reason to use Swift's Global Context here. * Give the SILRemarkStreamer ownership of the underlying file stream The SILModule didn't actually use this member, and it seems like somebody needs to own it, so just give it to the remarks streamer directly.
Delete all of the formalism and infrastructure around maintaining our own copy of the global context. The final frontier is the Builtins, which need to lookup intrinsics in a given scratch context and convert them into the appropriate Swift annotations and types. As these utilities have wormed their way through the compiler, I have decided to leave this alone for now.
Contributor
Author
|
@swift-ci test |
Contributor
Author
|
⛵ |
Contributor
|
Hey @CodaFi, thanks for fixing the issues you found in the remark streamer. Unfortunately, the Is there any way to make that work? It also sounds like I might have missed a test for this, but I'm not sure how to add this kind of end-to-end test, that breaks after this patch: or |
Contributor
Author
|
Aha, good to know. In that case, I think we can delay installing this on an LLVMContext until we pair a |
CodaFi
added a commit
to CodaFi/swift
that referenced
this pull request
Apr 21, 2020
Corrects a mistake introduced in swiftlang#31106 I was under the impression that the LLVMContext for an instance of llvm::remarks::RemarkStreamer was somehow just scratch-space. It turns out the ASMPrinters don't gather remarks data from modules, they gather it from the remark streamer associated with the module's context. Thus, we cannot have the module's context be distinct from whatever context the streamer is eventually associated with. In order to bring these two systems back into harmony, introduce a simple ownership contract to SILRemarkStreamer. That is, it starts owned by a SILModule, in which case the SILRemarkStreamer holds onto the underlying LLVM object as the optimizer emits remarks. When it comes time to IRGen the module, then and only then do we install the streamer on the module's context. This tranfers ownership of the underlying LLVM streamer to LLVM itself, so it acts as a consuming operation. When we are about to perform IR Generation, the SILModule will be expiring anyways, so the streamer was already about to be destroyed. We're just putting it to better use doing its job.
CodaFi
added a commit
to CodaFi/swift
that referenced
this pull request
Apr 22, 2020
Corrects a mistake introduced in swiftlang#31106 I was under the impression that the LLVMContext for an instance of llvm::remarks::RemarkStreamer was somehow just scratch-space. It turns out the ASMPrinters don't gather remarks data from modules, they gather it from the remark streamer associated with the module's context. Thus, we cannot have the module's context be distinct from whatever context the streamer is eventually associated with. In order to bring these two systems back into harmony, introduce a simple ownership contract to SILRemarkStreamer. That is, it starts owned by a SILModule, in which case the SILRemarkStreamer holds onto the underlying LLVM object as the optimizer emits remarks. When it comes time to IRGen the module, then and only then do we install the streamer on the module's context. This tranfers ownership of the underlying LLVM streamer to LLVM itself, so it acts as a consuming operation. When we are about to perform IR Generation, the SILModule will be expiring anyways, so the streamer was already about to be destroyed. We're just putting it to better use doing its job.
CodaFi
added a commit
to CodaFi/swift
that referenced
this pull request
May 12, 2021
Our LLVM backend is supposed to have a build-time optimization where we hash the IR and do not proceed with LLVM code generation if the hash matches the one we last used to emit any products. This has never quite worked for a variety of reasons - we fixed a number of those reasons in swiftlang#31106 but this test remained flaky for its entire lifetime. It's really not worth all the trouble to keep investigating this, so I'm removing this test. rdar://77654695
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Swift needed its own global context because LLVM removed the concept of the "Global LLVMContext" upstream a while ago. This context infected a bunch of the compiler infrastructure.
This patch chases all of those global context dependencies back down to a single file: the builtins. Notably,
I also noticed a potential use-after-move in the SILRemarksStreamer construction code that I've fixed by just refactoring the construction interface.
The builtins are still using a global context out of convenience. We could probably just refactor builtin lookup to be a service installed in the ASTContext and give it its own context for scratch space. For now, I'll just preserve the managed static that was here in case there is a better option I'm overlooking.