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

Add a script that converts FLINT headers to julia types #2010

Draft
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

lgoettgens
Copy link
Collaborator

@lgoettgens lgoettgens commented Jan 30, 2025

This is a first step towards #1875.

I used some techniques described in #1889, but extended them and automated everything.

The only file that is supposed to be edited is the FlintCTypes_template.jl. The script then searches for the mentioned header files in the FLINT_jll artifact, strips away C-specific things like include guards, and converts all structs, enums, and typedefs to equivalent julia constructs.

This change on its own is non-breaking, as I don't change any already existing types (that's planned in a next PR). The new file isn't even included yet.

cc @albinahlback

@lgoettgens lgoettgens requested a review from fingolfin January 30, 2025 13:16
Copy link

codecov bot commented Jan 30, 2025

Codecov Report

Attention: Patch coverage is 64.28571% with 5 lines in your changes missing coverage. Please review.

Project coverage is 88.27%. Comparing base (82193e6) to head (29471dc).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/flint/FlintCHelpers.jl 66.66% 4 Missing ⚠️
src/flint/FlintCTypes.jl 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2010      +/-   ##
==========================================
- Coverage   88.31%   88.27%   -0.04%     
==========================================
  Files          98      100       +2     
  Lines       36126    36195      +69     
==========================================
+ Hits        31903    31951      +48     
- Misses       4223     4244      +21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thofma
Copy link
Member

thofma commented Jan 30, 2025

Do you plan to handle unions? I noticed that fq_default is missing.

(And the _template should probably be moved to etc/ too.)

@lgoettgens
Copy link
Collaborator Author

Do you plan to handle unions? I noticed that fq_default is missing.

Didn't spot that, I'll look into it.

@lgoettgens lgoettgens marked this pull request as draft January 30, 2025 13:42
@lgoettgens
Copy link
Collaborator Author

Do you plan to handle unions? I noticed that fq_default is missing.

unions now work as well (at least typedef union {...} fq_default_struct now results in a julia struct with a single NTuple{sizeof-computation, UInt8} field with intransparent memory usage.

After talking to @albinahlback about an hour ago, I now include some more header files and start with all content, but strip away everything that does not match any of the regexes. This then gets rid of all preprocessor instructions, function signatures etc.
Furthermore, I already upgraded this branch to work only with FLINT 3.2.0 (or rather the rc1 of that) as there are some refactorings included that avoid the need to include gmp.h as well.

as we could extract structs from large block comments by accident
@lgoettgens lgoettgens marked this pull request as ready for review January 30, 2025 19:42
@lgoettgens lgoettgens requested review from thofma and removed request for fingolfin and thofma January 30, 2025 19:42
@lgoettgens lgoettgens marked this pull request as draft January 30, 2025 19:43
Copy link
Member

@thofma thofma left a comment

Choose a reason for hiding this comment

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

Looked at some of the more complicated things, and I did not spot problems. But I did not check everything.

# Most of this file is generated from FLINT's header files.
# Do not edit manually, only the corresponding `etc/*_template.jl` file should be edited.

# This file was generated using FLINT_jll v300.100.301+0.
Copy link
Contributor

Choose a reason for hiding this comment

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

This states that it was generated with FLINT v3.1.3-p1 (I suppose), but further down it says v3.2.0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FLINT_jll v300.100.301 is actually FLINT v3.1.3 (everything times 100), but with an additional bump when rebuilding with aarch-*-freebsd support (cf. JuliaPackaging/Yggdrasil#10029).

This will automatically fix itself once FLINT 3.2.0 is released and the corresponding jll is built. The way that Overrides.toml work in julia is that I basically inject the FLINT 3.2.0-rc1 build results into the FLINT_jll v300.100.301 package (locally). So this is just an intrinsic problem with how overrides work, and nothing for me to fix. (One should just not regenerate this with Overrides active)

src/flint/FlintCTypes.jl Outdated Show resolved Hide resolved
src/flint/FlintCTypes.jl Show resolved Hide resolved
Comment on lines 776 to 786
const n_poly_t = Ptr{n_poly_struct}

const n_fq_poly_struct = n_poly_struct

const n_fq_poly_t = n_poly_t

struct n_bpoly_struct
coeffs::Ptr{n_poly_struct}
alloc::slong
length::slong
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure whether you actually want these things. Perhaps you do. But if you would like to avoid these stuff, I would encourage you to only translate stuff that you really need. Unless it is documented, it can break.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Filtering seems kind of hard. I would rather document the breakage possibilities in Nemo and not count this file into SemVer guarantee.

Comment on lines +709 to +715
uniondata::NTuple{maximum(sizeof, (
fq_mat_t,
fq_nmod_mat_t,
fq_zech_mat_t,
nmod_mat_t,
fmpz_mod_mat_t,
)), UInt8}
Copy link
Member

Choose a reason for hiding this comment

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

Just wanted to mention that I'd been thinking about doing something akin to this (separate from this PR) to allow more optimized code:

Suggested change
uniondata::NTuple{maximum(sizeof, (
fq_mat_t,
fq_nmod_mat_t,
fq_zech_mat_t,
nmod_mat_t,
fmpz_mod_mat_t,
)), UInt8}
entries::Ptr{Nothing}
r::slong
c::slong
rows::Ptr{Nothing}
uniondata::NTuple{maximum(sizeof, (
fq_mat_t,
fq_nmod_mat_t,
fq_zech_mat_t,
nmod_mat_t,
fmpz_mod_mat_t,
) - 32), UInt8}

(where 32 of course is 2*sizeof(Ptr{Nothing}) + 2*sizeof(slong)).

So that one can then write fast accessors to these fields. Similar for fq_default_poly.

Not sure how to fit this with this PR... but one option would be to automate this as part of Union handling: if forming the C union of C struct types $T_1, \dots, T_k$ then we should in principle be able to figure out if they have common entries at the start, and automatically "extract" them (with slight cheating to "merge" Ptr types).

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.

4 participants