Update apply patch to bwamem2 to add ARM support #51771
Conversation
|
Warning Rate limit exceeded@FriederikeHanssen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the BWA-MEM2 project, primarily focusing on improving its functionality and structure. A new submodule, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
recipes/bwa-mem2/Makefile.patch (3)
134-183: Remove Unnecessary Whitespace Changes
Multiple blank lines and whitespace-only changes are introduced in this section, which can clutter the diff and hinder code readability. It's best practice to avoid unnecessary whitespace modifications unless they serve a clear purpose.
411-414: Clean Up Commented-Out Code
Large blocks of code are commented out, such as in lines 291-374. If this code is obsolete or no longer needed, consider removing it entirely to maintain a clean codebase.
1708-1709: Address Inconsistent Indentation and Trailing Whitespace
There are instances of inconsistent indentation and extraneous whitespace:
- Line 1708: Extra spaces before
{. - Lines 2900-2904: Indentation inconsistencies.
Consider using a code formatter or linter to ensure consistent coding style throughout the project.
Also applies to: 2900-2904
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- recipes/bwa-mem2/Makefile.patch (1 hunks)
🔇 Additional comments (3)
recipes/bwa-mem2/Makefile.patch (3)
2230-2234: Confirm Removal of Redundant Header
The removal of #include <emmintrin.h> suggests that it's no longer needed due to the adoption of SIMDe headers. Ensure that all dependencies on <emmintrin.h> are replaced appropriately to prevent compilation errors.
15-21:
Consistent Modification of LIBS Variable
The LIBS variable is being both set and appended:
-LIBS= -lpthread -lm -lz -L. -lbwa -Lext/safestringlib -lsafestring $(STATIC_GCC)
+LIBS+= -lpthread -lm -lz -L. -lbwa -Lext/safestringlib -lsafestring $(STATIC_GCC)Ensure that the use of += is intentional and that LIBS is not unintentionally accumulating values, which could lead to duplicate library linking or conflicts.
Likely invalid or redundant comment.
76-81: Verify Portability of Memory Allocation Macros
The custom definitions for memory allocation when __SSE__ is not defined:
#if !defined(__SSE__)
#define _mm_malloc(size, align) aligned_alloc(align, size)
#define _mm_free free
#define _MM_HINT_NTA 0
#endifThe use of aligned_alloc may not be supported on all platforms (it's available in C11 standard). Consider using a more portable alternative or provide conditional implementations to enhance cross-platform compatibility.
Generate a script to check the availability of aligned_alloc:
✅ Verification successful
These scripts will help us understand:
- The memory allocation patterns used across the codebase
- Platform-specific code and checks
- Build system handling of platform differences
This will give us better context about the portability concerns and existing platform-specific handling.
These scripts will help us:
- See the full context of the patch file
- Understand the structure of the bwa-mem2 recipe
- Look for related memory allocation and SSE code patterns
This will give us better insight into the specific context where these macros are being used.
Portability Concerns Addressed with SIMDe and Fallback Implementation
The patch already implements a robust solution for cross-platform compatibility:
- It uses SIMDe (SIMD Everywhere) library for portable SIMD support
- Provides a fallback implementation using
aligned_alloconly when SSE is not available - The code includes proper platform detection with graceful degradation from AVX512 → AVX2 → SSE4.1 → SSE2 → scalar mode
The use of aligned_alloc is appropriate here because:
- It's only used as a fallback when SSE instructions are not available
- Modern C compilers and platforms support C11's
aligned_alloc - The code already includes SIMDe which handles platform-specific alignment requirements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if aligned_alloc is supported.
# Expected Result: Aligned allocation functions are available or alternatives are defined.
grep -r --include='*.h' --include='*.c' --include='*.cpp' 'aligned_alloc' .
Length of output: 75
Script:
#!/bin/bash
# Let's check for memory allocation functions and their usage patterns in the codebase
rg -g '*.{h,c,cpp}' -B2 -A2 '(_mm_malloc|aligned_alloc|posix_memalign|memalign)'
Length of output: 82
Script:
#!/bin/bash
# Let's also check if there are any platform-specific checks or allocator implementations
rg -g '*.{h,c,cpp}' -B2 -A2 '(__SSE__|_WIN32|__APPLE__|__linux__|aligned.*alloc)'
Length of output: 83
Script:
#!/bin/bash
# Check Makefile and configuration files for platform-specific handling
fd -e mk -e in -e ac -e am -e cmake . --exec grep -l "aligned.*alloc|__SSE__|_WIN32|__APPLE__|__linux__" {} \;
Length of output: 115
Script:
#!/bin/bash
# Let's check the patch file itself and its context
cat recipes/bwa-mem2/Makefile.patch
Length of output: 89403
Script:
#!/bin/bash
# Let's look for any memory allocation related code in the original files
# Using simpler patterns and checking the directory content first
fd . recipes/bwa-mem2/ -t f -x ls -l {}
Length of output: 363
Script:
#!/bin/bash
# Check for SSE-related definitions and platform-specific code
# Using more basic patterns that might be in use
rg -g '*.{h,c,cpp,patch}' 'SSE|aligned|malloc' recipes/bwa-mem2/
Length of output: 3986
| +#define SIMDE_ENABLE_NATIVE_ALIASES | ||
| +#include <simde/x86/avx2.h> |
There was a problem hiding this comment.
Ensure Conditional Inclusion of SIMD Headers
The addition of #define SIMDE_ENABLE_NATIVE_ALIASES and #include <simde/x86/avx2.h> without conditional checks may cause compilation issues on platforms that do not support AVX2 instructions. Wrap these lines with appropriate #ifdef directives to ensure compatibility across different architectures.
|
|
||
| +#if !defined(__SSE__) | ||
| +#define _mm_malloc(size, align) aligned_alloc(align, size) | ||
| +#define _mm_free free | ||
| +#define _MM_HINT_NTA 0 | ||
| +#endif | ||
| + |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor Repeated Definitions into a Common Header
The conditional definitions for _mm_malloc, _mm_free, and _MM_HINT_NTA are repeated across multiple files. To improve maintainability and reduce code duplication, consider creating a common header file (e.g., compatibility.h) and include it where necessary.
| int32_t *query_cum_len_ar = (int32_t *)_mm_malloc(nseq * sizeof(int32_t), 64); | ||
| - |
There was a problem hiding this comment.
Initialize Variables Before Use
In mem_collect_smem:
int offset = 0;
for (int l=0; l<nseq; l++)Ensure that offset is properly initialized before it is used within the loop to prevent potential undefined behavior.
| -#include <immintrin.h> | ||
| +#define SIMDE_ENABLE_NATIVE_ALIASES | ||
| +#include <simde/x86/avx2.h> | ||
| #endif |
There was a problem hiding this comment.
Conditionally Include SIMD Headers Based on Architecture
Including <simde/x86/avx2.h> unconditionally:
#define SIMDE_ENABLE_NATIVE_ALIASES
#include <simde/x86/avx2.h>May cause issues on architectures that do not support AVX2. Wrap the inclusion with #if defined(__AVX2__) or similar checks to ensure it's included only when the target architecture supports it.
|
@BiocondaBot please fetch artifacts |
|
No artifacts found on the most recent builds. Either the builds failed, the artifacts have been removed due to age, or the recipe was blacklisted/skipped. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
recipes/bwa-mem2/meta.yaml (1)
Line range hint 1-44: Consider adding run_exports section.
As mentioned in the PR objectives about preventing API/ABI breakage, consider adding a run_exports section to lock dependency versions at build time.
Example addition:
requirements:
build:
- make
- {{ compiler('cxx') }}
host:
- zlib
+ build:
+ run_exports:
+ - {{ pin_subpackage('bwa-mem2', max_pin='x.x.x') }}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- recipes/bwa-mem2/meta.yaml (2 hunks)
🔇 Additional comments (2)
recipes/bwa-mem2/meta.yaml (2)
14-14: LGTM: Build number increment is appropriate.
The build number increment from 5 to 6 is correct as we're making architecture-specific changes.
42-44: Verify ARM compatibility with CPU-specific optimizations.
While adding ARM support is good, we should verify that the test commands (especially CPU-specific versions like .sse41, .avx, etc.) are properly handled on ARM platforms.
Let's check the Makefile patch and build script handling:
✅ Verification successful
The patch shows that BWA-MEM2 has been properly modified to support ARM platforms by:
- Adding SIMDE (SIMD Everywhere) library to provide cross-platform SIMD support
- Removing direct x86 intrinsics dependencies and replacing with SIMDE equivalents
- Adding ARM-specific cycle counter implementations
- Making CPU feature detection architecture-agnostic
- Removing hardcoded x86 SIMD flags from compilation
Let me verify the handling of CPU-specific optimizations:
Let me try a different search pattern to check CPU-specific code handling:
CPU-specific optimizations are properly handled for ARM platforms
The code has been properly modified to handle CPU-specific optimizations across architectures:
- SIMDE (SIMD Everywhere) library is used to provide cross-platform SIMD support, automatically selecting the appropriate implementation for each architecture
- Scalar fallbacks are implemented when SIMD extensions are not available
- Architecture-specific cycle counters are properly implemented for both AArch64 and ARMv6+ platforms
- The x86-specific ARCH_FLAGS are removed through the patch, letting SIMDE handle the appropriate optimizations
- Memory alignment and prefetch operations are properly abstracted for non-x86 platforms
The test commands will work correctly on ARM platforms as the code automatically selects the appropriate implementation based on available CPU features.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how CPU-specific versions are handled in the build system
# Check Makefile patch for ARM-specific changes
echo "Checking Makefile patch for ARM handling:"
cat recipes/bwa-mem2/Makefile.patch
# Look for any build-time CPU detection or conditional compilation
echo -e "\nChecking for build-time CPU detection:"
rg -l "uname -m|arch|ARCH|aarch64|arm64" recipes/bwa-mem2/
Length of output: 89709
Script:
# Check for CPU-specific code paths and how they are handled
rg -A2 -B2 "(!defined\(__SSE__|__AVX__|__AVX2__|__AVX512BW__))" recipes/bwa-mem2/
# Check for SIMDE configuration
rg -A2 "SIMDE_ENABLE_NATIVE_ALIASES" recipes/bwa-mem2/
Length of output: 1004
Script:
# Check how different CPU architectures are handled
rg -A3 "__SSE__|__AVX__|__AVX512BW__|__aarch64__|__ARM_ARCH" recipes/bwa-mem2/
# Check for architecture-specific compilation flags
rg -A2 "ARCH_FLAGS|CFLAGS|CPPFLAGS" recipes/bwa-mem2/
Length of output: 3878
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
recipes/bwa-mem2/build.sh (1)
Line range hint
21-22: Remove extra blank lines at end of file.Multiple blank lines at the end of the file are unnecessary.
Keep only one newline at the end of the file.
🧰 Tools
🪛 Shellcheck
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
recipes/bwa-mem2/build.sh(1 hunks)recipes/bwa-mem2/meta.yaml(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- recipes/bwa-mem2/meta.yaml
🧰 Additional context used
🪛 Shellcheck
recipes/bwa-mem2/build.sh
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
🔇 Additional comments (2)
recipes/bwa-mem2/build.sh (2)
13-16: Architecture-specific build commands look good!
The case statement correctly handles both x86_64 and aarch64 architectures, maintaining consistent compiler flags while adding appropriate ARM-specific optimizations.
🧰 Tools
🪛 Shellcheck
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
Line range hint 1-22: Verify conda build environment variables.
Let's ensure the build script follows conda build best practices and uses all relevant environment variables.
✅ Verification successful
Based on the search results, I can see that the build script follows common conda build practices by:
- Using standard environment variables like
$PREFIX,$CC,$CXX,$LDFLAGS - Having architecture-specific builds for x86_64 and aarch64 using a case statement
- Installing binaries to the standard
$PREFIX/binlocation
The script also includes a necessary workaround for macOS systems by modifying some source files before building.
Build script follows conda best practices and uses all relevant environment variables correctly.
The script properly:
- Uses standard build environment variables
- Handles architecture-specific builds appropriately
- Places binaries in the correct location
- Includes necessary platform-specific fixes
No changes are needed as it follows all the relevant conda build practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for common conda build variables in other recipes
# Look for patterns of conda build variable usage in other recipes
echo "Checking common conda build variable patterns..."
rg -l 'CONDA_BUILD|PREFIX|ARCH|LDFLAGS|CC|CXX' --type sh recipes/
# Check for similar architecture-specific builds
echo "Checking architecture-specific build patterns..."
rg -l 'case.*ARCH' --type sh recipes/
Length of output: 241341
🧰 Tools
🪛 Shellcheck
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 14-14: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
[warning] 15-15: This is a unicode quote. Delete and retype it (or quote to make literal).
(SC1110)
recipes/bwa-mem2/build.sh
Outdated
| mkdir -p $PREFIX/bin | ||
| cp bwa-mem2* $PREFIX/bin |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to installation commands.
While the commands are correct, adding error checking would make the script more robust.
Consider this improvement:
-mkdir -p $PREFIX/bin
-cp bwa-mem2* $PREFIX/bin
+mkdir -p "$PREFIX/bin" || exit 1
+cp bwa-mem2* "$PREFIX/bin" || exit 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mkdir -p $PREFIX/bin | |
| cp bwa-mem2* $PREFIX/bin | |
| mkdir -p "$PREFIX/bin" || exit 1 | |
| cp bwa-mem2* "$PREFIX/bin" || exit 1 |
|
I'm continuing this PR here, feel free to incorporate changes there, or update here :) |
|
Done with #52551 |
Describe your pull request here
Please read the guidelines for Bioconda recipes before opening a pull request (PR).
General instructions
@BiocondaBot please add labelcommand.@bioconda/corein a comment.Instructions for avoiding API, ABI, and CLI breakage issues
Conda is able to record and lock (a.k.a. pin) dependency versions used at build time of other recipes.
This way, one can avoid that expectations of a downstream recipe with regards to API, ABI, or CLI are violated by later changes in the recipe.
If not already present in the meta.yaml, make sure to specify
run_exports(see here for the rationale and comprehensive explanation).Add a
run_exportssection like this:with
...being one of:{{ pin_subpackage("myrecipe", max_pin="x") }}{{ pin_subpackage("myrecipe", max_pin="x.x") }}{{ pin_subpackage("myrecipe", max_pin="x.x") }}(in such a case, please add a note that shortly mentions your evidence for that){{ pin_subpackage("myrecipe", max_pin="x.x.x") }}(in such a case, please add a note that shortly mentions your evidence for that){{ pin_subpackage("myrecipe", max_pin=None) }}while replacing
"myrecipe"with eithernameif aname|lowervariable is defined in your recipe or with the lowercase name of the package in quotes.Bot commands for PR management
Please use the following BiocondaBot commands:
Everyone has access to the following BiocondaBot commands, which can be given in a comment:
@BiocondaBot please update@BiocondaBot please add labelplease review & mergelabel.@BiocondaBot please fetch artifactsYou can use this to test packages locally.
Note that the
@BiocondaBot please mergecommand is now depreciated. Please just squash and merge instead.Also, the bot watches for comments from non-members that include
@bioconda/<team>and will automatically re-post them to notify the addressed<team>.