Fixed build on Linux with GCC #44

stemann · 2024-11-19T12:13:44Z

Also, Fixed CMakeLists wrt. setting MLX_BUILD_BENCHMARKS=OFF.

Follow-up after #38

Needed for JuliaPackaging/Yggdrasil#9809

stemann · 2024-11-19T12:23:20Z

~~Still having some trouble with:~~

The type provided to mlx_closure_value_and_grad_new_ in https://github.com/ml-explore/mlx-c/blob/main/mlx/c/closure.cpp#L192 and https://github.com/ml-explore/mlx-c/blob/main/mlx/c/closure.cpp#L238

The type provided to mlx_closure_custom_vmap_new_ in https://github.com/ml-explore/mlx-c/blob/main/mlx/c/closure.cpp#L609, and https://github.com/ml-explore/mlx-c/blob/main/mlx/c/closure.cpp#L663

~~@andresy Could you take a quick peek at the error(s) in the log at https://github.com/stemann/mlx-c/actions/runs/11912756270/job/33197036071#step:8:175 ?~~

stemann · 2024-11-19T15:32:29Z

Never mind. Got it fixed: https://github.com/stemann/mlx-c/actions/runs/11916338116

stemann · 2024-11-19T15:40:54Z

I have removed the GitHub Actions build workflow from this PR, but it is available for a separate PR if you would like to get build CI via GitHub Actions: https://github.com/stemann/mlx-c/tree/feature/github_actions_build : https://github.com/stemann/mlx-c/actions/runs/11916455284

andresy · 2024-11-19T22:04:36Z

mlx/c/array.h

@@ -5,6 +5,7 @@

 #include "mlx/c/string.h"

+#include <cstdint>


I did not need this one on my side, curious which GCC compiler/system are you using, and where the error is located?

this is a C header, so includes should be C, not C++; is the error popping up in array.cpp or in array.h?

GCC 11.4: https://github.com/stemann/mlx-c/actions/runs/11910841319/job/33191101517#step:4:293

In file included from /home/runner/work/mlx-c/mlx-c/mlx/c/array.cpp:5: /home/runner/work/mlx-c/mlx-c/mlx/c/array.h:176:26: error: ‘uint8_t’ was not declared in this scope 176 | int mlx_array_item_uint8(uint8_t* res, const mlx_array arr); | ^~~~~~~

Though, I'm using GCC 10 in JuliaPackaging/Yggdrasil#9809

I might have messed up wrt. C vs C++ headers.

Uh ok. Definitively need a stdint.h there, thanks

To be more specific, I was using:

Locally, in a container mcr.microsoft.com/devcontainers/cpp:1-debian-11: GCC 10.2 (on Debian 11).

GitHub Actions, on "ubuntu-latest" runner: GCC 11.4

BinaryBuilder env. (Julia): GCC 10.2 (cross-compile env. running on an Alpine Linux (musl) - cross-compiling to various targets)

andresy · 2024-11-19T22:07:47Z

mlx/c/closure.cpp

@@ -183,7 +183,7 @@ extern "C" mlx_closure_value_and_grad mlx_closure_value_and_grad_new_func(
            "mlx_closure_value_and_grad returned a non-zero value");
      }
      auto cpp_res =
-          std::tie(mlx_vector_array_get_(res_0), mlx_vector_array_get_(res_1));
+          std::make_pair(mlx_vector_array_get_(res_0), mlx_vector_array_get_(res_1));


Sorry this is not really documented, but this file is auto-generated, so one should not modify it manually. Instead, modify mlxtypes.py

