Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update CPU Architectures, Update YATeTo #962

Merged
merged 10 commits into from Sep 27, 2023

Conversation

davschneller
Copy link
Contributor

@davschneller davschneller commented Sep 22, 2023

  • Adds several new CPU architectures, as shown in YATeTo right now (but not yet apple-m2). Also adds dummy architectures for SVE and NEON.
  • Splits between ALIGNMENT and VECTORSIZE. The latter, VECTORSIZE should equal the YaTeTo value; it's from now on used in calculating any aligned reals or basis functions. The former value, ALIGNMENT may be larger (e.g. as large as a cache line), but mostly, it's still the same value.
  • Updates YATeTo. That also includes Remove some deepcop[ies] to speed up strengthReduction yateto#66 , and by that a significant speed improvement for the kernel generation.
  • Reworks the GEMM_TOOLS_LIST=auto option to include LIBXSMM_JIT before LIBXSMM, and the new architectures. Also, all of these programs need to be found by CMake before they are considered for auto. Eigen is now always added at the end in this case.
  • Redzone is now disabled by default (especially for noarch), and enabled for most X86_64 CPUs again.

@davschneller
Copy link
Contributor Author

Maybe also related to #960 . (i.e. the split between ALIGNMENT and VECTORSIZE)

@davschneller davschneller changed the title Update CPU Architectures, update YATeto Update CPU Architectures, update YATeTo Sep 22, 2023
@davschneller davschneller changed the title Update CPU Architectures, update YATeTo Update CPU Architectures, Update YATeTo Sep 22, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2023

Codecov Report

Merging #962 (c83b8d9) into master (1579dfa) will not change coverage.
Report is 2 commits behind head on master.
The diff coverage is 0.00%.

❗ Current head c83b8d9 differs from pull request most recent head 6a1eb0e. Consider uploading reports for the commit 6a1eb0e to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@           Coverage Diff           @@
##           master     #962   +/-   ##
=======================================
  Coverage   14.39%   14.39%           
=======================================
  Files         253      253           
  Lines       14223    14223           
=======================================
  Hits         2047     2047           
  Misses      12176    12176           
Files Coverage Δ
src/Kernels/common.hpp 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -126,7 +126,7 @@ constexpr unsigned int
* @return aligned number of reals.
**/
constexpr unsigned int getNumberOfAlignedReals(unsigned int i_numberOfReals,
unsigned int i_alignment = ALIGNMENT) {
unsigned int i_alignment = VECTORSIZE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we then also change the variable name to i_vectorsize?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO no, because it is an alignment (to vectorsize)

Copy link
Contributor

Choose a reason for hiding this comment

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

(same below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've left it that way for now—since even though it's VECTORSIZE, we're aligning everything. Instead, we may have to rename the ALIGNMENT variable soon, I'd guess. And also, make both of them a C++ constexpr maybe.

I've also cleaned up the file a bit (i.e. remove i_ etc.) and templated the functions with the size of the reals used. ... That could also become a parameter instead.

@@ -145,7 +145,7 @@ constexpr unsigned int getNumberOfAlignedReals(unsigned int i_numberOfReals,
**/
constexpr unsigned int
getNumberOfAlignedBasisFunctions(unsigned int i_convergenceOrder = CONVERGENCE_ORDER,
unsigned int i_alignment = ALIGNMENT) {
unsigned int i_alignment = VECTORSIZE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Change variable name?

@@ -161,7 +161,7 @@ constexpr unsigned int
**/
constexpr unsigned
getNumberOfAlignedDerivativeBasisFunctions(unsigned int i_convergenceOrder = CONVERGENCE_ORDER,
unsigned int i_alignment = ALIGNMENT) {
unsigned int i_alignment = VECTORSIZE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Change variable name?

@sebwolf-de sebwolf-de added this pull request to the merge queue Sep 27, 2023
Merged via the queue into master with commit 7901b7c Sep 27, 2023
5 checks passed
@krenzland krenzland deleted the davschneller/update-cpu-archs branch September 27, 2023 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants