-
Notifications
You must be signed in to change notification settings - Fork 143
Fix popcnt CPU support detection #177
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Artem Siryk <[email protected]>
Also, such compile and link check feels a bit strange to me. Should this probably be somehow implemented in the x86_64 check via running the compiled code with executing CPUID instruction and checking the 23rd bit (POPCNT bit)? Also I ignore 32 bit architectures because its native support was dropped in OCaml 5+, is this fine? |
For this to work on Windows with esy, I needed the following, diff --git a/src/discover/discover.ml b/src/discover/discover.ml
index 5ea3fb0..d3c313a 100644
--- a/src/discover/discover.ml
+++ b/src/discover/discover.ml
@@ -17,7 +17,7 @@ let () =
(fun c ->
let arch = ocaml_config_var c "architecture" in
let cc = ocaml_config_var_exn c "native_c_compiler" in
- let is_gcc =
+ let is_gcc () =
(Process.run c cc [ "--version" ] ~env:[]).stdout
|> String.lowercase_ascii
|> String.starts_with ~prefix:"gcc"
@@ -32,7 +32,7 @@ let () =
if popcntd <> ""
then popcntd
(* needed to prevent crashing, only gcc has this option *)
- else if is_gcc
+ else if is_gcc ()
then try_flag c "-mpopcntb"
else ""
| "spa" -> try_flag c "-mpopc" When
Seem like a bug somewhere in the toolchain where backslashes are used which confuses bash on Windows. |
Probably a Windows path length issue (260 char limit) after Filename.quote_command is used the name may exceed 260 chars, plus backslash escaping problems. Windows is likely struggling with long paths when creating temp files for stdout and backslashes are lost during processing of the error string. Have you tried enabling LongPaths in Windows? Might solve this. Later the stdout_fn and stderr_fn look correct, so the issue is somewhere deeper - you're right. Thanks for the suggested changes - I'll apply them. |
I have both |
Does anyone know how this surfaced just now? I just started getting this error, but it looks like this base version has been out for a while. |
Likely due to an macOS/XCode upgrade. |
I am facing issue with building the library on Mac, the recent patches have solved most of the issues. However when I try to install it within an homebrew formula it doesn't work for all the recently released versions. However, for example I have for instance created a branch on top of v0.16.4 cherry-picking your commit and I can now compile it. I can provide steps to reproduce but from what I understand the
Therefore it considers that
I haven't looked at your patch in details but I just wanted to share that it fixes my problem on Mac |
No description provided.