Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Draft] Pass JITUserContext::user_context to custom handlers. Add UserContext param. #6278

Closed
wants to merge 2 commits into from

Conversation

mzient
Copy link

@mzient mzient commented Oct 1, 2021

The draft intended as a live demonstration of ideas discussed in #6255. It is still not ready for review, let alone merging.

Problem recap: currently, it's impossible to pass per-launch user context and handlers to JIT compiled functions. This becomes problematic when the target is an accelerator (e.g. CUDA device), where it may be desirable o execute the code in user-provided context. It's especially important when running in multi-device systems.
It also decouples the user context passed to custom JIT handlers from JITUserContext.

This draft explores possibilities of passing user context to JIT-compiled code.

Usage:

struct MyMemPool {
  // ...
  void *allocate(size_t);
  void free(void *);

  static MyMemPool &get(int device_id);
};

void *my_malloc(void *pool, size_t size) {
  return ((MyMemPool*)pool)->allocate(size);
}

void my_free(void *pool, void *mem) {
  return ((MyMemPool*)pool)->free(mem);
}

int main() {
  Func f("my_func");
  Param<float> scale;
  // define the function
  f.set_custom_allocator(my_malloc, my_free);
  f.compile_jit();

  // now the function can run in parallel with 2 different allocators
  
  std::thread t1([&]() {
    UserContextParam uc;
    ParamMap pm1({
      { scale, 2 },
      { uc, &MyMemPool::get(0) }
    });
    Buffer<uint8_t> out_buf1( /* sizes here */);
    f.realize(out_buf1, pm1);
  });

  std::thread t2([&]() {
    ParamMap pm2({
      { scale, 2 },
      { uc, &MyMemPool::get(1) }
    });
    Buffer<uint8_t> out_buf2( /* sizes here */);
    f.realize(out_buf2, pm2);
  });

  t1.join();
  t2.join();
}

Signed-off-by: Michał Zientkiewicz [email protected]

@zvookin
Copy link
Member

zvookin commented Oct 1, 2021

I need at least a description of how this is proposed to work. It likely should be in comments in the files extending the APIs. There is no test to show usage. Until this is addressed, I cannot do much with this PR.

I think the basic idea is ok, but the main reason you're having problems with the existing thing is that there wasn't enough thinking about use cases upfront. Even ParamMap is insufficiently documented that I keep having to justify its existence. (It's purpose is to provide an enduser accessible way to make JIT, realize, calls that take parameters in a threadsafe way.)

@mzient
Copy link
Author

mzient commented Oct 4, 2021

@zvookin Thanks for the comments.
This code is still very much in draft state - its intended purpose was to serve as an illustration for the discussion of the proposed feature rather than an actual pull request. When it's ready for review, I'll convert it to a regular PR. If this generates too much noise in your systems (I see that there were some CI jobs running, for example), please let me know - I'll close it and re-open at a later stage.

@zvookin
Copy link
Member

zvookin commented Oct 4, 2021

Per "its intended purpose was to serve as an illustration for the discussion of the proposed feature rather than an actual pull request"...

Expecting folks to figure out the issues that need to be discussed from the implementation alone is a lot to ask. Some sentences explaining what you are trying to do, decisions you've made, and perhaps 10 or 20 lines of code showing how this will be used, even if it doesn't compile or work yet, would go a long way toward facilitating the discussion needed. I believe these would also assist you in your work as well.

* Apply clang-tidy and clang-format

Signed-off-by: Michał Zientkiewicz <[email protected]>
@mzient
Copy link
Author

mzient commented Oct 5, 2021

I've added a usage example and linked the issue. We can also move the discussion here, if that's a preferred workflow in your project - or I can even close this draft until it reaches next readiness level.
I'm sorry if I made myself too much at home here, bringing with me the customs from my project, where drafts are rarely proactively looked at.

@mzient mzient changed the title Pass JITUserContext::user_context to custom handlers. Add UserContext param. [Draft] Pass JITUserContext::user_context to custom handlers. Add UserContext param. Oct 5, 2021
@steven-johnson
Copy link
Contributor

Is this PR still active? Should it be closed?

@mzient
Copy link
Author

mzient commented Nov 17, 2021

Is this PR still active? Should it be closed?

I think it can be closed - similar functionality is provided by #6313, which has already been merged.

@mzient mzient closed this Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants