split avx from avx2 #252

LHT129 · 2024-12-24T10:51:45Z

refactor all simd functions, just like FP32
add openblas for ubuntu-aarch deps

wxyucs · 2024-12-24T16:00:28Z

tests/test_index_old.cpp

@@ -13,18 +13,9 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

+#include "simd/simd.h"


function test cases should not depend on any internal definitions or interfaces, so don't include header files from src/

some basic func like l2sqr, innerproduct is common in functest & internal, If I write it twice, is it a bit redundant? The correctness of the simd module has been guaranteed by unittest.

src/simd/sq8_simd.h

wxyucs · 2024-12-24T16:16:40Z

src/simd/avx512.cpp

-        sum = _mm512_add_ps(sum, _mm512_mul_ps(a, b));  // accumulate the product
+        __m512 a = _mm512_loadu_ps(query + i * 16);  // load 16 floats from memory
+        __m512 b = _mm512_loadu_ps(codes + i * 16);  // load 16 floats from memory
+        sum = _mm512_fmadd_ps(a, b, sum);            // accumulate the product


LHT129 · 2024-12-25T12:09:12Z

src/simd/CMakeLists.txt

-# FIXME(LHT): cause illegal instruction on platform which has avx only
-#if (DIST_CONTAINS_AVX2)
-#    set_source_files_properties (avx.cpp PROPERTIES COMPILE_FLAGS "-mavx2 -mfma")
-#endif ()


LHT129 · 2024-12-25T12:09:19Z

src/simd/CMakeLists.txt

@@ -56,7 +57,7 @@ endmacro ()

 simd_add_definitions (DIST_CONTAINS_SSE -DENABLE_SSE=1)
 simd_add_definitions (DIST_CONTAINS_AVX -DENABLE_AVX=1)
-#simd_add_definitions (DIST_CONTAINS_AVX2 -DENABLE_AVX2=1)


inabao · 2024-12-25T11:27:14Z

src/simd/avx2.cpp

+    result += avx::SQ8ComputeL2Sqr(query + i, codes + i, lower_bound + i, diff + i, dim - i);
+    return result;
+#else
+    return vsag::avx::SQ8ComputeL2Sqr(query, codes, lower_bound, diff, dim);  // TODO


avx::SQ8ComputeL2Sqr

inabao · 2024-12-25T11:32:17Z

src/simd/avx512.cpp

    return result;
 #else
-    return generic::SQ8ComputeCodesIP(codes1, codes2, lowerBound, diff, dim);
+    return generic::SQ8ComputeCodesIP(codes1, codes2, lower_bound, diff, dim);


why here we use generic

inabao · 2024-12-25T11:32:27Z

src/simd/avx512.cpp

    return result;
 #else
-    return generic::SQ8ComputeCodesL2Sqr(codes1, codes2, lowerBound, diff, dim);
+    return generic::SQ8ComputeCodesL2Sqr(codes1, codes2, lower_bound, diff, dim);


inabao · 2024-12-25T11:32:46Z

src/simd/avx512.cpp

-}  // namespace avx512
-
-}  // namespace vsag
+}  // namespace vsag::avx512


add new line?

lint warning here

inabao · 2024-12-25T11:35:48Z

src/simd/simd.cpp

    }
    ret.dist_support_avx512f = true;
    ret.dist_support_avx512dq = true;
    ret.dist_support_avx512bw = true;
    ret.dist_support_avx512vl = true;
 #endif
