[LLVM] Create LLVM scope object for use with LLVM libraries #12140
Conversation
This implements RFC 80. See apache/tvm-rfcs#83. Summary of changes: - Created an `LLVMScope` class. Uses of LLVM functions and data struc- tures should be contained within the lifetime of an object of this class. LLVMScope object contains LLVMContext, and implements member functions to deserialize an llvm::Module. - Created an `LLVMTarget` class. Once an LLVMScope object has been created, an object of LLVMTarget class can be created from TVM target string, or Target object for "llvm" target. Once LLVM command line flags are added to the "llvm" target, one of the goals of this object will be to save/restore relevant LLVM global state. Another objective for the LLVMTarget object is to be a single location for all LLVM-related compilation structures and options (such as TargetMachine, FastMathFlags, etc.)
There was a problem hiding this comment.
thanks @kparzysz-quic this looks like a pretty good refactor. ccing a few folks who might be affected @manupa-arm @Mousius @tqchen @junrushao1994 @csullivan @comaniac @Hzfengsy
src/target/llvm/llvm_scope.cc
Outdated
|
|
||
| namespace { | ||
| bool InitializeLLVM() { | ||
| static std::atomic_flag initialized = ATOMIC_FLAG_INIT; |
There was a problem hiding this comment.
why do we need the atomic here? i don't think we typically support multithreaded operation
There was a problem hiding this comment.
The previous initialization code (in llvm_common.cc) had an atomic flag in it (although in a bit more complex scheme), so I kept it. If you still think it should be removed, let me know---I have no problem changing it.
Edit: llvm_common.cc is not shown in the diff. You need to show it manually for the link to work.
Move empty implementations of EnterWithScope/ExitWithScope to header since it helps see that these functions are only stubs.
|
Pinging all interested parties... Are there any concerns that haven't been addressed? |
|
Is it ok to merge? |
|
@tqchen: are you ok with this? |
|
@tqchen would you like to review this? Thanks a lot! |
|
Sorry for delayed reply. Approving as it is a good chunk of improvement. It would be great to discuss naming and documentation a bit. When I see LLVMScope, I was expecting LLVM to taken in-effect. But in reality, we can have multiple With<LLVMTarget> target(scope, ...)So LLVMScope is not exactly an unique RAII object. Sorry that I missed it when reading the RFC but it becomes obvious when reading implementation of LLVMModule. Perhaps we should consider clarify and rename (it should be an LLVMContext?) in this case. Followup actions:
|
|
Originally the scope took effect immediately, but there was an ordering problem with deserializing LLVM IR: LLVM flags would only be known once we've made LLVM calls, and once we've created an LLVM context. So later in the RFC discussion I proposed separating the scope into two parts: one part that encloses the LLVM context creation, and one that saves/restores LLVM options. I'd be happy to make further improvements to this, as we decide going on in the future. |
|
Thanks @kparzysz-quic , my guess is that we just need naming and documentation clarifications. e.g. the perhaps we can change name of LLVMScope to LLVMContext to capture the fact that it is " LLVM context creation," and the scoping happens in |
I'm a bit concerned that it could create confusion given that LLVMContext is name of LLVM type. Let me think a bit, maybe I can come up with a different name. |
|
What do you think about |
|
LLVMInstance sounds good |
|
I edited the commit message to use |
…2140) This implements RFC 80. See apache/tvm-rfcs#83. Summary of changes: - Created an `LLVMInstance` class. Uses of LLVM functions and data struc- tures should be contained within the lifetime of an object of this class. LLVMInstance object contains LLVMContext, and implements member functions to deserialize an llvm::Module. - Created an `LLVMTarget` class. Once an LLVMInstance object has been created, an object of LLVMTarget class can be created from TVM target string, or Target object for "llvm" target. Once LLVM command line flags are added to the "llvm" target, one of the goals of this object will be to save/restore relevant LLVM global state. Another objective for the LLVMTarget object is to be a single location for all LLVM-related compilation structures and options (such as TargetMachine, FastMathFlags, etc.)
This implements RFC 80. See apache/tvm-rfcs#83.
Summary of changes:
LLVMScopeclass. Uses of LLVM functions and data structures should be contained within the lifetime of an object of this class. LLVMScope object contains LLVMContext, and implements member functions to deserialize an llvm::Module.LLVMTargetclass. Once an LLVMScope object has been created, an object of LLVMTarget class can be created from TVM target string, or Target object for "llvm" target. Once LLVM command line flags are added to the "llvm" target, one of the goals of this object will be to save/restore relevant LLVM global state. Another objective for the LLVMTarget object is to be a single location for all LLVM-related compilation structures and options (such as TargetMachine, FastMathFlags, etc.)