cpp_make_tuple = "std::make_pair" if n == 2 else "std::make_tuple" types.append( { "cpp": cpp_tuple + "<" + ", ".join(cpp_types) + ">", "alt": [cpp_tuple + "<" + ", ".join(alt_types) + ">"] + alts, "c_to_cpp": lambda s: cpp_make_tuple + "("

The file can then be generated via

python ../../python/closure_generator.py --implementation > closure.cpp

(a clang-format -i closure.cpp would need to follow)

I added the change to the mlxtypes.py code generator, but if I run it there are a lot more changes than I would expect (I would have expected zero - keeping my manual change).

Ah - got you message on clang-format now. Now executed.

andresy · 2024-11-19T22:08:39Z

mlx/c/compile.h

@@ -8,6 +8,8 @@

 #include <stdio.h>

+#include <cstdint>


same comments as above

this is a C header

where is the error coming up? (I do not have it on my end)

It was for uintptr_t, it seems: https://github.com/stemann/mlx-c/actions/runs/11911185130/job/33192141400#step:5:334

andresy · 2024-11-19T22:10:03Z

mlx/c/fast.cpp

@@ -3,6 +3,8 @@
 /* This file is auto-generated. Do not edit manually. */
 /*                                                    */

+#include <cstdarg>


This is an auto-generated file. I do not get the error there as well, so curious to know where it occurs. For this one, and the one in metal.cpp, I would suggest to put additional includes in mlx/c/private/mlx.h

cstdarg was for the va_x functions, like va_list, va_arg etc.

Noted, i.e. moving it to mlx/c/private/mlx.h.

andresy · 2024-11-19T22:10:16Z

mlx/c/metal.cpp

@@ -3,6 +3,8 @@
 /* This file is auto-generated. Do not edit manually. */
 /*                                                    */

+#include <cstring>


see comment above

Noted, i.e. moving it to mlx/c/private/mlx.h.

I tried replacing it with <string.h>, but it failed - again complaining that there is no strncpy: https://github.com/stemann/mlx-c/actions/runs/11923235104

Using (now in mlx/c/private/mlx.h) fixes it: https://github.com/stemann/mlx-c/actions/runs/11923278948

andresy

Thanks! Asking for few changes, let me know if that works for you, as I would prefer to have these added before I tag the next MLX C version

stemann · 2024-11-19T23:03:51Z

It would be great to get these changes merged in, and a new version tagged.

Your requested changes should now be implemented - except I have not run the mlxtypes.py-script (too many unrelated changes would occur).

* Added missing includes of cstdint, cstring etc. * Replaced pair construction with std::tie with std::make_pair.

-- as it is needed for the (generated) compile.h file

stemann force-pushed the feature/build_linux branch from b646958 to 626a6e5 Compare November 19, 2024 15:30

stemann marked this pull request as ready for review November 19, 2024 15:35

stemann force-pushed the feature/build_linux branch from 626a6e5 to 846407a Compare November 19, 2024 15:36

stemann changed the title ~~WIP Fixed build on Linux with GCC~~ Fixed build on Linux with GCC Nov 19, 2024

andresy reviewed Nov 19, 2024

View reviewed changes

andresy requested changes Nov 19, 2024

View reviewed changes

stemann requested a review from andresy November 19, 2024 23:31

stemann and others added 8 commits November 19, 2024 16:26

Fixed CMakeLists wrt. setting MLX_BUILD_BENCHMARKS=OFF

162a862

Fixed build on Linux with GCC

cee1d9a

* Added missing includes of cstdint, cstring etc. * Replaced pair construction with std::tie with std::make_pair.

Moved includes from fast.cpp and metal.cpp to private/mlx.h

782b4eb

Added change to mlxtypes.py code generator

669f1a3

Replaced C++ headers with C headers

061cbe8

Added cstring for strncpy

001e922

Ran mlxtypes.py and clang-format

18f4802

add stdint.h to code generator

9fe2944

-- as it is needed for the (generated) compile.h file

andresy force-pushed the feature/build_linux branch from 6455e76 to 9fe2944 Compare November 20, 2024 00:34

cstdarg instead of stdarg.h for C++ header

74c3ea4

andresy approved these changes Nov 20, 2024

View reviewed changes

andresy merged commit 1262536 into ml-explore:main Nov 20, 2024

stemann deleted the feature/build_linux branch November 20, 2024 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed build on Linux with GCC #44

Fixed build on Linux with GCC #44

stemann commented Nov 19, 2024 •

edited

Loading

stemann commented Nov 19, 2024 •

edited

Loading

stemann commented Nov 19, 2024

stemann commented Nov 19, 2024 •

edited

Loading

andresy Nov 19, 2024

stemann Nov 19, 2024 •

edited

Loading

stemann Nov 19, 2024

andresy Nov 19, 2024

stemann Nov 19, 2024

andresy Nov 19, 2024

andresy Nov 19, 2024

stemann Nov 19, 2024

stemann Nov 19, 2024 •

edited

Loading

stemann Nov 19, 2024

andresy Nov 19, 2024

stemann Nov 19, 2024

andresy Nov 19, 2024

stemann Nov 19, 2024

stemann Nov 19, 2024

andresy Nov 19, 2024

stemann Nov 19, 2024

stemann Nov 19, 2024

andresy left a comment •

edited

Loading

stemann commented Nov 19, 2024

Fixed build on Linux with GCC #44

Fixed build on Linux with GCC #44

Conversation

stemann commented Nov 19, 2024 • edited Loading

stemann commented Nov 19, 2024 • edited Loading

stemann commented Nov 19, 2024

stemann commented Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

stemann Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stemann Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresy left a comment • edited Loading

Choose a reason for hiding this comment

stemann commented Nov 19, 2024

stemann commented Nov 19, 2024 •

edited

Loading

stemann commented Nov 19, 2024 •

edited

Loading

stemann commented Nov 19, 2024 •

edited

Loading

stemann Nov 19, 2024 •

edited

Loading

stemann Nov 19, 2024 •

edited

Loading

andresy left a comment •

edited

Loading