Xsimd by mkatliar · Pull Request #12 · mkatliar/blast

mkatliar · 2024-07-23T15:25:39Z

SIMD operations reimplemented using xsimd library.
Architecture-specific code moved to blast/math/simd/arch.

roversch

Great! All in all, the code became a lot smaller.

I don't have any blocking comments, just minor suggestions and questions.

roversch · 2024-08-05T09:44:15Z

+        sudo apt-get update
+        sudo apt install libboost-exception-dev libbenchmark-dev -y
+
+    - name: Install LLVM and Clang 18


Any way to make this version indepedent?

It should be not a problem. But the CI system has clang-14, and on my system the oldest clang I can install with apt is clang-15. Compiling with older clang requires adding the typename keyword here and there. Also the code builds with gcc now, but I am not sure about older versions.

I suggest that we keep it in mind and come back to the compiler support issue later.

roversch · 2024-08-05T09:44:27Z

        cmake -B ${{github.workspace}}/build \
        -DCMAKE_BUILD_TYPE=${{env.BUILD_TYPE}} \
-        -DCMAKE_CXX_COMPILER=clang++ \
+        -DCMAKE_CXX_COMPILER=clang++-18 \


Answered above

roversch · 2024-08-05T09:45:04Z

    && cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo \
        -DCMAKE_CXX_COMPILER="clang++-15" \
-        -DCMAKE_CXX_FLAGS="-march=native -mfma -mavx -mavx2 -msse4 -fno-math-errno" \
+        -DCMAKE_CXX_FLAGS="-march=native -mfma -mavx -mavx2 -msse4 -fno-math-errno -DXSIMD_DEFAULT_ARCH='fma3<avx2>'" \


Would be nice to detect this automatically (but that is maybe out of scope of this pull request)

I believe xsimd does automatically deduce the best possible architecture if you don't specify one. For now I made it explicit because currently we only support avx2.

roversch · 2024-08-05T09:51:01Z

+
+    template <typename Arch>
+    requires std::is_base_of_v<xsimd::avx2, Arch>
+    inline std::tuple<xsimd::batch<float, Arch>, xsimd::batch<std::int32_t, Arch>> imax(xsimd::batch<float, Arch> const& v1, xsimd::batch<std::int32_t, Arch> const& idx) noexcept


Some documentation would be nice here. If it's a standard approach, at least a link or so.

Added in 2f757bb

roversch · 2024-08-05T09:55:57Z

-        using MaskType = typename Simd<std::remove_cv_t<T>>::MaskType;
        using SimdVecType = SimdVec<std::remove_cv_t<T>>;
+        using IntrinsicType = SimdVecType::IntrinsicType;
+        using MaskType = SimdMask<std::remove_cv_t<T>>;


Just a question: I guess you remove const and volatile qualifiers here, because otherwise you run into problems with const correctness?

Yes, without it I used to run into compiler errors. We can try removing it and see what happens.

roversch · 2024-08-05T09:59:13Z

+                v[i] = mask[i] ? ptr_[spacing_ * i] : T {};

-            return SimdVecType {v};
+            return SimdVecType {v, false};


This was surprising to me. What is the false doing here?

It specifies whether v is aligned or not.

roversch · 2024-08-05T09:59:54Z

        return ReturnType(*pm);
    }
-}
+}


How did this even compile before if you're missing braces :D

I think this class was not used. But it will come back to life in the next PR.

roversch · 2024-08-05T10:01:36Z


-        using MaskType = typename Simd<ET2>::MaskType;
-        using IntType = typename Simd<ET2>::IntType;
+        using MaskType = SimdMask<ET2, xsimd::default_arch>;


The xsimd::default_arch is implied here I guess? Not really necessary?

True. I think I wrote this line before I added a default value for Arch in SimdMask.

roversch · 2024-08-05T10:03:18Z

+     *
+     * @return Number of SIMD registers for AVX2
+     */
+    std::size_t constexpr registerCapacity(xsimd::avx2)


Maybe add a todo for other architectures?

The compiler will force us to add it when we try to compile for a different arch.

roversch · 2024-08-05T10:05:31Z

