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

NSIMD: Pass architecture choice to compiler #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

eschnett
Copy link

Define a preprocessor variable with the choice of SIMD architecture. If not set, NSIMD chooses a default.

# Pass compiler options to Cactus
echo "BEGIN DEFINE"
echo "NSIMD_${NSIMD_SIMD}"
echo "END DEFINE"
Copy link
Member

Choose a reason for hiding this comment

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

This does not capture everything NSIMD_OPTIONS did in cactusamrex/NSIMD. E.g. it cannot capture the (additional one assumes, even though defines.md says "one of") flag NSIMD_FMA

Looking at the library it seems that NSIMD_OPTIONALS is not actually used by anything (nor defined in configure.ccl). So they might just as well be renamed to NSIMD_OPTIONS and used as in cactusamrex/NSIMD. Looking at defines.md they should be present both in Cactus (via the DEFINE) and when compiling the library itself, yes? An alternative (and more flexible since one can pass values) would be to have a NSIMD_EXTRA_CXXFLAGS that can set these flags. 

In my test it seems that cmake ../ -Dsimd=foobar is accepted (literally) and foobar is just what gets tagged on the the file name but is not used to actually set any SIMD extension :-(

Copy link
Author

Choose a reason for hiding this comment

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

This might be an outdated feature from a previous major release of NSIMD. Currently the only important option is the architecture which is encoded in the library file name (e.g. sse2, avx512) used for linking.

Copy link
Member

Choose a reason for hiding this comment

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

Grr. GitHub ate my comment. Here we go again.

Alright, so I did some testing to see what the build system and compiler actually do.

I tested by removing or adding the the DEFINE and CXXFLAGS= bits of the commit.

I found that:

  1. for building libNSIMD_AVX2.so it is sufficient to have -Dsimd_AVX2 set via CMake to get vmovd instructions show up in the disassembled output (and movsd if one uses NSIMD_SIMD=CPU) so that fiddling with CXXFLAGS to build the library and have it use AVX2 is not required (have not tested FMA).
  2. for buidling client code (ie Cactus code using nsimd_all.h) I do seem to need the DEFINE as tested using the NSIMD tutorial.cpp which switches instructions used for the mask_storeu used based on whether NSIMD_AVX2 is defined or not (vmovdqa vs movslq).

The NSIMD_FMA and NSIMD_AVX2 defines are checked for in nsimd.h but not defined there based on any build time (of libNSIMD_AVX2.so) options as far as I can tell.

Test code is in a branch rhaas/nsimd-test and the disassembled code in the attached txt files (GitHub limits the allowed extensions).

test.cc-noAVX2.S.txt
test.cc-AVX2.S.txt

Copy link
Author

Choose a reason for hiding this comment

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

Interesting...

Copy link
Member

Choose a reason for hiding this comment

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

Might need we look into vectorization in Z4c benchmarks once more :-)

Define a preprocessor variable with the choice of SIMD architecture.
If not set, NSIMD chooses a default.
@rhaas80 rhaas80 force-pushed the eschnett/nsimd-arch branch from 0578f32 to 4598e8d Compare December 5, 2023 18:51
@rhaas80 rhaas80 force-pushed the eschnett/nsimd-arch branch from 4598e8d to 24a786e Compare December 5, 2023 19:10
@rhaas80
Copy link
Member

rhaas80 commented Dec 5, 2023

I added code for the OPTIONALS (kept the variable name after all):

# actually have NSIMD compile code for the chosen architecture
CXXFLAGS="$CXXFLAGS -DNSIMD_${NSIMD_SIMD}"
for opt in ${NSIMD_OPTIONALS}; do
    CXXFLAGS="$CXXFLAGS -D$opt"
done

to build and DEFINE sections. This at least passes the defines to g++ when buliding NSIMD itself (and I see changes in the produced library file).

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.

2 participants