Add Multi-threaded DOTC, GEMV, GEMM along with BLAS Support #155

maliasadi · 2021-10-13T14:09:07Z

Context:
Add Cache-Friendly and Multi-threaded DOTC, GEMV, GEMM along with BLAS Support to PL-lightning

Description of the Change:
Update innerProd and innerProdC.
Add Transpose, matrixVecProd, and matrixMatProd.

Benefits:
Utilizing BLAS operations: inner, matrix-vector, and matrix-matrix products are orders of magnitude faster than plain implementation of such arithmetic. This PR adds BLAS support as well as a cache-friendly and multi-threaded implementation of these operations in PL-lightning.

Possible Drawbacks:

Related GitHub Issues:

codecov · 2021-10-13T14:11:18Z

Codecov Report

Merging #155 (1de986e) into master (3cc54a3) will not change coverage.
The diff coverage is n/a.

❗ Current head 1de986e differs from pull request most recent head 8b9bc71. Consider uploading reports for the commit 8b9bc71 to get more accurate results

@@            Coverage Diff            @@
##            master      #155   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          182       182           
=========================================
  Hits           182       182

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3cc54a3...8b9bc71. Read the comment docs.

github-actions · 2021-10-13T14:11:38Z

Test Report (C++) on Ubuntu

      1 files ±    0       1 suites ±0 0s ⏱️ ±0s
  345 tests +  18   345 ✔️ +  18 0 💤 ±0 0 ❌ ±0
1 919 runs +734 1 919 ✔️ +734 0 💤 ±0 0 ❌ ±0

Results for commit 8b9bc71. ± Comparison against base commit 3cc54a3.

♻️ This comment has been updated with latest results.

pennylane_lightning/src/util/Util.hpp

mlxd

Hey Ali, just left a few comments here.

…eAI/pennylane-lightning into faster_linalgeb_support

mlxd · 2021-10-18T08:34:24Z

pennylane_lightning/src/util/Util.hpp

