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
build(llama): manipulate cmake flags via cargo features #1716
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1716 +/- ##
==========================================
- Coverage 55.46% 53.94% -1.53%
==========================================
Files 125 115 -10
Lines 10936 9625 -1311
==========================================
- Hits 6066 5192 -874
+ Misses 4870 4433 -437 ☔ View full report in Codecov by Sentry. |
I have i7-3820 which does not support avx2. This patch works for me. I complemented default cmake parameters in llama.cpp/CMakeLists.txt |
crates/llama-cpp-bindings/Cargo.toml
Outdated
@@ -4,9 +4,18 @@ version = "0.10.0-dev.0" | |||
edition = "2021" | |||
|
|||
[features] | |||
default = ["native", "avx", "fma"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default = ["native", "avx", "fma"] | |
# non-windows | |
default = ["avx", "avx2", "fma", "f16c"] | |
# windows | |
default = ["avx", "avx2", "fma"] |
This should be the actual default tabby is using, could you update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. What should I do with avx2? I disabled it by default in order to fix #1142. Was inspired by ollama/ollama#644 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with avx2 on by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deployed on a Intel(R) Core(TM) i7-3770 CPU @ 3.40GHz with the changes I've mentioned.
Works nice. Thank you for the PR
@@ -21,8 +21,60 @@ fn main() { | |||
|
|||
fn build_llama_cpp() { | |||
let mut config = Config::new("llama.cpp"); | |||
config.define("LLAMA_NATIVE", "OFF"); | |||
config.define("INS_ENB", "ON"); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from llama.cpp/CMakeLists.txt:
# instruction set specific
if (LLAMA_NATIVE)
set(INS_ENB OFF)
else()
set(INS_ENB ON)
endif()
option(LLAMA_AVX "llama: enable AVX" ${INS_ENB})
option(LLAMA_AVX2 "llama: enable AVX2" ${INS_ENB})
option(LLAMA_AVX512 "llama: enable AVX512" OFF)
option(LLAMA_AVX512_VBMI "llama: enable AVX512-VBMI" OFF)
option(LLAMA_AVX512_VNNI "llama: enable AVX512-VNNI" OFF)
option(LLAMA_FMA "llama: enable FMA" ${INS_ENB})
# in MSVC F16C is implied with AVX2/AVX512
if (NOT MSVC)
option(LLAMA_F16C "llama: enable F16C" ${INS_ENB})
endif()
so if LLAMA_NATIVE is ON then AVX, AVX2, FMA and F16C are OFF
and if LLAMA_NATIVE is OFF then AVX, AVX2, FMA and F16C are ON
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the remark, I misunderstood the interaction between this flags
|
||
if cfg!(feature = "native") { | ||
config.define("LLAMA_NATIVE", "ON"); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definig LLAMA_NATIVE = ON turns all other options automatically OFF so the following defines have no purpose. They should be moved in the else brach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Swapped the branches
crates/llama-cpp-bindings/build.rs
Outdated
config.define("LLAMA_NATIVE", "ON"); | ||
|
||
if cfg!(not(feature = "avx")) { | ||
config.define("LLAMA_AVX2", "OFF"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: should be LLAMA_AVX instead of LLAMA_AVX2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
} | ||
else { | ||
config.define("LLAMA_NATIVE", "OFF"); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definig LLAMA_NATIVE = OFF turns all other options automatically ON so the following defines have no purpose. They should switch places with the defines above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
crates/llama-cpp-bindings/build.rs
Outdated
config.define("LLAMA_NATIVE", "OFF"); | ||
|
||
if cfg!(feature = "avx") { | ||
config.define("LLAMA_AVX2", "ON"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
LLAMA_NATIVE enables a bunch of options automatically, so if cargo features are disabled, corresponding options will be disabled too. Now the `native` feature only enables `-march=native`
656f7bf
to
abfb376
Compare
Should I also disable |
fixes #1142