WIP Feature/cpp

[jmikedupont2]

This is a work in progress to compile with C++ and add in the magic header,
I just got the first version to compile by commenting out the initializers and things that did not work.
am sharing this with the community for comments, please do not merge.

[cebtenzzre]

Hm, I don't think ggerganov would approve of this. https://github.com/ggerganov/ggml is written in C for simplicity.

[jmikedupont2]

Yes, I understand that, but lets have a conversation about it?

…

On Tue, Nov 21, 2023, 12:10 Jared Van Bortel ***@***.***> wrote: Hm, I don't think ggerganov would approve of this. https://github.com/ggerganov/ggml is written in C for simplicity. — Reply to this email directly, view it on GitHub <#4155 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD5KQ2N4NGKHQ6PZZY2HDXTYFTOABAVCNFSM6AAAAAA7U2PIF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRRGMZDIOBVG4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[jmikedupont2]

I think this could uncover some bugs and I might have found some invalid type conversions already. We should plan to maintain a C++ branch of the code in parallel for a few months.

…

On Tue, Nov 21, 2023, 12:11 Jim Dupont ***@***.***> wrote: Yes, I understand that, but lets have a conversation about it? On Tue, Nov 21, 2023, 12:10 Jared Van Bortel ***@***.***> wrote: > Hm, I don't think ggerganov would approve of this. > https://github.com/ggerganov/ggml is written in C for simplicity. > > — > Reply to this email directly, view it on GitHub > <#4155 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AD5KQ2N4NGKHQ6PZZY2HDXTYFTOABAVCNFSM6AAAAAA7U2PIF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRRGMZDIOBVG4> > . > You are receiving this because you authored the thread.Message ID: > ***@***.***> >

[jmikedupont2]

for example converting a float to an int might not be what you want ```ggml.cpp:5494:46: warning: narrowing conversion of ‘p0’ from ‘float’ to ‘int32_t’ {aka ‘i\nt’} [-Wnarrowing] 5494 | int32_t params[] = { op, k0, k1, s0, s1, p0, p1 }; | ^~ ```

…

On Tue, Nov 21, 2023, 12:23 Jim Dupont ***@***.***> wrote: I think this could uncover some bugs and I might have found some invalid type conversions already. We should plan to maintain a C++ branch of the code in parallel for a few months. On Tue, Nov 21, 2023, 12:11 Jim Dupont ***@***.***> wrote: > Yes, I understand that, but lets have a conversation about it? > > On Tue, Nov 21, 2023, 12:10 Jared Van Bortel ***@***.***> > wrote: > >> Hm, I don't think ggerganov would approve of this. >> https://github.com/ggerganov/ggml is written in C for simplicity. >> >> — >> Reply to this email directly, view it on GitHub >> <#4155 (comment)>, >> or unsubscribe >> <https://github.com/notifications/unsubscribe-auth/AD5KQ2N4NGKHQ6PZZY2HDXTYFTOABAVCNFSM6AAAAAA7U2PIF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRRGMZDIOBVG4> >> . >> You are receiving this because you authored the thread.Message ID: >> ***@***.***> >> >

[cebtenzzre]

for example converting a float to an int might not be what you want

You can also build master with cmake -B build -DCMAKE_C_FLAGS='-Wfloat-conversion' && make -C build main to see this warning. That probably is a bug, so perhaps we should enable -Wfloat-conversion by default.

[jmikedupont2]

yes but that is not the point here, the point is that getting it to compile on c++ can uncover things and highlight things and is a worthwhile exercize. Also the usage of templates could clean up the code a lot.

…

On Tue, Nov 21, 2023, 12:55 Jared Van Bortel ***@***.***> wrote: for example converting a float to an int might not be what you want You can also build master with cmake -B build -DCMAKE_C_FLAGS='-Wfloat-conversion' && make -C build main to see this warning. That probably is a bug, so perhaps we should enable -Wfloat-conversion by default. — Reply to this email directly, view it on GitHub <#4155 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD5KQ2JIQG3ZGTJ3XKXN3WLYFTTIDAVCNFSM6AAAAAA7U2PIF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRRGM4TIOJSGQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[jmikedupont2]

