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

Remove arraylist_t from external native code APIs. #56693

Merged
merged 1 commit into from
Nov 28, 2024

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Nov 27, 2024

GPUCompiler.jl used to rely on calls to the lookup function (mapping MIs to CIs) to keep track of MIs used during compilation, which Enzyme.jl then uses for... whatever it does. After #54899, the IR already contains CIs, resulting in this not being reliable anymore, as the lookup callback isn't invoked for every CI anymore.

I noticed that we already had accessor functions for fetching the list of MIs involved in a native compilation, jl_get_llvm_mis, however these functions currently use arraylist_t objects that are not usable to external consumers like GPUCompiler.jl. This PR switches those APIs over to a more traditional C style, passing pointers to the number of elements and the underlying data.

I guess it may be better for Enzyme.jl to consume CIs instead of MIs so that we don't need to maintain this mapping, but I focussed first on making the existing logic work again, and generalizing the current APIs doesn't seem to have much of a drawback.

@maleadt maleadt added the gpu Affects running Julia on a GPU label Nov 27, 2024
@maleadt maleadt requested review from vtjnash and gbaraldi November 27, 2024 08:06
This makes them usable for GPUCompiler.jl and other external consumers.
@maleadt maleadt force-pushed the tb/native_code_arraylist branch from 8128473 to 2bf87af Compare November 27, 2024 09:19
@maleadt
Copy link
Member Author

maleadt commented Nov 27, 2024

But why? These functions are used in staticdata.c, so in C code. Using Array's from there doesn't work, and I don't see the value of using a Memory only to then just have to copy data out into the arraylist_t functions that already live there (such as jl_rebuild_methtables).

@gbaraldi
Copy link
Member

Yeah in a second thought I realized that it would be annoying, so I just deleted my comment 😄

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. It used to be arraylist_t was a good way of expressing this, since C still doesn't have this ability in the stdlib :/

@maleadt maleadt merged commit 9162b14 into master Nov 28, 2024
7 checks passed
@maleadt maleadt deleted the tb/native_code_arraylist branch November 28, 2024 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gpu Affects running Julia on a GPU
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants