Skip to content

hal/riscv-rvv: refactor the building process#27301

Merged
asmorkalov merged 9 commits intoopencv:4.xfrom
fengyuentau:4x/hal/riscv_rvv/refactor_build
May 14, 2025
Merged

hal/riscv-rvv: refactor the building process#27301
asmorkalov merged 9 commits intoopencv:4.xfrom
fengyuentau:4x/hal/riscv_rvv/refactor_build

Conversation

@fengyuentau
Copy link
Copy Markdown
Member

@fengyuentau fengyuentau commented May 12, 2025 •
edited
Loading

Current hal/riscv-rvv is built with all headers without building an object. This slows down the compilation progress, especially when re-compiling for minor changes in those headers (~170 files need to be re-compiled). This patch solves the problem.

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

@fengyuentau fengyuentau added this to the 4.12.0 milestone May 12, 2025
@fengyuentau fengyuentau requested a review from asmorkalov May 12, 2025 16:24
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented May 13, 2025

There is some strange test failure with RISC-V Clang:

[ RUN      ] Core_HAL/mathfuncs.accuracy/6, where GetParam() = (6, HAL_SQRT)
/home/ci/opencv/modules/core/test/test_hal_core.cpp:141: Failure
Expected: (cvtest::norm(dst, dst0, NORM_INF | NORM_RELATIVE)) <= (eps), actual: 0.902721 vs 1e-10
[  FAILED  ] Core_HAL/mathfuncs.accuracy/6, where GetParam() = (6, HAL_SQRT) (1 ms)

@asmorkalov asmorkalov self-assigned this May 13, 2025
fengyuentau
fengyuentau commented May 13, 2025
View reviewed changes
@fengyuentau
Copy link
Copy Markdown
Member Author

fengyuentau commented May 13, 2025

By the way, I tried to put

#include "opencv2/imgproc/hal/interface.h"

in rvv_hal.hpp but it cannot be found during compiling core. This is solved by putting in common.hpp under src/imgproc. Any hints?

asmorkalov
asmorkalov reviewed May 13, 2025
View reviewed changes
hal/riscv-rvv/src/imgproc/common.hpp Outdated
{
if ((unsigned)p < (unsigned)len)
;
else if (borderType == BORDER_REPLICATE)
Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov May 13, 2025

Choose a reason for hiding this comment

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

Please use CV_HAL_BORDER_XXX constants from modules/core/include/opencv2/core/hal/interface.h

Copy link
Copy Markdown
Member Author

@fengyuentau fengyuentau May 13, 2025

Choose a reason for hiding this comment

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

Done.

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented May 13, 2025 •
edited
Loading

By the way, I tried to put

#include "opencv2/imgproc/hal/interface.h"

in rvv_hal.hpp but it cannot be found during compiling core. This is solved by putting in common.hpp under src/imgproc. Any hints?

The HAL header is used in several places: HAL build itself and all modules, where it's injected. For the first case you have target_include_directories in the HAL cmake. For all other cases the include directories are defined by the module itself.
Looks like Carotene had similar issue before and I see the following hack:

#if defined OPENCV_IMGPROC_HAL_INTERFACE_H

struct cvhalFilter2D;

struct FilterCtx
{
    CAROTENE_NS::Size2D ksize;
    CAROTENE_NS::s16* kernel_data;
    CAROTENE_NS::BORDER_MODE border;
};
inline int TEGRA_FILTERINIT(cvhalFilter2D **context, uchar *kernel_data, size_t kernel_step, int kernel_type, int kernel_width, int kernel_height,
                            int max_width, int max_height, int src_type, int dst_type, int borderType, double delta, int anchor_x, int anchor_y, bool allowSubmatrix, bool allowInplace)
{
...

#endif

There is no interface include for imgproc, and the block will be skipped in other modules than imgproc. The OPENCV_IMGPROC_HAL_INTERFACE_H defined in the interface header. The interface header for imgproc is included by the module itself.

@fengyuentau
Copy link
Copy Markdown
Member Author

fengyuentau commented May 13, 2025

@asmorkalov Thank you for the explanation! I kind of understand the problem now but just had a failed try on moving the include back to imgproc.hpp. Changes are below:

Details 
diff --git a/hal/riscv-rvv/include/imgproc.hpp b/hal/riscv-rvv/include/imgproc.hpp
index 66c75786a0..3ce1f82ef2 100644
--- a/hal/riscv-rvv/include/imgproc.hpp
+++ b/hal/riscv-rvv/include/imgproc.hpp
@@ -5,6 +5,10 @@
 #ifndef OPENCV_RVV_HAL_IMGPROC_HPP
 #define OPENCV_RVV_HAL_IMGPROC_HPP
 
+#if defined (OPENCV_IMGPROC_HAL_INTERFACE_H)
+
+#include "opencv2/imgproc/hal/interface.h"
+
 struct cvhalFilter2D;
 
 namespace cv { namespace rvv_hal { namespace imgproc {
@@ -246,4 +250,6 @@ int cvtBGRtoBGR(const uchar * src_data, size_t src_step, uchar * dst_data, size_
 
 }}} // cv::rvv_hal::imgproc
 
+#endif // OPENCV_IMGPROC_HAL_INTERFACE_H
+
 #endif // OPENCV_RVV_HAL_IMGPROC_HPP
diff --git a/hal/riscv-rvv/src/imgproc/common.hpp b/hal/riscv-rvv/src/imgproc/common.hpp
index 819b43421c..f8a8d536f4 100644
--- a/hal/riscv-rvv/src/imgproc/common.hpp
+++ b/hal/riscv-rvv/src/imgproc/common.hpp
@@ -10,7 +10,6 @@
 #define OPENCV_HAL_RVV_IMGPROC_COMMON_HPP_INCLUDED
 
 #include "opencv2/core/hal/interface.h"
-#include "opencv2/imgproc/hal/interface.h"
 
 namespace cv { namespace rvv_hal { namespace imgproc { namespace common {

It seems that HAL is built before opencv modules?

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented May 13, 2025

It seems that HAL is built before opencv modules?

Yes, it should. The library for linkage should be ready for module build.

@fengyuentau
Copy link
Copy Markdown
Member Author

fengyuentau commented May 13, 2025

It seems that HAL is built before opencv modules?

Yes, it should. The library for linkage should be ready for module build.

I guess, I will leave it as-is for now for the problem.

@fengyuentau
Copy link
Copy Markdown
Member Author

fengyuentau commented May 14, 2025 •
edited
Loading

Found regressions in imgproc (testd on K1):

[  FAILED  ] 8 tests, listed below:
[  FAILED  ] Size_MatType_OutMatDepth_integral_sqsum.integral_sqsum/1, where GetParam() = (640x480, 8UC1, CV_32F)
[  FAILED  ] Size_MatType_OutMatDepth_integral_sqsum.integral_sqsum/4, where GetParam() = (640x480, 8UC2, CV_32F)
[  FAILED  ] Size_MatType_OutMatDepth_integral_sqsum.integral_sqsum/10, where GetParam() = (640x480, 8UC4, CV_32F)
[  FAILED  ] Size_MatType_OutMatDepth_integral_sqsum.integral_sqsum/16, where GetParam() = (1280x720, 8UC2, CV_32F)
[  FAILED  ] Size_MatType_OutMatDepth_integral_sqsum.integral_sqsum/22, where GetParam() = (1280x720, 8UC4, CV_32F)
[  FAILED  ] Size_MatType_OutMatDepth_integral_sqsum.integral_sqsum/25, where GetParam() = (1920x1080, 8UC1, CV_32F)
[  FAILED  ] Size_MatType_OutMatDepth_integral_sqsum.integral_sqsum/28, where GetParam() = (1920x1080, 8UC2, CV_32F)
[  FAILED  ] Size_MatType_OutMatDepth_integral_sqsum.integral_sqsum/34, where GetParam() = (1920x1080, 8UC4, CV_32F)

Confirmed that they are introduced from the previous optimization of integral: #27060 (comment).

@asmorkalov I believe they should be fixed in a separate PR.

@fengyuentau fengyuentau changed the title hal/riscv-rvv: refactor the building process to make it faster in re-compiling hal/riscv-rvv: refactor the building process May 14, 2025
@asmorkalov asmorkalov merged commit 547cef4 into opencv:4.x May 14, 2025
28 checks passed
@fengyuentau fengyuentau deleted the 4x/hal/riscv_rvv/refactor_build branch May 14, 2025 13:29
@asmorkalov asmorkalov mentioned this pull request May 27, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asmorkalov asmorkalov asmorkalov approved these changes

+1 more reviewer

@jshingala jshingala jshingala approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@asmorkalov asmorkalov

Labels

category: build/install platform: riscv

Projects

None yet

Milestone

4.12.0

Development

Successfully merging this pull request may close these issues.

3 participants

@fengyuentau @asmorkalov @jshingala