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

Calculation goes off the rails with AVX-512 kernels #2029

Closed
jabl opened this issue Feb 25, 2019 · 21 comments
Closed

Calculation goes off the rails with AVX-512 kernels #2029

jabl opened this issue Feb 25, 2019 · 21 comments

Comments

@jabl
Copy link

jabl commented Feb 25, 2019

Hi,

I don't know if this issue belongs to https://github.com/cp2k/cp2k or here, but anyway, here goes:

I have built cp2k 6.1 using OpenBLAS 0.3.5 built with DYNAMIC_ARCH=1 (via spack), running on an avx-512 equipped skylake-x machine. For my testcase I'm using the "test A" from the PRACE benchmarking suite ( http://www.prace-ri.eu/UEABS/CP2K/CP2K_TestCaseA.tar.gz ). Unfortunately the result is that the calculation diverges almost immediately, the end of the output says


SCF WAVEFUNCTION OPTIMIZATION

  ----------------------------------- OT ---------------------------------------
  Minimizer      : DIIS                : direct inversion
                                         in the iterative subspace
                                         using   7 DIIS vectors
                                         safer DIIS on
  Preconditioner : FULL_KINETIC        : inversion of T + eS
  Precond_solver : DEFAULT
  stepsize       :    0.15000000                  energy_gap     :    0.20000000
  eps_taylor     :   0.10000E-15                  max_taylor     :             4
  ----------------------------------- OT ---------------------------------------

  Step     Update method      Time    Convergence         Total energy    Change
  ------------------------------------------------------------------------------
     1 OT DIIS     0.15E+00  167.7     0.00331514    -16961.8835192894 -1.70E+04
     2 OT SD       0.15E+00   49.1     7.4819E+14 ********************  7.72E+15
     3 OT SD       0.15E+00   47.9     1.2647E+97 ********************  2.07E+80

 *******************************************************************************
 *   ___                                                                       *
 *  /   \                                                                      *
 * [ABORT]                                                                     *
 *  \___/                KS energy is an abnormal value (NaN/Inf).             *
 *    |                                                                        *
 *  O/|                                                                        *
 * /| |                                                                        *
 * / \                                                     qs_ks_methods.F:866 *
 *******************************************************************************


 ===== Routine Calling Stack ===== 

            7 qs_ks_build_kohn_sham_matrix
            6 rebuild_ks_matrix
            5 qs_ks_update_qs_env
            4 scf_env_do_scf_inner_loop
            3 scf_env_do_scf
            2 qs_energies
            1 CP2K

Running on a non-AVX-512 machine all goes well, or by using OpenBLAS 0.2.20 from Ubuntu 18.04 package repos (without AVX-512 kernels).

@brada4
Copy link
Contributor

brada4 commented Feb 25, 2019

Does it fail with develop version? It adds some fixes that may or may not completely address your issue (some in avx-512 side, others in gcc9 side)

@martin-frbg
Copy link
Collaborator

This seems to match the earlier report in #1955 (for which unfortunately no small self-contained reproducer has been found yet). I doubt CP2k fits the "small and self-contained" category either, but at least it appears to be fortran...

@martin-frbg
Copy link
Collaborator

martin-frbg commented Feb 25, 2019

BTW as a (hopefully) short-term workaround you should be able to force the DYNAMIC_ARCH library to use the non-AVX512 Haswell kernels at runtime by export OPENBLAS_CORETYPE=Haswell rather than resorting to 0.2.20 (18+ months old and with a sizable number of known bugs including thread races).

@martin-frbg
Copy link
Collaborator

@brada4 the only AVX512-related PR since release 0.3.5 affected autodetection of cpu capabilities only. The change implicated in #1955 happened after 0.3.3, but I do not think it advisable to prefer that version over a current 0.3.5 clamped to use Haswell kernels.

@jabl
Copy link
Author

jabl commented Feb 25, 2019