-
    return ret;
 }

 DistanceFunc
 GetInnerProductDistanceFunc(size_t dim) {


Can we remove the functions? Just use vsag::InnerProductDistance replace it

too much changes, in this pr ,only split the function, but not change usage

inabao · 2024-12-25T11:36:20Z

tests/CMakeLists.txt

@@ -10,9 +10,9 @@ endif ()
 if (DIST_CONTAINS_AVX)
    target_compile_definitions (unittests PRIVATE ENABLE_AVX=1)
 endif ()
-#if (DIST_CONTAINS_AVX2)
-#    target_compile_definitions (unittests PRIVATE ENABLE_AVX2=1)
-#endif ()


LHT129 · 2024-12-25T12:09:12Z

src/simd/CMakeLists.txt

-# FIXME(LHT): cause illegal instruction on platform which has avx only
-#if (DIST_CONTAINS_AVX2)
-#    set_source_files_properties (avx.cpp PROPERTIES COMPILE_FLAGS "-mavx2 -mfma")
-#endif ()


LHT129 · 2024-12-25T12:09:19Z

src/simd/CMakeLists.txt

@@ -56,7 +57,7 @@ endmacro ()

 simd_add_definitions (DIST_CONTAINS_SSE -DENABLE_SSE=1)
 simd_add_definitions (DIST_CONTAINS_AVX -DENABLE_AVX=1)
-#simd_add_definitions (DIST_CONTAINS_AVX2 -DENABLE_AVX2=1)


src/simd/CMakeLists.txt

jiaweizone · 2024-12-25T12:56:05Z

src/simd/fp32_simd.cpp

@@ -28,6 +28,10 @@ GetFP32ComputeIP() {
    } else if (SimdStatus::SupportAVX2()) {
 #if defined(ENABLE_AVX2)
        return avx2::FP32ComputeIP;
+#endif
+    } else if (SimdStatus::SupportAVX()) {
+#if defined(ENABLE_AVX)


if not ENABLE_AVX then the if statement will be empty ?

jiaweizone · 2024-12-25T12:56:46Z

src/simd/fp32_simd.cpp

+#endif
+    } else if (SimdStatus::SupportAVX()) {
+#if defined(ENABLE_AVX)
+        return avx::FP32ComputeL2Sqr;


src/simd/generic.cpp

jiaweizone · 2024-12-25T12:58:41Z

src/simd/normalize.cpp

@@ -28,6 +28,10 @@ GetNormalize() {
    } else if (SimdStatus::SupportAVX2()) {
 #if defined(ENABLE_AVX2)
        return avx2::Normalize;
+#endif
+    } else if (SimdStatus::SupportAVX()) {
+#if defined(ENABLE_AVX)


jiaweizone · 2024-12-25T13:00:24Z

src/simd/sse.cpp

-    float sum = TmpRes[0] + TmpRes[1] + TmpRes[2] + TmpRes[3];
-
-    return sum;
+InnerProduct(const void* pVect1v, const void* pVect2v, const void* qty_ptr) {


jiaweizone

LGTM

wxyucs

lgtm

- refactor all simd functions, just like FP32 - add openblas for ubuntu-aarch deps - hgraph sq recall reduce to 0.96 Signed-off-by: LHT129 <tianlan.lht@antgroup.com>

LHT129 added the kind/improvement Code improvements (variable/function renaming, refactoring, etc. ) label Dec 24, 2024

LHT129 self-assigned this Dec 24, 2024

LHT129 requested review from wxyucs, inabao, ShawnShawnYou and jiaweizone as code owners December 24, 2024 10:51

pull-request-size bot added the size/XXL label Dec 24, 2024

LHT129 force-pushed the avx branch 5 times, most recently from 59a72c4 to 17d89c8 Compare December 24, 2024 15:53

wxyucs reviewed Dec 24, 2024

View reviewed changes

LHT129 force-pushed the avx branch 3 times, most recently from 6207ce7 to c80c83b Compare December 25, 2024 03:00

LHT129 added the version/0.12 label Dec 25, 2024

inabao reviewed Dec 25, 2024

View reviewed changes

LHT129 commented Dec 25, 2024

View reviewed changes

jiaweizone reviewed Dec 25, 2024

View reviewed changes

LHT129 force-pushed the avx branch 2 times, most recently from 9c80eb4 to c279e9f Compare December 25, 2024 14:35

jiaweizone approved these changes Dec 25, 2024

View reviewed changes

wxyucs approved these changes Dec 26, 2024

View reviewed changes

split avx from avx2

fcfc97f

- refactor all simd functions, just like FP32 - add openblas for ubuntu-aarch deps - hgraph sq recall reduce to 0.96 Signed-off-by: LHT129 <tianlan.lht@antgroup.com>

LHT129 force-pushed the avx branch from c279e9f to fcfc97f Compare December 26, 2024 03:25

ShawnShawnYou approved these changes Dec 26, 2024

View reviewed changes

LHT129 merged commit b55e104 into antgroup:main Dec 26, 2024
8 checks passed

LHT129 deleted the avx branch December 26, 2024 06:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

split avx from avx2 #252

split avx from avx2 #252

LHT129 commented Dec 24, 2024

wxyucs Dec 24, 2024

LHT129 Dec 25, 2024

wxyucs Dec 24, 2024

LHT129 Dec 25, 2024

LHT129 Dec 25, 2024

inabao Dec 25, 2024

LHT129 Dec 25, 2024

inabao Dec 25, 2024

LHT129 Dec 25, 2024

inabao Dec 25, 2024

LHT129 Dec 25, 2024

inabao Dec 25, 2024

LHT129 Dec 25, 2024

inabao Dec 25, 2024

LHT129 Dec 25, 2024

inabao Dec 25, 2024

LHT129 Dec 25, 2024

LHT129 Dec 25, 2024

LHT129 Dec 25, 2024

jiaweizone Dec 25, 2024

LHT129 Dec 25, 2024

jiaweizone Dec 25, 2024

LHT129 Dec 25, 2024

jiaweizone Dec 25, 2024

LHT129 Dec 25, 2024

jiaweizone Dec 25, 2024

LHT129 Dec 25, 2024

jiaweizone left a comment

wxyucs left a comment

split avx from avx2 #252

split avx from avx2 #252

Conversation

LHT129 commented Dec 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiaweizone left a comment

Choose a reason for hiding this comment

wxyucs left a comment

Choose a reason for hiding this comment