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

Define magic numbers as integer constants (#1518) #1520

Merged
merged 1 commit into from
May 20, 2023

Conversation

imaami
Copy link
Contributor

@imaami imaami commented May 18, 2023

The underlying representation of multibyte character literals is
implementation-defined. This could, at least in principle, cause
cross-build data export/import issues independent of endianness.

Define magic numbers as integer literals to be on the safe side.

@ice0
Copy link

ice0 commented May 19, 2023

@imaami
Can you make a short example showing the problem?
I've read the whole discussion but still haven't got proof that this could be the problem.

@imaami
Copy link
Contributor Author

imaami commented May 19, 2023

@imaami Can you make a short example showing the problem? I've read the whole discussion but still haven't got proof that this could be the problem.

It isn't currently a practical problem as far as compiler behavior is concerned. My first conclusion was incorrect, all three major compilers (GCC, Clang, MSVC) generate the same 4-byte sequence from the relevant multibyte character literals ('ggjt' etc.). But they don't agree with one another because of any standardized language feature, they just happen to agree for whatever reason. They could all behave in different ways and would nevertheless conform to the C and C++ standards.

The only "problem" left on the table is the implementation-defined nature of multibyte character literals and how a compiler is allowed to represent one in memory. Nothing in the C or C++ standards mandates a specific order for the constituent bytes. So although in practice this issue is only a theoretical one as of now, defining the magic numbers as integer literals would still be a more robust alternative.

@imaami
Copy link
Contributor Author

imaami commented May 19, 2023

Oh, and there's also another thing: the language standards define the type of a multibyte character literal as int. If llama.cpp assumes that multibyte character literals are guaranteed to have an underlying representation of at least 4 bytes, then let me tell you, exporting a weights file on an 8-bit MCU is going to fail miserably due to int being only 16 bits wide. ;) (Although I suspect trying to do that would be unpleasant for other reasons too.)

Copy link
Member

@ggerganov ggerganov left a comment

Choose a reason for hiding this comment

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

@imaami
Copy link
Contributor Author

imaami commented May 19, 2023

I guess these also have to be updated:

https://github.com/ggerganov/llama.cpp/blob/2d5db48371052087a83974abda3767d1aedec598/llama.cpp#L430-L447

How did I miss these? :D I'll get on it and rebase to the current master too.

@slaren
Copy link
Member

slaren commented May 19, 2023

@imaami imaami force-pushed the magic-multibyte-char-literals branch from 8cd145e to 2be4e54 Compare May 20, 2023 12:53
The underlying representation of multibyte character literals is
implementation-defined. This could, at least in principle, cause
cross-build data export/import issues independent of endianness.

Define magic numbers as integer literals to be on the safe side.

Signed-off-by: Juuso Alasuutari <[email protected]>
@imaami imaami force-pushed the magic-multibyte-char-literals branch from 2be4e54 to c69f0cd Compare May 20, 2023 12:54
@imaami imaami requested a review from ggerganov May 20, 2023 12:56
@ggerganov ggerganov merged commit 29cf559 into ggml-org:master May 20, 2023
@imaami
Copy link
Contributor Author

imaami commented May 20, 2023

Wow, thanks for the quick approval. :)

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