Skip to content

Conversation

@igarnier
Copy link

@igarnier igarnier commented Dec 29, 2024

Hi! This is a WIP PR adding float16 support to ctypes (see issue). Happy to have early feedback on the general direction. For now it uses a vendored copy of https://github.com/Leonidas-from-XIV/mucppo for preprocessing.

So far only (lightly) tested only on ocaml 5.2, on x86-64.
edit: also tested on 4.8

TODOs

  • iron out naming of files to be preprocessed (use .cppo.ml instead of .frag.ml)
  • add preprocessing directives for the >= 5.2-specific parts on the C files as well

@yallop
Copy link
Owner

yallop commented Dec 30, 2024

Thank you for picking this up, @igarnier!

Some general early feedback: it would be good to avoid as much conditional compilation as possible. We currently don't depend on the OCaml version anywhere in the source code, so I'm not keen to introduce that kind of dependency for this feature.

Is it viable to expose Ctypes.float16 for all OCaml versions, but only actually implement the full set of bigarray operations on platforms that support it, failing at run-time where necessary on the others?

@igarnier
Copy link
Author

After tinkering a bit I think it's possible to remove most of the conditional compilation directives, with the function Ctype_bigarray_stubs.kind as a notable exception: it pattern matches on Bigarray_compat.kind which does not contain the Float16 constructor for versions < 5.2. More work needed.

@igarnier igarnier force-pushed the igarnier-float16 branch 2 times, most recently from 0ccecc8 to 8c3b6d6 Compare January 1, 2025 15:45
@igarnier
Copy link
Author

igarnier commented Jan 1, 2025

The latest version eschews most of the conditional compilation. Here are the remaining bits:

  • In ctypes_float16_availability.h, there's some code detecting whether the platform supports _Float16. If _Float16 is not available, reading or writing a field of type _Float16 will raise Failure (see type_info_stubs.c). This relies on the C preprocessor but we can easily use an if statement if you prefer. Also, calling Ctypes.float16 () will assert availability via a C stub relying on the above mechanism. Checking the soundness of this detection mechanism is important!
  • There's a new binary configure/gen_bigarray_kind_conv.ml that generates the proper kind type and conversion from Bigarray_compat.kind based on the OCaml version.

I removed the tests for now.

{
switch (typeinfo)
{
#if FLOAT16_AVAILABLE
Copy link
Author

Choose a reason for hiding this comment

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

Another place where we use the preprocessor. Not sure how to avoid this one.

#define STRINGIFY1(x) #x
#define STRINGIFY(x) STRINGIFY1(x)
#ifdef FLT16_MAX
Copy link
Author

Choose a reason for hiding this comment

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

Note: we reuse the same detection method as in ctypes_float16_availability . These should be kept in sync


(* Adapted from the OCaml stdlib. This is for compatibility with OCaml 4.03 where
[String.split_on_char] is not available. *)
let split_on_char sep s =
Copy link
Author

Choose a reason for hiding this comment

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

Copied some code from the stdlib, hope that's ok license-wise. Otherwise I'll hack something else.

Copy link
Owner

Choose a reason for hiding this comment

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

I think it's better not to copy the code. Perhaps something like the following would do:

let split_on_char c s = Str.(split (regexp (String.make 1 c))) s

Copy link
Author

Choose a reason for hiding this comment

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

Works with Str.regexp_string instead of Str.regexp. Pushed a fix.

@igarnier
Copy link
Author

Just squashed history. Coming back to the code after a few weeks, I'm not super satisfied with the compile-time generation of the function kind : type a b. (a, b) Bigarray_compat.kind -> a kind. If someone has a better idea I'm all ears!

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.

2 participants