Skip to content

Conversation

@rgryta
Copy link
Contributor

@rgryta rgryta commented Feb 24, 2024

No description provided.

@rgryta
Copy link
Contributor Author

rgryta commented Feb 24, 2024

Fixes issue as described in:
#4421

Comment on lines +939 to +945
if ("${CMAKE_SYSTEM_NAME}" STREQUAL "Android")
# Android armeabi-v7a
list(APPEND ARCH_FLAGS -mfpu=neon-vfpv4 -mno-unaligned-access -funsafe-math-optimizations)
else()
# Raspberry Pi 2
list(APPEND ARCH_FLAGS -mfpu=neon-fp-armv8 -mno-unaligned-access -funsafe-math-optimizations)
endif()
Copy link
Collaborator

@cebtenzzre cebtenzzre Feb 24, 2024

Choose a reason for hiding this comment

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

Is there a reason we are explicitly specifying -mfpu anyway? Shouldn't the default of -march=native cause the compiler to use the appropriate -mfpu by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should, I have no idea why the auto setting is overriden here as well.
I've crosscompiled for the Android armeabi-v7a with removed -mfpu flag and it compiled successfully as well. I've only set the flag to neon-vfpv4 as suggested in the attached Issue (#4421). Maybe @prusnak could shed some light onto the meaning of those flags for arm v6 and v7?

Copy link
Contributor

Choose a reason for hiding this comment

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

auto only works if you are compiling on the target system

specifying flags explicitly allows you to crosscompile for different system

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on this we should be able to replace -mfpu with -march and the compiler would do the rest for us. I do not own any of these Raspberries so I cannot even test this myself unfortunately.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it should be the build script's job to guess what kind of FPU you have based only on CMAKE_SYSTEM_PROCESSOR and CMAKE_SYSTEM_NAME anyway - best to explicitly specify the necessary -march and -mcpu (and -mfpu if desired, but shouldn't be necessary) in your toolchain file's CMAKE_C_FLAGS_INIT/CMAKE_CXX_FLAGS_INIT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question is then if we want to completely get rid of setting those flags?
I'm not that familiar with llama.cpp contribution guidelines so I do not know if there's any plan for such thing.
At the moment, I'd say it'd make sense given current build script structure.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose the problem is that Android builds are often cross-compiled (i.e. you almost never build on the actual device).

Agree it's a bit of a mess with these flags as it is (same in whisper.cpp). But since we lack all the various hardware to test so many cases, we can patch stuff one-by-one

Comment on lines +939 to +945
if ("${CMAKE_SYSTEM_NAME}" STREQUAL "Android")
# Android armeabi-v7a
list(APPEND ARCH_FLAGS -mfpu=neon-vfpv4 -mno-unaligned-access -funsafe-math-optimizations)
else()
# Raspberry Pi 2
list(APPEND ARCH_FLAGS -mfpu=neon-fp-armv8 -mno-unaligned-access -funsafe-math-optimizations)
endif()
Copy link
Member

Choose a reason for hiding this comment

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

I suppose the problem is that Android builds are often cross-compiled (i.e. you almost never build on the actual device).

Agree it's a bit of a mess with these flags as it is (same in whisper.cpp). But since we lack all the various hardware to test so many cases, we can patch stuff one-by-one

@ggerganov ggerganov merged commit 1289408 into ggml-org:master Feb 25, 2024
jordankanter pushed a commit to jordankanter/llama.cpp that referenced this pull request Mar 13, 2024
hodlen pushed a commit to hodlen/llama.cpp that referenced this pull request Apr 1, 2024
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