-    size_t constexpr SimdSize_v = SimdSize<T>::value;
-}
+    template <typename T, typename Arch = xsimd::default_arch>
+    std::size_t constexpr SimdSize_v = xsimd::batch<std::remove_cv_t<T>, Arch>::size;


Isn't this what SS was doing before? Do we have some duplication perhaps?

SS is just a shorthand to avoid long expressions like SimdSize_v<double> or xsimd::batch<double, Arch>::size. SS is a private implementation detail. SimdSize_v is supposed to be a shorter version of xsimd::batch<double, Arch>::size, it also hides the dependency on xsimd. We can replace it with something more modern and convenient, if we want, e.g.

template <typename T, typename Arch> constexpr std::size_t simdSize(Arch arch) { return xsimd::batch<T, Arch>::size; }

mkatliar added 2 commits July 23, 2024 16:05

Removed load() and store() from StaticPanelMatrix

0b9550a

Removed load() functions from Simd.hpp

b6eedca

mkatliar linked an issue Jul 23, 2024 that may be closed by this pull request

Switch to xsimd #11

Closed

mkatliar added 3 commits July 25, 2024 23:05

SimdVec implementation using xsimd

231d175

Moved operator[] to SimdVecBase

be5b0fd

Generic member functions in SimdVec

e27037d

mkatliar force-pushed the xsimd branch from 4e04980 to 06d8426 Compare July 26, 2024 13:05

Install xsimd step

39856a3

mkatliar force-pushed the xsimd branch from 06d8426 to 39856a3 Compare July 26, 2024 13:07

mkatliar added 2 commits July 26, 2024 15:11

Add missing "typename " keyword

7aa0563

Update "Build xsimd" step

d663683

mkatliar requested a review from roversch August 1, 2024 07:49

mkatliar added 11 commits August 1, 2024 13:09

Simd<> specializations working for both avx2 and fma3<avx2>

8b9124b

Remove XSIMD_DEFAULT_ARCH setting from CMakeLists.txt

17521ea

Code compiles for both avx2 and fma3<avx2>

c3adc2a

SimdVec decoupled from SimdVecBase

c21776f

SimdMask<> is an alias for xsimd::batch_bool<>

7ff99d0

Added SimdIndex, removed some messy types.

e7ed800

Got rid of Simd and SimdTraits classes

25538a0

Got rid of the old Simd.hpp header

87d7565

Default architecture changed to fma3<avx2> in Dockerfile

f91233a

Got rid of SimdVecBase

4b50684

Made the code compile with gcc (except internal compiler errors)

2c56305

mkatliar force-pushed the xsimd branch 5 times, most recently from d0066dc to 5c95f8c Compare August 2, 2024 16:19

Build with clang-16

7efe21b

mkatliar force-pushed the xsimd branch from 5c95f8c to 7efe21b Compare August 2, 2024 16:20

Build with clang-18

84695b9

mkatliar force-pushed the xsimd branch from d26264e to 84695b9 Compare August 2, 2024 16:24

mkatliar added 5 commits August 3, 2024 04:23

Removed RegisterMatrix specializations

c3d5882

Removed avx2 SimdVec specializations

d789021

Removed avx2 hsum()

01754af

Arch-specific code moved to blast/math/simd/arch

1b8c62c

Builds with gcc

9ebf1ed

mkatliar marked this pull request as ready for review August 3, 2024 03:34

mkatliar self-assigned this Aug 3, 2024

roversch approved these changes Aug 5, 2024

View reviewed changes

Documented iamax()

2f757bb

mkatliar merged commit 999b6c7 into master Aug 5, 2024

mkatliar deleted the xsimd branch August 5, 2024 12:18

roversch pushed a commit that referenced this pull request Aug 13, 2024

Low-level operations using xsimd (#12)

ed88356

roversch pushed a commit that referenced this pull request Sep 18, 2024

Low-level operations using xsimd (#12)

d3641d1

Conversation

mkatliar commented Jul 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roversch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkatliar Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mkatliar commented Jul 23, 2024 •

edited

Loading

mkatliar Aug 5, 2024 •

edited

Loading