Project

General

Profile

Bug #1809

FFTW AVX unnecessarily checks for 128-bit AVX support

Added by Szilárd Páll about 2 years ago. Updated almost 2 years ago.

Status:
Closed
Priority:
Low
Assignee:
Category:
build system
Target version:
Affected version - extra info:
5.1
Affected version:
Difficulty:
uncategorized
Close

Description

When the FFTW detection was changed to suggest the use of SSE2+AVX (d33720e6), the SIMD AVX feature check was changed to *_have_simd_avx_128 from *_have_simd_avx which is incorrect.

$ nm /opt/tcbsys/fftw/3.3.4-sse2-avx
/lib/libfftw3f.so | grep _have_simd00000000000f8a90 T fftwf_have_simd_avx
00000000000f8a80 T fftwf_have_simd_sse2

$ cmake 
[...]
-- Looking for fftwf_plan_r2r_1d in /opt/tcbsys/fftw/3.3.4-sse2-avx/lib/libfftw3f.so
-- Looking for fftwf_plan_r2r_1d in /opt/tcbsys/fftw/3.3.4-sse2-avx/lib/libfftw3f.so - found
-- Looking for fftwf_have_simd_sse in /opt/tcbsys/fftw/3.3.4-sse2-avx/lib/libfftw3f.so
-- Looking for fftwf_have_simd_sse in /opt/tcbsys/fftw/3.3.4-sse2-avx/lib/libfftw3f.so - not found
-- Looking for fftwf_have_simd_sse2 in /opt/tcbsys/fftw/3.3.4-sse2-avx/lib/libfftw3f.so
-- Looking for fftwf_have_simd_sse2 in /opt/tcbsys/fftw/3.3.4-sse2-avx/lib/libfftw3f.so - found
-- Looking for fftwf_have_simd_avx_128 in /opt/tcbsys/fftw/3.3.4-sse2-avx/lib/libfftw3f.so
-- Looking for fftwf_have_simd_avx_128 in /opt/tcbsys/fftw/3.3.4-sse2-avx/lib/libfftw3f.so - not found
-- Looking for fftwf_have_simd_avx2_128 in /opt/tcbsys/fftw/3.3.4-sse2-avx/lib/libfftw3f.so
-- Looking for fftwf_have_simd_avx2_128 in /opt/tcbsys/fftw/3.3.4-sse2-avx/lib/libfftw3f.so - not found
-- Looking for fftwf_have_simd_sse2 in /opt/tcbsys/fftw/3.3.4-sse2-avx/lib/libfftw3f.so
-- Looking for fftwf_have_simd_sse2 in /opt/tcbsys/fftw/3.3.4-sse2-avx/lib/libfftw3f.so - found
-- Using external FFT library - FFTW3

The detection is looking for fftwf_have_simd_avx_128 which is not present i nFFTW 3.3.4 nor in the current git which will be FFTW 3.3.5, see below.

I assume that we should be looking for fftwf_have_simd_avx and using whatever AVX support there is in all 3.3.x, regardless of whether that's 128-bit or 256-bit (and AFAIK 128-bit has never existed and will not be added to future releases either). Hence, we should not be warning the user about an nonexistent feature.

Associated revisions

Revision 1909f2ff (diff)
Added by Mark Abraham almost 2 years ago

Fix FindFFTW behaviour