This is another thing that popped up, bunch of the types dont have the full complement of functions

ggml.cpp:2034:3: warning: missing initializer for member ‘ggml_type_traits_t::from_float’\
 [-Wmissing-field-initializers]

[jmikedupont2]

Its running and producing text!

[cebtenzzre]

Yeah, that's probably not gonna happen either... for the most part this project uses a subset of C++11 that barely even includes templates. Kerfuffle and I are proposing to add some in #4092, we'll see how it goes - it could be that it ends up getting rewritten to do things the C way. C++20, a language which has modules, coroutines, and concepts, is almost certainly out of the question.

[jmikedupont2]

The future is going to happen in the the future for sure, I am super excited I got this done in one day. It does not matter if you merge it, it can live as a compatible branch for now.

…

On Tue, Nov 21, 2023, 21:23 Jared Van Bortel ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In CMakeLists.txt <#4155 (comment)>: > @@ -718,7 +718,7 @@ add_library(llama ) target_include_directories(llama PUBLIC .) -target_compile_features(llama PUBLIC cxx_std_11) # don't bump +target_compile_features(llama PUBLIC cxx_std_20) # don't bump Yeah, that's probably not gonna happen either... for the most part this project uses a subset of C++11 that barely even includes templates. Kerfuffle and I are proposing to add some in #4092 <#4092>, we'll see how it goes - it could be that it ends up getting rewritten to do things the C way. C++20, a language which has modules, coroutines, and concepts, is almost certainly out of the question. — Reply to this email directly, view it on GitHub <#4155 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD5KQ2IKZ73NPA5GU73FY3TYFVOZBAVCNFSM6AAAAAA7U2PIF6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONBTGQ2DKNJSGM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[ggerganov]

for example converting a float to an int might not be what you want

You can also build master with cmake -B build -DCMAKE_C_FLAGS='-Wfloat-conversion' && make -C build main to see this warning. That probably is a bug, so perhaps we should enable -Wfloat-conversion by default.

It's not a bug - it was added with the YOLO example to support padding with odd number:

ggerganov/ggml#576

It needs better comment for ggml_pool_2d and explicit cast to int

Regarding turning ggml to C++ - definitely not an option.
But appreciate the effort in this PR and don't mind having it live as a separate branch.

We can think about adding C++ bindings as proposed in ggerganov/ggml#581

[jmikedupont2]

Well one of the things that I'm looking into is that we can generate better bindings using the template metaprogramming and then we can use that inside of the C code so I'm thinking we can generate code to be used in the Legacy version

…

On Wed, Nov 22, 2023, 08:09 Georgi Gerganov ***@***.***> wrote: for example converting a float to an int might not be what you want You can also build master with cmake -B build -DCMAKE_C_FLAGS='-Wfloat-conversion' && make -C build main to see this warning. That probably is a bug, so perhaps we should enable -Wfloat-conversion by default. It's not a bug - it was added with the YOLO example to support padding with odd number: ggerganov/ggml#576 <ggerganov/ggml#576> It needs better comment for ggml_pool_2d and explicit cast to int Regarding turning ggml to C++ - definitely not an option. But appreciate the effort in this PR and don't mind having it live as a separate branch. We can think about adding C++ bindings as proposed in ggerganov/ggml#581 <ggerganov/ggml#581> — Reply to this email directly, view it on GitHub <#4155 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD5KQ2JJE6DPFZMLWZLFEQDYFX2PBAVCNFSM6AAAAAA7U2PIF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRSG42DCOBWGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[jmikedupont2]

Also If we can just refactor those bits like the initializers so the live in a different module or use conditional compliation then we can have the code compile without change in c++and c as well. I am not trying to refactor everything or remove malloc etc. So this could actually be cleaned up so it would be an optional flag.

[jmikedupont2]

#4209 see this better pr