Project

General

Profile

Bug #2588

anyTrue(SimdDIBool) implementation for AVX_256, double precision

Added by Damian Grillo about 2 years ago. Updated almost 2 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
mdrun
Target version:
Affected version - extra info:
Affected version:
Difficulty:
uncategorized
Close

Description

Dear developers:

I have been working on a patched version of GROMACS 2018 to include
the calculation of the local pressure tensor in spherical coordinates.
I have implemented it to support plain C, SIMD 4xN and 2xNN kernels.
Among other things, my method requires to determine if a SimdDIBool
variable has any true component, using the anyTrue(SimdDIBool) function
already implemented in the different SIMD implementations within the code.

I have found an unexpected behaviour of this function (many false negatives)
for AVX_256 and AVX2_256 in double precision, which is implemented in

src/gromacs/simd/impl_x86_avx_256/impl_x86_avx_256_simd_double.h

The original version is:

static inline bool gmx_simdcall
anyTrue(SimdDIBool a)
{
    return _mm_movemask_epi8(_mm_shuffle_epi32(a.simdInternal_, _MM_SHUFFLE(1, 0, 1, 0))) != 0;
}

For AVX_256 and AVX2_256, we require to know the four components stored in a.simdInternal_.
However, the components 2,3 are lost after the shuffle operation. Then, many false negatives
are thrown due to the missing data.

My new implementation for this function is:

static inline bool gmx_simdcall
anyTrue(SimdDIBool a)
{
    return _mm_movemask_epi8(a.simdInternal_) != 0;
}

I have been testing it and seems to be working properly.

I hope it helps you.

Regards.

Damian.

Associated revisions

Revision c8e885b4 (diff)
Added by Erik Lindahl almost 2 years ago

Fix SIMD anyTrue bug, and add better unit test

The double precision versions could occasionally
miss elements.

Fixes #2588.

Change-Id: Ie033a3af0e5de25205533868062716a83f33df3c

History

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

  • Description updated (diff)

#2 Updated by Roland Schulz about 2 years ago

Thanks for the report. Could you upload it as a patch to gerrit? Ideally could you also add a unit test which fails for the old function?

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

  • Assignee changed from Damian Grillo to Erik Lindahl

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

Gerrit received a related patchset '1' for Issue #2588.
Uploader: Erik Lindahl ()
Change-Id: gromacs~master~Ie033a3af0e5de25205533868062716a83f33df3c
Gerrit URL: https://gerrit.gromacs.org/8171

#5 Updated by Erik Lindahl almost 2 years ago

  • Status changed from New to Fix uploaded

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

Gerrit received a related patchset '1' for Issue #2588.
Uploader: Erik Lindahl ()
Change-Id: gromacs~release-2018~Ie033a3af0e5de25205533868062716a83f33df3c
Gerrit URL: https://gerrit.gromacs.org/8183

#7 Updated by Erik Lindahl almost 2 years ago

  • Status changed from Fix uploaded to Resolved

#8 Updated by Paul Bauer almost 2 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF