[GitHub] [tvm-rfcs] kparzysz-quic commented on pull request #83: [RFC] Create LLVM scope class for use with LLVM libraries

[GitBox <gi...@apache.org>]

kparzysz-quic commented on PR #83:
URL: https://github.com/apache/tvm-rfcs/pull/83#issuecomment-1179203419

   Both `LoadIR` or `ParseIR` take an optional pre-existing `LLVMContext`, and return a pair `{llvm::Module, LLVMScope}`, where the scope object in the pair is newly constructed, and contains the given `LLVMContext` (or create a new one if none was given).  In the first call to `ParseIR`, the caller can take the second element of the pair and just use it as the scope.  In a subsequent call, the caller can simply discard the second element, which will then be automatically destroyed.
   An example of calling `ParseIR` or `LoadIR` when a LLVM scope already exists is in [`HandleImport` in codegen_llvm.cc](https://github.com/apache/tvm/pull/11933/files#diff-f61b04b100f5145f2681340c81d3f2af221239594ed01e2e24896522329ce92cR275).
   
   You bring up a good point about when the scope "takes effect", or binds to LLVM.  I propose that the scope becomes active when a new `LLVMContext` is created (i.e. the scope is really determined by `LLVMContext`, not by the `LLVMScope`).  We could also define "takes effect" to mean that locks (or failure-to-create object) could be acquired.  That would mean that `ParseIR` and `LoadIR` return an active scope.  Also, if they are called with a context argument (meaning that they are called from an active scope), they wouldn't need to create a new context, and so they wouldn't block (if we were to add locks).  In such case they would return another scope object with the same `LLVMContext`.  This shouldn't cause any problems even if both scope objects were used, but could make the code confusing to read.  If we follow the convention that the "extra" scope is to be ignored, this would avoid the confusion.  There may be details to be sorted out about releasing locks, etc, but those are i
 mplementation details.
   
   _Side note: Ideally, prior to the activation of the scope, none of LLVM code or data types would be exposed to the user, but this RFC doesn't try to go that far.  The main concern here is a platform on which saving/restoring LLVM flags could be done._
   
   I also suggest to create two classes: `LLVMScope` and `LLVMTarget`.  The `LLVMTarget` would contain all the LLVM target flags and options, while `LLVMScope` would deal with activation, `ParseIR` and `LoadIR`.  The `LLVMTarget` object could be passed around, copied, etc (without concerns about locking or activating anything), and its purpose would be to have all the target flags/options in one place, and it could only exist once scope has been activated.
   
   Something like
   ```C++
   class LLVMTarget {
   public:
     LLVMTarget(const LLVMTarget&);  // copy ctor, etc.
   
   private:
     friend class LLVMScope;
     LLVMTarget(ctx, values extracted from tvm::Target);
   
     std::shared_ptr<llvm::LLVMContext> ctx_;    // set via the private ctor, passed from LLVMContext
     llvm::TargetMachine *tm;
     ...
   };
   
   class LLVMScope {
   public:
     LLVMScope(const Target& target) {
       // Parse the target into individual attributes, e.g. mtriple, etc. This would allow users
       // to create LLVMScope from Target and modify it before creating LLVMTarget.
       //
       // If the target has non-empty list of LLVM flags, it would be considered "modifying"
       // the global state, otherwise it would be "read-only".  This would allow creating
       // multiple "read-only" concurrent scopes, but only one "modifying" one.
     }
   
     void activate() {
        ctx_ = std::shared_ptr<llvm::LLVMContext>(CreateNewContext(is_read_only));
     }
   
     static std::pair<llvm::Module, LLVMScope> ParseIR(std::string ir) {   // same for LoadIR
       auto ctx = std::shared_ptr<llvm::LLVMContext>(CreateNewContext(false /*assume modifying*/));
       llvm::Module m = llvm::parse_string_etc(ctx, ...)
       LLVMScope scp(ctx, get_target_string(m));   // create an already active scope
       return {m, scp};
     }
   
     LLVMTarget getTargetInfo() {
       ICHECK(ctx_ != nullptr) << "must activate first";
       ...
     }
   
   private:
     static llvm::LLVMContext* CreateNewContext(bool is_read_only) {
        // Create context with appropriate locking
     }
    
     LLVMScope(std::shared_ptr<llvm::LLVMContext> ctx, const Target& target);
     std::shared_ptr<llvm::LLVMContext> ctx_;
   };
   ```
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org