Skip to content

Add handling for variadic functions on arm64 #50

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

Merged
merged 1 commit into from
Oct 28, 2022
Merged

Conversation

jim22k
Copy link
Member

@jim22k jim22k commented Oct 28, 2022

osx-arm64 handles variadic functions differently than x86. cffi doesn't appear to handle this, so a bit of a hack is used to trick cffi into providing varargs in the expected manner for arm64.

https://developer.apple.com/documentation/xcode/writing-arm64-code-for-apple-platforms
https://devblogs.microsoft.com/oldnewthing/20220823-00/?p=107041

The trick is to cast varargs as char * after interpreting float/double as ints in a bitwise manner.
This was a result of trial and error after reading the above links. I don't fully understand the spec
and if this covers all use cases, but it passes the tests in this repo and allows python-graphblas
to pass all of its tests on an M1 mac.

osx-arm64 handles variadic functions differently than x86. cffi doesn't
appear to handle this, so a bit of a hack is used to trick cffi into
providing varargs in the expected manner for arm64.
@jim22k jim22k requested a review from eriknw October 28, 2022 14:12
jim22k added a commit to jim22k/python-graphblas that referenced this pull request Oct 28, 2022
Corresponding changes were first made in python-suitesparse-graphblas
GraphBLAS/python-suitesparse-graphblas#50

Variadic arguments need to be wrapped so they can be handled differently
by python-suitesparse-graphblas cffi function calls.
@DrTimothyAldenDavis
Copy link
Member

DrTimothyAldenDavis commented Oct 28, 2022

Awesome! I can also fix this in SuiteSparse:GraphBLAS. I can write a set of functions that are not variadic, but just take a single parameter of a certain type.

Consider GxB_Global_option_set. This method has a 3rd argument that can be double, double *, int, int64_t, or void *. This function always has exactly 2 parameters. I use the va_args so I can write just one function to take all 5 of these types of inputs. I don't use va_arg to have more or less parameters; it is always 2. GxB_Matrix_Option_set always has exactly 3 parameters.

So I would add 5 functions:

GxB_Global_option_set_FP64
GxB_Global_option_set_INT32
GxB_Global_option_set_INT64
GxB_Global_option_set_FP64STAR    for (double *) inputs
GxB_Global_option_set_VOIDSTAR   for (void *) inputs

and so on for all the other GxB_ set /get methods. You'll see what I would do if you grep for va_arg.

If I did this, I could actually strip the use of va_arg from SuiteSparse:GraphBLAS entirely, except in "historical" methods. I would keep GxB_Global_option_set in place, call it "historical", which means I'll keep it forever but just not document it, and recommend another function instead. It would still using va_arg, for backward compatibility. But all it would do would be to grab the 3rd parameter and call one of the 5 GxB_Global_option_set_* methods above. Then GxB_set would use only the non-variadic methods.

Had I known that va_arg would be a portability issue, I would have avoided it in the first place. This issue should be considered when defining the v2.1 C API of GraphBLAS.

This does lead to a bit of an explosion of function names. I was trying to avoid that. Witness the number of GrB_apply functions in the 2.0 C API: EIGHTY of them. That does not include any GxB_apply variants. That is the C API Spec. Far too many.

@eriknw
Copy link
Member

eriknw commented Oct 28, 2022

LGTM, thanks for working on this @jim22k. Shall we merge, release, and try to build on conda-forge?

I'm curious how well variadic argument handling works in other language bindings on different platforms/architectures.

@DrTimothyAldenDavis it would be straightforward for us to change use to type-specific get and set functions, and we probably would if available so we wouldn't need to bother and worry about differences in how variadic is handled and limitations with cffi bindings tools.

@jim22k jim22k merged commit 1da287a into GraphBLAS:main Oct 28, 2022
eriknw pushed a commit to python-graphblas/python-graphblas that referenced this pull request Oct 29, 2022
* Updates to handle variadic functions on arm64

Corresponding changes were first made in python-suitesparse-graphblas
GraphBLAS/python-suitesparse-graphblas#50

Variadic arguments need to be wrapped so they can be handled differently
by python-suitesparse-graphblas cffi function calls.

* Update minimum version of python-suitesparse-graphblas
@jim22k jim22k deleted the arm64 branch March 23, 2023 18:41
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