FFTW 3.3.5 with --enable-avx* will enable any useful 128-bit
SIMD flavours by default. (See Erik's 579cec9a6 in their repo.) Our
detection code will observe this, and will be silent.

With earlier FFTW, if we're doing a GROMACS AVX build, and we have
FFTW SIMD, and not SSE SIMD, then we want to warn the user to
reconfigure FFTW to add SSE support. Made this behaviour correct, and
minimized the necessary infrastructure for it.

Added detection support for some other SIMD-support symbols that
are present in the FFTW repo for upcoming hardware.

Fixes #1809

Change-Id: If586250895664581316505a5595da7442e789f8d

History

#1 Updated by Gerrit Code Review Bot about 2 years ago

Gerrit received a related patchset '1' for Issue #1809.
Uploader: Szilárd Páll ()
Change-Id: I454aa7165cfcab3cacc42e1980691ea88cbdf6ab
Gerrit URL: https://gerrit.gromacs.org/5014

#2 Updated by Berk Hess about 2 years ago

Is there an actual issue here?
The logic of the check has changed from negative (we should not use AVX) to positive (we should have 128-bit SIMD support). The *_have_simd_avx_128 have been added to FFTW by Erik and should be in the next FFTW release.

#3 Updated by Szilárd Páll about 2 years ago

Berk Hess wrote:

Is there an actual issue here?
The logic of the check has changed from negative (we should not use AVX) to positive (we should have 128-bit SIMD support). The *_have_simd_avx_128 have been added to FFTW by Erik and should be in the next FFTW release.

There seems to be no have_simd_avx_128, not even in Erik's version:

$ nm programs/fftw/3.4.5-sse-avx-avx2/lib/libfftw3f.so | grep _have_simd
00000000001145a0 T fftwf_have_simd_avx
0000000000114610 T fftwf_have_simd_avx_128_fma
0000000000114700 T fftwf_have_simd_avx2
0000000000114690 T fftwf_have_simd_avx2_128
0000000000114590 T fftwf_have_simd_sse2

So based on this it seems that "simd_avx" = AVX 128-bit? Not sure, but neither current git nor 3.3.4 has simd_avx_128.

#4 Updated by Berk Hess about 2 years ago

Is that Erik's latest version? He changed this after we discussed the Gromacs FFTW SIMD detection.

#5 Updated by Erik Lindahl about 2 years ago

Hi,

I remember having a conversation with Steven that they wondered how useful it really was to have 128-bit AVX as a separate option, since apparently their typical policy is to only do that if they gain >10%.

IIRC, instead I merged vanilla 128-bit AVX into the main AVX port, but I still managed to convince them we should keep a separate AVX-128-FMA version for AMD. I just double-checked the repo that my memory served me right, too.

So, the drawback for Gromacs short-term is that we might not have a bullet-proof way to identify 128-bit AVX, unless we simply go with the version number being >3.3.4.

#6 Updated by Szilárd Páll about 2 years ago

Berk Hess wrote:

Is that Erik's latest version? He changed this after we discussed the Gromacs FFTW SIMD detection.

This is Erik's latest version.

So does -enable-avx generate 128-bit or 256-bit AVX kernels?

#7 Updated by Szilárd Páll about 2 years ago

  • Description updated (diff)

#8 Updated by Mark Abraham about 2 years ago

Regarding the description of the issue: these lines aren't warnings. These are statements about the results of system checks run in check_library_exists(). CMake (and autoconf) projects do hundreds of these checks, because that's their job. Warnings are things we bring to the user's attention, like "Hey, FFTW isn't right, you should set up FFTW like <this>". Hence my question on gerrit, because I now understand there's no (what I call a) warning that you have seen.

The check for whether FFTW has any SIMD support is further down FindFFTW.cmake, and does check for the standard fftwf_have_simd_avx if nothing else has already been found. This is part of the logic we use in deciding whether to warn the user about what we've learned about their FFTW.

We should be checking for whatever feature-reporting functions FFTW 3.3.5 might actually have. Erik seems to be saying that there will never be a fftwf_have_simd_avx_128. So, instead we should test for *_have_simd_avx_128_fma, add it to the set of things that might set ${FFTW}_HAVE_SIMD (likewise add *_have_simd_avx2_128), and update the logic in cmake/gmxManageFFTLibraries.cmake so the SIMD-and-FFTW warnings we issue to GROMACS users are triggered accurately.

#9 Updated by Mark Abraham about 2 years ago

If there will be *_have_simd_avx2 then that should get added also. But we are distinguishing "SIMD" support from "128-bit SIMD support" in at least one place, so take care.

#10 Updated by Szilárd Páll about 2 years ago

You're right, I did not look through the code carefully enough. I thought that the check I modified did have an effect on what will be used at runtime, but I guess it does not as that's completely internal to FFTW, right? I did not assume that the main warning was issued based on it, though, but admittedly I confused status messages of the check_library_exists commands with warnings.

However, given the original assumption that there should/will be an fftwf_have_simd_avx_128, why doesn't the check at source:cmake/FindFFTW.cmake#L152 also test for have_simd_avx_128 rather than have_simd_avx?

It's still not entirely clear to me the assumptions the code in question makes. I particular, I don't understand why are there checks for 128-bit AVX/AVX2 whereas there is a single configure option to tell FFTW to build AVX/AVX2 (presumably both 128-bit and 256-bit in the latter case). Or are the vendor FFTW3 flavors that can be configured with either 128-bit and 256-bit AVX kernels?

So finally, should fftwf_have_simd_avx_128 related stuff be removed? Does Cray or any other vendor implement it?

#11 Updated by Berk Hess about 2 years ago

If I understood Erik's comment correctly, his avx_128 acceleration was not accepted by FFTW. In that case there is only avx_256. But Erik said he also always turned on sse2 when avx was enabled. That would remove the whole need for checking. Erik, did that change make it into FFTW?

So the avx_128 check is useless, but harmless. We should remove it, at least from master.

#12 Updated by Szilárd Páll about 2 years ago

  • Subject changed from FFTW AVX detection broken to FFTW AVX unnecessarily checks for 128-bit AVX support

#13 Updated by Szilárd Páll about 2 years ago

Berk Hess wrote:

If I understood Erik's comment correctly, his avx_128 acceleration was not accepted by FFTW. In that case there is only avx_256. But Erik said he also always turned on sse2 when avx was enabled. That would remove the whole need for checking. Erik, did that change make it into FFTW?

Some clarification of the assumptions (when is there SSE, AVX 128/256-bit assumed or expected to be present) would still be useful too because the code did not seem very self-explanatory.

So the avx_128 check is useless, but harmless. We should remove it, at least from master.

Mostly harmless, but can be confusing. The output indicates that AVX support was not found which not many read apparently (but Rossen did, he noticed the discrepancy).

#14 Updated by Szilárd Páll almost 2 years ago

  • Status changed from New to Blocked, need info

This is still pending, some advice on how to resolve the issue would be welcome.

#15 Updated by Rossen Apostolov almost 2 years ago

So finally, should fftwf_have_simd_avx_128 related stuff be removed? Does Cray or any other vendor implement it?

Cray's latest 3.3.4.3 comes with _sse2, _avx and _avx2. There's no *_128.

#16 Updated by Gerrit Code Review Bot almost 2 years ago

Gerrit received a related patchset '1' for Issue #1809.
Uploader: Mark Abraham ()
Change-Id: If586250895664581316505a5595da7442e789f8d
Gerrit URL: https://gerrit.gromacs.org/5110

#17 Updated by Mark Abraham almost 2 years ago

  • Status changed from Blocked, need info to Fix uploaded
  • Assignee set to Mark Abraham

I looked at the FFTW repo, and the whole issue will just go away in FFTW 3.3.5 if we do https://gerrit.gromacs.org/5110

#18 Updated by Mark Abraham almost 2 years ago

  • Status changed from Fix uploaded to Resolved

#19 Updated by Mark Abraham almost 2 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF