From 832361e2d7711b9cb7f828bdc6374e6ab3916c5c Mon Sep 17 00:00:00 2001 From: Denes Tarjan Date: Fri, 22 Mar 2024 10:55:41 +0100 Subject: [PATCH] [NFC] Optimize out unused code paths compile time Improve test coverage by making try_avoid_tail_loop constexpr --- intrinsiccv/include/intrinsiccv/operations.h | 25 ++++++++----------- intrinsiccv/include/intrinsiccv/traits.h | 15 ++++++++--- intrinsiccv/include/intrinsiccv/utils.h | 9 +++---- .../add_abs_with_threshold_neon.cpp | 3 ++- .../src/conversions/yuv_to_rgb_neon.cpp | 3 +-- test/api/test_add_abs_with_threshold.cpp | 20 +++++++++++++++ test/framework/operation.h | 15 ++++++++--- 7 files changed, 60 insertions(+), 30 deletions(-) diff --git a/intrinsiccv/include/intrinsiccv/operations.h b/intrinsiccv/include/intrinsiccv/operations.h index d1440ea8a..e369f6f92 100644 --- a/intrinsiccv/include/intrinsiccv/operations.h +++ b/intrinsiccv/include/intrinsiccv/operations.h @@ -126,6 +126,14 @@ class OperationBase { concrete_operation_type_t>; } + // Returns true if the innermost operation tries to avoid tail loop, otherwise + // false. + static constexpr bool try_to_avoid_tail_loop() + INTRINSICCV_STREAMING_COMPATIBLE { + return ::INTRINSICCV_TARGET_NAMESPACE::try_to_avoid_tail_loop< + concrete_operation_type_t>; + } + protected: // Constructor is protected so that only derived classes can instantiate. explicit OperationBase(OperationType &operation) @@ -159,11 +167,7 @@ class RowBasedOperation : public OperationBase { public: explicit RowBasedOperation(OperationType &operation) INTRINSICCV_STREAMING_COMPATIBLE - : OperationBase(operation), - try_avoid_tail_loop_{false} {} - - // Instructs the loop logic to try to avoid the tail loop. - void try_avoid_tail_loop() { try_avoid_tail_loop_ = true; } + : OperationBase(operation) {} // NOLINTBEGIN(cppcoreguidelines-avoid-goto, hicpp-avoid-goto) template @@ -189,15 +193,12 @@ class RowBasedOperation : public OperationBase { // Process leftover bytes on the unrolled-once vector path, if requested and // possible. - if constexpr (OperationType::is_unrolled_once()) { - if (try_avoid_tail_loop_ && - loop.try_avoid_tail_loop( + if constexpr (OperationType::is_unrolled_once() && OperationType::try_to_avoid_tail_loop()) { + if (loop.try_avoid_tail_loop( [&](size_t backward_step) INTRINSICCV_STREAMING_COMPATIBLE { // Adjust pointers backwards to include // the leftover bytes. ((columns -= backward_step), ...); - // Accept the step-back decision. - return true; })) { goto avoid_tail_loop; } @@ -211,10 +212,6 @@ class RowBasedOperation : public OperationBase { // clang-format on } // NOLINTEND(cppcoreguidelines-avoid-goto, hicpp-avoid-goto) - - private: - // True if tail loop should be avoided at all costs. - bool try_avoid_tail_loop_; }; // end of class RowBasedOperation // Facade to offer a simplified row-based operation interface. diff --git a/intrinsiccv/include/intrinsiccv/traits.h b/intrinsiccv/include/intrinsiccv/traits.h index dd1518ec5..2a32ebc9b 100644 --- a/intrinsiccv/include/intrinsiccv/traits.h +++ b/intrinsiccv/include/intrinsiccv/traits.h @@ -36,27 +36,36 @@ template using remove_streaming_compatible_t = typename remove_streaming_compatible::type; -// Tags an operation which would process data of one vector per iteration. +// Tags an operation to process data of one vector per iteration. class UnrollOnce {}; // Returns true if an instance derives from UnrollOnce, otherwise false. template constexpr bool is_unrolled_once = std::is_base_of_v; -// Tags an operation which would process data of two vectors per iteration. +// Tags an operation to process data of two vectors per iteration. class UnrollTwice {}; // Returns true if an instance derives from UnrollTwice, otherwise false. template constexpr bool is_unrolled_twice = std::is_base_of_v; -// Tags an operation which uses a different vector path to process the tail. +// Tags an operation to use a different vector path to process the tail. class UsesTailPath {}; // Returns true if an instance derives from UsesTailPath, otherwise false. template constexpr bool uses_tail_path = std::is_base_of_v; +// Tags an operation to avoid the tail loop if possible, by rewinding and doing +// a vector path. +class TryToAvoidTailLoop {}; + +// Returns true if an instance derives from TryToAvoidTailLoop, otherwise false. +template +constexpr bool try_to_avoid_tail_loop = + std::is_base_of_v; + // Primary template to handle types without T::vector_path() method. template constexpr bool has_vector_path = false; diff --git a/intrinsiccv/include/intrinsiccv/utils.h b/intrinsiccv/include/intrinsiccv/utils.h index 606c0dbad..e58fd4f82 100644 --- a/intrinsiccv/include/intrinsiccv/utils.h +++ b/intrinsiccv/include/intrinsiccv/utils.h @@ -194,12 +194,9 @@ class LoopUnroll final { return false; } - if (INTRINSICCV_LIKELY(callback(step() - remaining_length()))) { - length_ = step(); - return true; - } - - return false; + callback(step() - remaining_length()); + length_ = step(); + return true; } private: diff --git a/intrinsiccv/src/arithmetics/add_abs_with_threshold_neon.cpp b/intrinsiccv/src/arithmetics/add_abs_with_threshold_neon.cpp index 23ff6e9d8..8f67f467d 100644 --- a/intrinsiccv/src/arithmetics/add_abs_with_threshold_neon.cpp +++ b/intrinsiccv/src/arithmetics/add_abs_with_threshold_neon.cpp @@ -11,7 +11,8 @@ namespace intrinsiccv::neon { template class SaturatingAddAbsWithThreshold final : public UnrollOnce, - public UnrollTwice { + public UnrollTwice, + public TryToAvoidTailLoop { public: using VecTraits = neon::VecTraits; using VectorType = typename VecTraits::VectorType; diff --git a/intrinsiccv/src/conversions/yuv_to_rgb_neon.cpp b/intrinsiccv/src/conversions/yuv_to_rgb_neon.cpp index be9c92b74..e2209cb75 100644 --- a/intrinsiccv/src/conversions/yuv_to_rgb_neon.cpp +++ b/intrinsiccv/src/conversions/yuv_to_rgb_neon.cpp @@ -11,7 +11,7 @@ namespace intrinsiccv::neon { template -class YUVSpToRGBxOrBGRx final : public UnrollOnce { +class YUVSpToRGBxOrBGRx final : public UnrollOnce, public TryToAvoidTailLoop { public: using VecTraits = neon::VecTraits; using ScalarType = VecTraits::ScalarType; @@ -307,7 +307,6 @@ intrinsiccv_error_t yuv2rgbx_operation( OperationContextAdapter context_adapter{remaining_path_adapter}; ParallelRowsAdapter parallel_rows_adapter{context_adapter}; RowBasedOperation row_based_operation{parallel_rows_adapter}; - row_based_operation.try_avoid_tail_loop(); zip_parallel_rows(row_based_operation, rect, y_rows, uv_rows, rgbx_rows); return INTRINSICCV_OK; } diff --git a/test/api/test_add_abs_with_threshold.cpp b/test/api/test_add_abs_with_threshold.cpp index eae22c00b..e2be07688 100644 --- a/test/api/test_add_abs_with_threshold.cpp +++ b/test/api/test_add_abs_with_threshold.cpp @@ -134,26 +134,46 @@ TYPED_TEST_SUITE(SaturatingAddAbsWithThresholdTest, ElementTypes); TYPED_TEST(SaturatingAddAbsWithThresholdTest, TestPositive) { SaturatingAddAbsWithThresholdTestPositive{}.test(); SaturatingAddAbsWithThresholdTestPositive{}.with_padding(1).test(); + SaturatingAddAbsWithThresholdTestPositive{} + .with_padding(1) + .with_width(test::Options::vector_lanes() - 1) + .test(); } TYPED_TEST(SaturatingAddAbsWithThresholdTest, TestNegative) { SaturatingAddAbsWithThresholdTestNegative{}.test(); SaturatingAddAbsWithThresholdTestNegative{}.with_padding(1).test(); + SaturatingAddAbsWithThresholdTestNegative{} + .with_padding(1) + .with_width(test::Options::vector_lanes() - 1) + .test(); } TYPED_TEST(SaturatingAddAbsWithThresholdTest, TestMin) { SaturatingAddAbsWithThresholdTestMin{}.test(); SaturatingAddAbsWithThresholdTestMin{}.with_padding(1).test(); + SaturatingAddAbsWithThresholdTestMin{} + .with_padding(1) + .with_width(test::Options::vector_lanes() - 1) + .test(); } TYPED_TEST(SaturatingAddAbsWithThresholdTest, TestZero) { SaturatingAddAbsWithThresholdTestZero{}.test(); SaturatingAddAbsWithThresholdTestZero{}.with_padding(1).test(); + SaturatingAddAbsWithThresholdTestZero{} + .with_padding(1) + .with_width(test::Options::vector_lanes() - 1) + .test(); } TYPED_TEST(SaturatingAddAbsWithThresholdTest, TestMax) { SaturatingAddAbsWithThresholdTestMax{}.test(); SaturatingAddAbsWithThresholdTestMax{}.with_padding(1).test(); + SaturatingAddAbsWithThresholdTestMax{} + .with_padding(1) + .with_width(test::Options::vector_lanes() - 1) + .test(); } TYPED_TEST(SaturatingAddAbsWithThresholdTest, NullPointer) { diff --git a/test/framework/operation.h b/test/framework/operation.h index 29743297f..c338a79fe 100644 --- a/test/framework/operation.h +++ b/test/framework/operation.h @@ -33,6 +33,13 @@ class OperationTest { return *this; } + // Sets the number of padding bytes at the end of rows. + OperationTest& with_width( + size_t width) { + width_ = width; + return *this; + } + void test() { for (auto& input : inputs_) { input = ArrayType{width(), height(), padding()}; @@ -101,10 +108,7 @@ class OperationTest { virtual size_t height() { return test_elements().size(); } // Tested number of elements in a row. - size_t width() const { - // Sufficient number of elements to exercise both vector and scalar paths. - return 3 * test::Options::vector_lanes() - 1; - } + virtual size_t width() const { return width_; } // Returns the number of padding bytes at the end of rows. size_t padding() const { return padding_; } @@ -127,6 +131,9 @@ class OperationTest { std::array actual_; // Number of padding bytes at the end of rows. size_t padding_{0}; + // Tested number of elements in a row. + // Sufficient number of elements to exercise both vector and scalar paths. + size_t width_{3 * test::Options::vector_lanes() - 1}; }; // end of class OperationTest template -- GitLab