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

update to KMC fork to eliminate patching and allow compilation on AMD w/SIMD #39

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ggraham
Copy link

@ggraham ggraham commented Mar 30, 2024

refresh-bio KMC (https://github.com/refresh-bio/KMC) is a dependency, but has some compilation issues that prevent available instruction set detection on AMD.

I have changed the CMake dependency to a fork of KMC replaces that with detection via GCC with __builtin function calls, as well as a few other QOL changes to

  1. replace random_shuffle w/ shuffle, as the .diff file had done previously
  2. remove requirement for SIMDe, while still compiling for SIMD where detected by GCC (ARM64 w/ NEON compat)
  3. use CXX, CXXFLAGS, and LDFLAGS where appropriate to allow implicit compilation and not require re-defining CC as g++
  4. remove requirement for external zlib (up to the user now!)
  5. add makefile target just for the library used in cuttlefish (libkmc_core.a)

I have a pull request out for these changes in refresh-bio's repo as well but until that is merged this change will allow compilation and accurate SIMD instruction set detection on AMD as well.

@rob-p
Copy link
Contributor

rob-p commented Mar 30, 2024

Hi @ggraham,

Thanks for this effort! One other thing covered in the patch that I haven't checked in the fork yet is the ability to compile under clang. This is necessary as, unlike KMC, cuttlefish doesn't require GCC for compilation on OSX, and we have an OSX userbase we need to support. Any idea if this fork will compile cleanly on Apple Clang?

Thanks!
Rob

@ggraham
Copy link
Author

ggraham commented Mar 30, 2024

Hi Rob,

This fork will likely not compile cleanly w/ Apple Clang, or at all. Is the intent to compile for both OSX with ARM and Intel SIMD? That can certainly be done and if you want to keep support for Apple arch please hold until I've made those changes and tested!

Garrett

@rob-p
Copy link
Contributor

rob-p commented Apr 3, 2024

Hi Garrett,

Yup; the intent is to compile cleanly with GCC on Linux x86 and with Clang OSX x86 and ARM. I suppose there's no reason not to work with GCC/Linux/ARM as well, but I'm not currently testing that. Given the prevalence of the M chips now (and OSX among bioinformatician laptops), that support is pretty important to us.

That said, I'd be happy to pull from a fork that incorporates these rather than have to apply the patches, especially if you're able upstream your changes to KMC.

Best,
Rob

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.

2 participants