-            cblas_zdotu_sub(data_size, data_1, 1, data_2, 1, &result);
-        }
-    } else {
+#if defined(USE_CBLAS) && USE_CBLAS


Does if constexpr not work here? I'd prefer it if we could use it instead of the preprocessor where possible. Any non-enabled #ifdef code will never hit the compiler after the preprocessing step, whereas anything with if constexpr will, and hence can be checked for correctness, even if not used.

if constexpr works only for innerProd and innerProdC, but not for matrixVecProd and matrixMatProd. So, to be consistent, I switched using #ifdef for all places using BLAS operations. Let me know if you still think that using if constexpr is the better idea for innerProd and innerProdC?

I think we should figure out how to keep if constexpr in here as it will be much safer and maintainable.

In this case, using define directive to define undefined BLAS variables in matrixVecProd and matrixMatProd is a safe and practical approach to tackle this problem. What do you think?

For the top-level variables, maybe so, though I still think enabling this with if constexpr is the way to go wherever we can. With the preprocessor route half the code gets disabled before hitting the compiler; going with if constexpr allows for better static code analysis, and is a feature of the modern uses of the C++. Avoiding the preprocessor unless required has become a common theme in many C++ articles (https://cor3ntin.github.io/posts/undef_preprocessor/ is a good example). If using if constexpr is not possible, maybe in this instance many of the helpers in the type_traits library could help out here (https://en.cppreference.com/w/cpp/header/type_traits), such as enable_if for example.

As for keeping defines where needed at the top of the file, that can be fine, but the approach as taken in https://blog.tartanllama.xyz/if-constexpr/ with enums is probably preferable where possible. Thanks for tolerating me on this 😄

Thanks for clarifying your concerns. This is also a great read on if constexpr. I get it now what to do to "hopefully" address all the concerns within #ifdef blocks and define macros 😅

pennylane_lightning/src/util/Util.hpp

…htning into faster_linalgeb_support

mlxd · 2021-10-20T10:26:57Z

pennylane_lightning/src/util/Util.hpp

+#define CblasTrans 0
+#define CblasNoTrans 0
+#define CblasRowMajor 0


Since these flags are used to select logical options, defining them with an enum may be a safer move. This can be placed in an anonymous namespace to enable visibility within the file only.

This article by Sy Brand who works on Microsoft's MSVC compiler has some good examples of doing conditional compilation for different operating systems.

I guess it will be fine to mirror the enum from here, with the same values if not already defined.

mlxd · 2021-10-20T10:29:11Z

pennylane_lightning/src/util/Util.hpp

+#define DOTU_STD_CROSSOVER (1 << 10)
+#define DOTC_STD_CROSSOVER (1 << 10)


These may also be better used as template non-type arguments in the functions they are used in. This way we can conditional compile it using templating, and not be required to specify it as a defined magic value.

pennylane_lightning/src/util/Util.hpp

mlxd

Hey Ali, thanks for the changes here. Though, I noticed some things (listed below). A note too when building for OpenBLAS support: the resulting .so fails to link against the OpenBLAS library (not sure of the fix here).

Also, I am seeing this implementation slower for real use-cases (running Trevor's benchmark scripts for the Jacobain), in comparison to the non-BLAS implementations. I wonder if there is something we need to consider with tuning the methods to improve performance here.

mlxd · 2021-10-25T10:56:47Z

pennylane_lightning/src/util/Util.hpp

+constexpr bool USE_CBLAS = false;
+#endif
+
+#ifndef CBLAS_TRANSPOSE


This fails to catch the definition of CBLAS_TRANSPOSE when building against OpenBLAS on my machine (g++ v11.1), due to multiple definitions.

mlxd · 2021-10-25T10:57:31Z

pennylane_lightning/src/util/Util.hpp

+};
+#endif
+
+#ifndef CBLAS_LAYOUT


Similar here. I wonder if there is a better way of scoping these so they do not have problems with the already defined CBLAS versions.

maliasadi added 17 commits September 24, 2021 15:01

Add the multi-threaded inner-product support

5a1ccb5

Update inner-product functions

a3c1d9a

Update innerProd using the best available method

392f409

Update formatting

4378436

Update innerProdC using the best available method

917f8ca

Update formatting

d4f4aa5

Update Util headers

690e3c3

Add matrixVecProd using the best available method

37fd253

Update formatting

9be9cbe

Add cache-friendly matrix transpose

0cd11bd

Update functions descriptions

05b9760

Add matrixMatProd using the best available method

1914111

Update matrixMatProd

df040b8

Fix some issues with matrixMatProd transpose=true

fa45256

Update matrixMatProd

2f37f2c

Update formatting

d95c4cd

Update Log

411b048

maliasadi added 2 commits October 13, 2021 11:31

Add matrixVecProd tests

12a6e0c

Update matrixMatProd tests

389dc85

maliasadi changed the title ~~Add Cache-Friendly DOTC, GEMV, GEMM along with BLAS Support~~ Add Multi-threaded DOTC, GEMV, GEMM along with BLAS Support Oct 13, 2021

maliasadi added 8 commits October 13, 2021 13:37

Update matrixMatProd tests

ea78fa8

Update matrixMatProd tests

35f7ac7

Update tests and formatting

0dc7d13

Apply clang-tidy suggestions

352f836

Add omp support for innerProd(C)

b6d5c21

Add omp support for matrixVecProd

51f35fb

Add omp support for matrixVecProd

323c404

Add omp support for matrixMatProd

5853103

mlxd reviewed Oct 15, 2021

View reviewed changes

pennylane_lightning/src/util/Util.hpp Outdated Show resolved Hide resolved

mlxd reviewed Oct 15, 2021

View reviewed changes

pennylane_lightning/src/util/Util.hpp Outdated Show resolved Hide resolved

mlxd reviewed Oct 15, 2021

View reviewed changes

pennylane_lightning/src/util/Util.hpp Outdated Show resolved Hide resolved

mlxd reviewed Oct 15, 2021

View reviewed changes

pennylane_lightning/src/util/Util.hpp Outdated Show resolved Hide resolved

mlxd reviewed Oct 15, 2021

View reviewed changes

maliasadi added 7 commits October 15, 2021 10:18

Merge branch 'master' into faster_linalgeb_support

49bdbf9

Apply code review suggestions

8c405ff

Merge branch 'faster_linalgeb_support' of https://github.com/PennyLan…

7bc5b67

…eAI/pennylane-lightning into faster_linalgeb_support

Apply clang-tidy-11 suggestions

49851af

Update variable names

f9d01f7

Update Transpose

a6a8ddd

Apply code review suggestions

292928c

mlxd reviewed Oct 18, 2021

View reviewed changes

maliasadi added 3 commits October 18, 2021 10:02

Fix the issue with MacOS wheel

808e33c

Merge branch 'master' of https://github.com/PennyLaneAI/pennylane-lig…

87931f7

…htning into faster_linalgeb_support

Apply code review suggestions

5fc0756

mlxd requested changes Oct 20, 2021

View reviewed changes

maliasadi and others added 3 commits October 20, 2021 13:19

Apply code review suggestions

6818c6f

Apply clang-tidy-11 suggestions

49ab883

Merge branch 'master' into faster_linalgeb_support

7c99078

mlxd reviewed Oct 25, 2021

View reviewed changes

maliasadi requested a review from chaeyeunpark November 12, 2021 17:20

maliasadi and others added 5 commits November 15, 2021 01:57

Update OMP innerProd

5f90788

Fix formatting check / black

7d45c69

Update

39cada4

Merge branch 'master' into faster_linalgeb_support

16194ba

Merge branch 'master' into faster_linalgeb_support

8b9bc71

mlxd approved these changes Nov 15, 2021

View reviewed changes

maliasadi merged commit 63929ed into master Nov 15, 2021

maliasadi deleted the faster_linalgeb_support branch November 15, 2021 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Multi-threaded DOTC, GEMV, GEMM along with BLAS Support #155

Add Multi-threaded DOTC, GEMV, GEMM along with BLAS Support #155

maliasadi commented Oct 13, 2021 •

edited

Loading

codecov bot commented Oct 13, 2021 •

edited

Loading

github-actions bot commented Oct 13, 2021 •

edited

Loading

mlxd left a comment

mlxd Oct 18, 2021

maliasadi Oct 18, 2021

mlxd Oct 19, 2021

maliasadi Oct 20, 2021

mlxd Oct 20, 2021 •

edited

Loading

maliasadi Oct 20, 2021

mlxd Oct 20, 2021 •

edited

Loading

mlxd Oct 20, 2021

mlxd Oct 20, 2021

mlxd Oct 20, 2021

mlxd left a comment

mlxd Oct 25, 2021

mlxd Oct 25, 2021

		#define DOTU_STD_CROSSOVER (1 << 10)
		#define DOTC_STD_CROSSOVER (1 << 10)

Add Multi-threaded DOTC, GEMV, GEMM along with BLAS Support #155

Add Multi-threaded DOTC, GEMV, GEMM along with BLAS Support #155

Conversation

maliasadi commented Oct 13, 2021 • edited Loading

codecov bot commented Oct 13, 2021 • edited Loading

Codecov Report

github-actions bot commented Oct 13, 2021 • edited Loading

Test Report (C++) on Ubuntu

mlxd left a 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

mlxd Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlxd Oct 20, 2021 • edited Loading

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

mlxd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maliasadi commented Oct 13, 2021 •

edited

Loading

codecov bot commented Oct 13, 2021 •

edited

Loading

github-actions bot commented Oct 13, 2021 •

edited

Loading

mlxd Oct 20, 2021 •

edited

Loading

mlxd Oct 20, 2021 •

edited

Loading