Argh. I was just about to report success with the latest OpenBLAS git, but upon closer inspection it turned out that some change since 0.3.5 has caused it to add the NO_AVX512 preprocessor symbol, and thus no AVX512 kernels were actually built. Any idea how to get rid of that (might be because my build host does not have AVX-512, but with DYNAMIC_ARCH that shouldn't matter, right?)?

@martin-frbg
Copy link
Collaborator

Maybe I introduced a logic flaw, but at least in theory the recent changes should affect only whether the host cpu is identified as SkylakeX (both cpu and os capable of AVX512) or Haswell. NO_AVX512 should only get set when your compiler actually fails to process a simple test case embedded in the c_check file (or fails to accept the -march=skylake-avx512 option).

@martin-frbg
Copy link
Collaborator

Meh. That check must have been broken for quite some time - somehow I managed to drop the -c from
the compile line so the avx512 check always failed with "undefined reference to main" ??

@martin-frbg
Copy link
Collaborator

martin-frbg commented Feb 25, 2019

Fixed on the develop branch now (added -c to compiler arguments on line 235 of c_check). With NO_AVX512 set, SKYLAKEX would not be on the default list of DYNAMIC_ARCH targets - I have yet to understand if/how builds on actual SkylakeX hardware would get around the bogus failure. ☹️

@jabl
Copy link
Author

jabl commented Feb 25, 2019

Thanks, that fix allowed me to produce a build of the develop branch with the AVX-512 kernels included. All our skylake-x nodes are fully booked at the moment, lets see by tomorrow if the develop version fares better with cp2k than 0.3.5, though based on your previous comment maybe I shouldn't be getting my hopes up.

@martin-frbg
Copy link
Collaborator

Indeed I do not expect develop to be better in this regard - but building from source will allow you to apply the "fix" from #1955 to disable the avx512 DGEMM kernel. (Though given that AVX512 support in OpenBLAS is still pretty much limited to SGEMM and DGEMM that would not gain you much over a plain Haswell build). At least by now it is clear that the AVX512 issue is real and not just some coincidental bug or calling convention mismatch in Julia @fenrus75

@jabl
Copy link
Author

jabl commented Feb 25, 2019

Oh, I got lucky and my job was already scheduled. Unfortunately, I now again got the same failure as I originally reported in this issue.

As mentioned, cp2k isn't really small nor self-contained, and furthermore the testcase I've been using requires about 100 GB RAM.

@brada4
Copy link
Contributor

brada4 commented Feb 25, 2019

it will help setting OPENBLAS_CORETYPE=HASWELL to "fix" DYNAMIC_ARCH build (or build with TARGET=HASWELL if on your own)

@fenrus75
Copy link
Contributor

still clamoring for ANY way that can repro this ...

@jabl
Copy link
Author

jabl commented Feb 25, 2019

Well, for ANY way, with the disclaimer that cp2k is not anywhere near simple, and that the testcase requires about 100 GB RAM, see https://github.com/AaltoScienceIT/docker-fgci-benchmark

If following the instructions in the README, use the tag "v2.2" instead of "latest" to get the version using the OpenBLAS 0.3.6dev version.

@martin-frbg
Copy link
Collaborator

I wonder if we might be able to get something similar with the more compact ELK code (broadly similar kind of computations, from elk.sourceforge.net). Still haven't got AVX512 hardware though

@brada4
Copy link
Contributor

brada4 commented Feb 25, 2019

Probably it is possible to subset inputs to make it viable on common hardware...

@fenrus75
Copy link
Contributor

fenrus75 commented Feb 25, 2019 via email

@martin-frbg
Copy link
Collaborator

Experimenting with Intel SDE in lieu of actual hardware, I now find that BLAS-Tester is actually unhappy about DSYMM:

./xdl3blastst -R symm 

-------------------------------- SYMM --------------------------------
TST# S U    M    N ALPHA  LDA  LDB  BETA  LDC   TIME MFLOP  SpUp  TEST
==== = = ==== ==== ===== ==== ==== ===== ==== ====== ===== ===== =====
ERROR:  resid=22184890850.394222, normD=114.531676, normA=27.442124, normB=29.226439, normC=28.989082, eps=2.220446e-15
   resid=22184890850.394222
   0 L L  100  100   1.0 1000 1000   1.0 1000   0.01 201.2  1.00 -----
   0 L L  100  100   1.0 1000 1000   1.0 1000   0.09  21.7  0.11 FAIL 

(etc.)

@fenrus75
Copy link
Contributor

fenrus75 commented Mar 6, 2019 via email

@martin-frbg
Copy link
Collaborator

..provided this can be reproduced on the actual hardware at all (?)

markuskowa added a commit to markuskowa/nixpkgs that referenced this issue Apr 23, 2019
The AVX512 optimization has a bug in openblas-0.3.5
and can also lead to slower code on Xeon Silver CPUs.

See OpenMathLib/OpenBLAS#1955
and OpenMathLib/OpenBLAS#2029
FRidh pushed a commit to NixOS/nixpkgs that referenced this issue Apr 27, 2019
The AVX512 optimization has a bug in openblas-0.3.5
and can also lead to slower code on Xeon Silver CPUs.

See OpenMathLib/OpenBLAS#1955
and OpenMathLib/OpenBLAS#2029
markuskowa added a commit to NixOS/nixpkgs that referenced this issue Apr 29, 2019
The AVX512 optimization has a bug in openblas-0.3.5
and can also lead to slower code on Xeon Silver CPUs.

See OpenMathLib/OpenBLAS#1955
and OpenMathLib/OpenBLAS#2029

(cherry picked from commit 7035aa7)
TiborGY added a commit to TiborGY/OpenBLAS that referenced this issue Jul 7, 2019
* With the Intel compiler on Linux, prefer ifort for the final link step 

icc has known problems with mixed-language builds that ifort can handle just fine. Fixes OpenMathLib#1956

* Rename operands to put lda on the input/output constraint list

* Fix wrong constraints in inline assembly

for OpenMathLib#2009

* Fix inline assembly constraints

rework indices to allow marking argument lda4 as input and output. For OpenMathLib#2009

* Fix inline assembly constraints

rework indices to allow marking argument lda as input and output.

* Fix inline assembly constraints

* Fix inline assembly constraints

* Fix inline assembly constraints in Bulldozer TRSM kernels

rework indices to allow marking i,as and bs as both input and output (marked operand n1 as well for simplicity). For OpenMathLib#2009

* Correct range_n limiting

same bug as seen in OpenMathLib#1388, somehow missed in corresponding PR OpenMathLib#1389

* Allow multithreading TRMV again

revert workaround introduced for issue OpenMathLib#1332 as the actual cause appears to be my incorrect fix from OpenMathLib#1262 (see OpenMathLib#1388)

* Fix error introduced during cleanup

* Reduce list of kernels in the dynamic arch build

to make compilation complete reliably within the 1h limit again

* init

* move fix to right place

* Fix missing -c option in AVX512 test

* Fix AVX512 test always returning false due to missing compiler option

* Make x86_32 imply NO_AVX2, NO_AVX512 in addition to NO_AVX

fixes OpenMathLib#2033

* Keep xcode8.3 for osx BINARY=32 build

as xcode10 deprecated i386

* Make sure that AVX512 is disabled in 32bit builds

for OpenMathLib#2033

* Improve handling of NO_STATIC and NO_SHARED

to avoid surprises from defining either as zero. Fixes OpenMathLib#2035 by addressing some concerns from OpenMathLib#1422

* init

* address warning introed with OpenMathLib#1814 et al

* Restore locking optimizations for OpenMP case

restore another accidentally dropped part of OpenMathLib#1468 that was missed in OpenMathLib#2004 to address performance regression reported in OpenMathLib#1461

* HiSilicon tsv110 CPUs optimization branch

add HiSilicon tsv110 CPUs  optimization branch

* add TARGET support for  HiSilicon tsv110 CPUs

* add TARGET support for HiSilicon tsv110 CPUs

* add TARGET support for HiSilicon tsv110 CPUs

* Fix module definition conflicts between LAPACK and ReLAPACK

for OpenMathLib#2043

* Do not compile in AVX512 check if AVX support is disabled

xgetbv is function depends on NO_AVX being undefined - we could change that too, but that combo is unlikely to work anyway

* ctest.c : add __POWERPC__ for PowerMac

* Fix crash in sgemm SSE/nano kernel on x86_64

Fix bug OpenMathLib#2047.

Signed-off-by: Celelibi <celelibi@gmail.com>

* param.h : enable defines for PPC970 on DarwinOS

fixes:
gemm.c: In function 'sgemm_':
../common_param.h:981:18: error: 'SGEMM_DEFAULT_P' undeclared (first use in this function)
 #define SGEMM_P  SGEMM_DEFAULT_P
                  ^

* common_power.h: force DCBT_ARG 0 on PPC970 Darwin

without this, we see
../kernel/power/gemv_n.S:427:Parameter syntax error
and many more similar entries

that relates to this assembly command
dcbt 8, r24, r18

this change makes the DCBT_ARG = 0
and openblas builds through to completion on PowerMac 970
Tests pass

* Make TARGET=GENERIC compatible with DYNAMIC_ARCH=1

for issue OpenMathLib#2048

* make DYNAMIC_ARCH=1 package work on TSV110.

* make DYNAMIC_ARCH=1 package work on TSV110

* Add Intel Denverton

for OpenMathLib#2048

* Add Intel Denverton

* Change 64-bit detection as explained in OpenMathLib#2056

* Trivial typo fix

as suggested in OpenMathLib#2022

* Disable the AVX512 DGEMM kernel (again)

Due to as yet unresolved errors seen in OpenMathLib#1955 and OpenMathLib#2029

* Use POSIX getenv on Cygwin

The Windows-native GetEnvironmentVariable cannot be relied on, as
Cygwin does not always copy environment variables set through Cygwin
to the Windows environment block, particularly after fork().

* Fix for OpenMathLib#2063: The DllMain used in Cygwin did not run the thread memory
pool cleanup upon THREAD_DETACH which is needed when compiled with
USE_TLS=1.

* Also call CloseHandle on each thread, as well as on the event so as to not leak thread handles.

* AIX asm syntax changes needed for shared object creation

* power9 makefile. dgemm based on power8 kernel with following changes : 32x unrolled 16x4 kernel and 8x4 kernel using (lxv stxv butterfly rank1 update). improvement from 17 to 22-23gflops. dtrmm cases were added into dgemm itself

* Expose CBLAS interfaces for I?MIN and I?MAX

* Build CBLAS interfaces for I?MIN and I?MAX

* Add declarations for ?sum and cblas_?sum

* Add interface for ?sum (derived from ?asum)

* Add ?sum

* Add implementations of ssum/dsum and csum/zsum

as trivial copies of asum/zsasum with the fabs calls replaced by fmov to preserve code structure

* Add ARM implementations of ?sum

(trivial copies of the respective ?asum with the fabs calls removed)

* Add ARM64 implementations of ?sum

as trivial copies of the respective ?asum kernels with the fabs calls removed

* Add ia64 implementation of ?sum

as trivial copy of asum with the fabs calls removed

* Add MIPS implementation of ?sum

as trivial copy of ?asum with the fabs calls removed

* Add MIPS64 implementation of ?sum

as trivial copy of ?asum with the fabs replaced by mov to preserve code structure

* Add POWER implementation of ?sum

as trivial copy of ?asum with the fabs replaced by fmr to preserve code structure

* Add SPARC implementation of ?sum

as trivial copy of ?asum with the fabs replaced by fmov to preserve code structure

* Add x86 implementation of ?sum

as trivial copy of ?asum with the fabs calls removed

* Add x86_64 implementation of ?sum

as trivial copy of ?asum with the fabs calls removed

* Add ZARCH implementation of ?sum

as trivial copies of the respective ?asum kernels with the ABS and vflpsb calls removed

* Detect 32bit environment on 64bit ARM hardware

for OpenMathLib#2056, using same approach as OpenMathLib#2058

* Add cmake defaults for ?sum kernels

* Add ?sum

* Add ?sum definitions for generic kernel

* Add declarations for ?sum

* Add -lm and disable EXPRECISION support on *BSD

fixes OpenMathLib#2075

* Add in runtime CPU detection for POWER.

* snprintf define consolidated to common.h

* Support INTERFACE64=1

* Add support for INTERFACE64 and fix XERBLA calls

1. Replaced all instances of "int" with "blasint"
2. Added string length as "hidden" third parameter in calls to fortran XERBLA

* Correct length of name string in xerbla call

* Avoid out-of-bounds accesses in LAPACK EIG tests

see Reference-LAPACK/lapack#333

* Correct INFO=4 condition

* Disable reallocation of work array in xSYTRF

as it appears to cause memory management problems (seen in the LAPACK tests)

* Disable repeated recursion on Ab_BR in ReLAPACK xGBTRF

due to crashes in LAPACK tests

* sgemm/strmm

* Update Changelog with changes from 0.3.6

* Increment version to 0.3.7.dev

* Increment version to 0.3.7.dev

* Misc. typo fixes

Found via `codespell -q 3 -w -L ith,als,dum,nd,amin,nto,wis,ba -S ./relapack,./kernel,./lapack-netlib`

* Correct argument of CPU_ISSET for glibc <2.5

fixes OpenMathLib#2104

* conflict resolve

* Revert reference/ fixes

* Revert Changelog.txt typos

* Disable the SkyLakeX DGEMMITCOPY kernel as well

as a stopgap measure for numpy/numpy#13401 as mentioned in OpenMathLib#1955

* Disable DGEMMINCOPY as well for now

OpenMathLib#1955

* init

* Fix errors in cpu enumeration with glibc 2.6

for OpenMathLib#2114

* Change two http links to https

Closes OpenMathLib#2109

* remove redundant code OpenMathLib#2113

* Set up CI with Azure Pipelines

[skip ci]

* TST: add native POWER8 to CI

* add native POWER8 testing to
Travis CI matrix with ppc64le
os entry

* Update link to IBM MASS library, update cpu support status

* first try migrating one of the arm builds from travis

* fix tabbing in azure commands

* Update azure-pipelines.yml

take out offending lines (although stolen from https://github.com/conda-forge/opencv-feedstock azure-pipelines fiie)

* Update azure-pipelines.yml

* Update azure-pipelines.yml

* Update azure-pipelines.yml

* Update azure-pipelines.yml

* DOC: Add Azure CI status badge

* Add ARMV6 build to azure CI setup (OpenMathLib#2122)

using aytekinar's Alpine image and docker script from the Travis setup

[skip ci]

* TST: Azure manylinux1 & clean-up

* remove some of the steps & comments
from the original Azure yml template

* modify the trigger section to use
develop since OpenBLAS primarily uses
this branch; use the same batching
behavior as downstream projects NumPy/
SciPy

* remove Travis emulated ARMv6 gcc build
because this now happens in Azure

* use documented Ubuntu vmImage name for Azure
and add in a manylinux1 test run to the matrix

[skip appveyor]

* Add NO_AFFINITY to available options on Linux, and set it to ON

to match the gmake default. Fixes second part of OpenMathLib#2114

* Replace ISMIN and ISAMIN kernels on all x86_64 platforms (OpenMathLib#2125)

* Mark iamax_sse.S as unsuitable for MIN due to issue OpenMathLib#2116
* Use iamax.S rather than iamax_sse.S for ISMIN/ISAMIN on all x86_64 as workaround for OpenMathLib#2116

* Move ARMv8 gcc build from Travis to Azure

* Move ARMv8 gcc build from Travis to Azure

* Update .travis.yml

* Test drone CI

* install make

* remove sudo

* Install gcc

* Install perl

* Install gfortran and add a clang job

* gfortran->gcc-gfortran

* Switch to ubuntu and parallel jobs

* apt update

* Fix typo

* update yes

* no need of gcc in clang build

* Add a cmake build as well

* Add cmake builds and print options

* build without lapack on cmake

* parallel build

* See if ubuntu 19.04 fixes the ICE

* Remove qemu armv8 builds

* arm32 build

* Fix typo

* TST: add SkylakeX AVX512 CI test

* adapt the C-level reproducer code for some
recent SkylakeX AVX512 kernel issues, provided
by Isuru Fernando and modified by Martin Kroeker,
for usage in the utest suite

* add an Intel SDE SkylakeX emulation utest run to
the Azure CI matrix; a custom Docker build was required
because Ubuntu image provided by Azure does not support
AVX512VL instructions

* Add option USE_LOCKING for single-threaded build with locking support

for calling from concurrent threads

* Add option USE_LOCKING for single-threaded build with locking support

* Add option USE_LOCKING for SMP-like locking in USE_THREAD=0 builds

* Add option USE_LOCKING but keep default settings intact

* Remove unrelated change

* Do not try ancient PGI hacks with recent versions of that compiler

should fix OpenMathLib#2139

* Build and run utests in any case, they do their own checks for fortran availability

* Add softfp support in min/max kernels

fix for OpenMathLib#1912

* Revert "Add softfp support in min/max kernels"

* Separate implementations of AMAX and IAMAX on arm

As noted in OpenMathLib#1912 and comment on OpenMathLib#1942, the combined implementation happens to "do the right thing" on hardfp, but cannot return both value and index on softfp where they would have to share the return register

* Ensure correct output for DAMAX with softfp

* Use generic kernels for complex (I)AMAX to support softfp

* improved zgemm power9 based on power8

* upload thread safety test folder

* hook up c++ thread safety test (main Makefile)

*  add c++ thread test option to Makefile.rule

* Document NO_AVX512 

for OpenMathLib#2151

* sgemm pipeline improved, zgemm rewritten without inner packs, ABI lxvx v20 fixed with vs52

* Fix detection of AVX512 capable compilers in getarch

21eda8b introduced a check in getarch.c to test if the compiler is capable of
AVX512. This check currently fails, since the used __AVX2__ macro is only
defined if getarch itself was compiled with AVX2/AVX512 support. Make sure this
is the case by building getarch with -march=native on x86_64. It is only
supposed to run on the build host anyway.

* c_check: Unlink correct file

* power9 zgemm ztrmm optimized

* conflict resolve

* Add gfortran workaround for ABI violations in LAPACKE

for OpenMathLib#2154 (see gcc bug 90329)

* Add gfortran workaround for ABI violations

for OpenMathLib#2154 (see gcc bug 90329)

* Add gfortran workaround for potential ABI violation 

for OpenMathLib#2154

* Update fc.cmake

* Remove any inadvertent use of -march=native from DYNAMIC_ARCH builds

from OpenMathLib#2143, -march=native precludes use of more specific options like -march=skylake-avx512 in individual kernels, and defeats the purpose of dynamic arch anyway.

* Avoid unintentional activation of TLS code via USE_TLS=0

fixes OpenMathLib#2149

* Do not force gcc options on non-gcc compilers

fixes compile failure with pgi 18.10 as reported on OpenBLAS-users

* Update Makefile.x86_64

* Zero ecx with a mov instruction

PGI assembler does not like the initialization in the constraints.

* Fix mov syntax

* new sgemm 8x16

* Update dtrmm_kernel_16x4_power8.S

* PGI compiler does not like -march=native

* Fix build on FreeBSD/powerpc64.

Signed-off-by: Piotr Kubaj <pkubaj@anongoth.pl>

* Fix build for PPC970 on FreeBSD pt. 1

FreeBSD needs DCBT_ARG=0 as well.

* Fix build for PPC970 on FreeBSD pt.2

FreeBSD needs those macros too.

* cgemm/ctrmm power9

* Utest needs CBLAS but not necessarily FORTRAN

* Add mingw builds to Appveyor config

* Add getarch flags to disable AVX on x86

(and other small fixes to match Makefile behaviour)

* Make disabling DYNAMIC_ARCH on unsupported systems work

needs to be unset in the cache for the change to have any effect

* Mingw32 needs leading underscore on object names

(also copy BUNDERSCORE settings for FORTRAN from the corresponding Makefile)
@martin-frbg
Copy link
Collaborator

Fixed now by wjc404's new AVX512 DGEMM kernel from #2286

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

No branches or pull requests

4 participants