Skip to content

Conversation

@r-abishek
Copy link
Member

@kiritigowda @rrawther This PR starts addressing #602 on libffts.a

  • Removes libffts.a from RPP repository,
  • Add cmake support to dynamically link (.so) libffts library instead of the .a file.
  • Adds a new FindFFTS.cmake file.
  • Adds FFTS as a Submodule.

RPP tests pass locally on this PR.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses issue #602 by transitioning from static linking of libffts.a to dynamic linking of the FFTS library. The changes remove the bundled FFTS static library from the repository and implement proper CMake support for finding and linking against an externally installed FFTS library.

  • Removes libffts.a and FFTS source files from the repository
  • Adds FFTS as a git submodule and implements dynamic linking via FindFFTS.cmake
  • Updates build configuration to conditionally enable audio support based on FFTS availability

Reviewed Changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
third_party/ffts/include/ffts_attributes.h Removes FFTS header file (now handled by submodule)
third_party/ffts/include/ffts.h Removes FFTS header file (now handled by submodule)
third_party/ffts/FFTS_LICENSE Removes FFTS license file (now handled by submodule)
third_party/ffts Adds FFTS as a git submodule
src/modules/tensor/cpu/kernel/spectrogram.cpp Updates include path and removes FFTS alignment attributes
src/modules/CMakeLists.txt Removes direct FFTS include directory
cmake/FindFFTS.cmake Adds CMake module for finding FFTS library
CMakeLists.txt Implements conditional FFTS linking and install logic
.gitmodules Defines FFTS submodule configuration

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@r-abishek r-abishek changed the title Ar/build fix ffts dynamic link libffts Dynamic link and Submodule Sep 23, 2025
@kiritigowda kiritigowda self-assigned this Sep 26, 2025
@kiritigowda
Copy link
Collaborator

@r-abishek -- this PR required build scripts update for all infrastructures. Lets make those changes before merging this

@kiritigowda
Copy link
Collaborator

Requires

git submodule sync --recursive
git submodule update --init --recursive --remote

@rrawther rrawther assigned lakshmiamd and unassigned kiritigowda Sep 26, 2025
Copy link
Contributor

@rrawther rrawther left a comment

Choose a reason for hiding this comment

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

Added some comments. Can be merged after @kiritigowda approves it

@r-abishek
Copy link
Member Author

Requires

git submodule sync --recursive
git submodule update --init --recursive --remote

@kiritigowda All comments addressed here.
CI says CMake is unable to find ffts.h
Adding the above commands to bring in submodules before running the cmake should fix it

@kiritigowda
Copy link
Collaborator

@r-abishek -- one imp change required for this PR

  • If the FFT module is not found in the folder, we need to exclude this during the build, this will fall back to no FFT
/var/jenkins_home/workspace/mainline_precheckin_rpp_PR-619/jlMlSKTL0/rpp/src/modules/tensor/cpu/kernel/spectrogram.cpp:26:10: fatal error: 'ffts/ffts.h' file not found
   26 | #include <ffts/ffts.h>
      |          ^~~~~~~~~~~~~
1 error generated when compiling for gfx1030.
make[2]: *** [src/modules/CMakeFiles/modules.dir/build.make:2218: src/modules/CMakeFiles/modules.dir/tensor/cpu/kernel/spectrogram.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [CMakeFiles/Makefile2:918: src/modules/CMakeFiles/modules.dir/all] Error 2
make: *** [Makefile:166: all] Error 2
+ sudo make install
[  0%] Building CXX object src/modules/CMakeFiles/modules.dir/tensor/cpu/kernel/spectrogram.cpp.o
/var/jenkins_home/workspace/mainline_precheckin_rpp_PR-619/jlMlSKTL0/rpp/src/modules/tensor/cpu/kernel/spectrogram.cpp:26:10: fatal error: 'ffts/ffts.h' file not found
   26 | #include <ffts/ffts.h>

@codecov
Copy link

codecov bot commented Oct 29, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #619      +/-   ##
===========================================
+ Coverage    88.24%   88.46%   +0.22%     
===========================================
  Files          195      179      -16     
  Lines        82712    81314    -1398     
===========================================
- Hits         72985    71932    -1053     
+ Misses        9727     9382     -345     

see 23 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@r-abishek
Copy link
Member Author

@kiritigowda cmake changes added

@kiritigowda kiritigowda added the enhancement New feature or request label Nov 17, 2025
@kiritigowda kiritigowda changed the title libffts Dynamic link and Submodule ROCm 8.0.0 -- libffts Dynamic link and Submodule Nov 17, 2025
@rrawther
Copy link
Contributor

rrawther commented Dec 4, 2025

@kiritigowda : Since we are branched out of 7.2, can we merge this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:precheckin enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants