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

Export jl_method_set_source. #49236

Merged
merged 1 commit into from
Jun 16, 2023
Merged

Export jl_method_set_source. #49236

merged 1 commit into from
Jun 16, 2023

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Apr 3, 2023

I need this to re-implement jl_make_opaque_closure_method for GPU-compatible opaque closures. Alternatively, exporting jl_make_opaque_closure_method would work too since it doesn't actually do anything GPU-incompatible, but I figured it is cleaner to reimplement that method since nothing about OpaqueClosures is supposed to be GPU-compatible.

@maleadt maleadt added the domain:gpu Affects running Julia on a GPU label Apr 3, 2023
@maleadt maleadt requested a review from Keno April 3, 2023 16:24
@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 4, 2023

jl_new_opaque_closure_from_code_info is already exported, and supposed to take care of some of the correctness details with inference I believe around doing this?

@maleadt
Copy link
Member Author

maleadt commented Apr 4, 2023

As per Keno, nothing about OpaqueClosures is portable, so I don't think we should be using the existing machinery for a GPU-compatible equivalent. For example, jl_new_opaque_closure_from_code_info inserts into the CPU caches, which we don't have any use case for. Also, the (experimental) GPU equivalent isn't 100% semantically equivalent to the CPU opaque pointers, so I don't think we even want to use the heuristics that are in place (gated on m.is_for_opaque_closure).

@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 4, 2023

This just looks very unreliable though, given the number of unstable APIs that are being ccall'd in that PR

@maleadt
Copy link
Member Author

maleadt commented Apr 4, 2023

There's not that many, right? GPUCompiler is already pretty dependent on internal APIs, so that's nothing new.

Anyway, the use case is to compile and call DAECompiler-generated IR from a GPU kernel. Essentially I'm just wrapping a CI and MI around that IR and putting it in GPUCompiler's cache (I don't care much about the OpaqueClosure semantics at this point). Do you have a better approach in mind?

@maleadt
Copy link
Member Author

maleadt commented Jun 14, 2023

Bump. @vtjnash, can you explain how else you'd implement something like JuliaGPU/CUDA.jl#1853? AFAIU, the only additional internal method I'm calling there is jl_method_set_source, so what makes that very unreliable, and how would you improve it?

@maleadt maleadt force-pushed the tb/export_method_set_source branch from a67dd8e to c1fc10a Compare June 15, 2023 18:20
@Keno
Copy link
Member

Keno commented Jun 15, 2023

I would suggest we just merge this as is, so we can get experience with OpaqueClosure semantics on GPUs. Clearly there's something not quite comfortable with how opaque closures (on CPU as well) are using Method objects, but I think it'll be easier to know how to refactor that once we've had a chance to play with it. Of course these are internal and unstable APIs, but we're also talking about the GPU code here - calling internal and unstable APIs is kinda the whole thing.

@maleadt maleadt merged commit 71c5115 into master Jun 16, 2023
1 check passed
@maleadt maleadt deleted the tb/export_method_set_source branch June 16, 2023 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:gpu Affects running Julia on a GPU
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants