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

symv broken on development version for ZEN #1388

Closed
chriselrod opened this issue Dec 5, 2017 · 19 comments
Closed

symv broken on development version for ZEN #1388

chriselrod opened this issue Dec 5, 2017 · 19 comments

Comments

@chriselrod
Copy link

chriselrod commented Dec 5, 2017

I've reproduced the issue in Julia and Fortran, both linked to a fresh source build of the latest master.

program symvtest
implicit none
integer, parameter :: n = 20, p = 12
real, dimension(n, p) :: Xb
real, dimension(p, p) :: A
real, dimension(p) :: x, y, ys

call random_number(Xb)
call random_number(x)

A = matmul( transpose(Xb), Xb )
y = matmul( A, x )

ys = 0.0

call ssymv('U', p, 1.0, A, p, x, 1, 0.0, ys, 1)

print *, 'Correct:', y
print *, 'ssymv:', ys

end program symvtest
$ gfortran-7.2 symvtest.f08 -o symvt2 -pthread /opt/OpenBLAS/lib/libopenblas.a

chris@elrod:~/Documents/programming/fortran$ ./symvt2
 Correct:   40.6064873       43.2426643       37.7620964       37.9279366       37.8264160       41.4897804       40.9279518       31.3577194       28.1611786       36.6945801       34.9506340       36.2780457    
 ssymv:   57.7142792       59.8994789       52.9540024       54.8316040       52.9050522       57.4723549       58.2451744       43.7106018       28.1611786       36.6945801       34.9506340       36.2780457

In Julia:

julia> A = randn(10,20) |> X -> X * X';

julia> x = rand(10);

julia> (A * x)' #calls gemv
1×10 RowVector{Float64,Array{Float64,1}}:
 11.538  7.90415  -1.03592  -3.84546  15.5033  3.37954  7.7948  4.26935  6.29426  5.57295

julia> Base.LinAlg.BLAS.symv('U', A, x)'
1×10 RowVector{Float64,Array{Float64,1}}:
 21.5629  15.7696  -4.14023  -10.6005  28.2271  5.50057  17.9761  6.60855  6.29426  5.57295

EDIT:
When building OpenBLAS it runs tests; excerpt:

OPENBLAS_NUM_THREADS=2 ./xdcblat3 < din3
 TESTS OF THE DOUBLE PRECISION LEVEL 3 BLAS

 THE FOLLOWING PARAMETER VALUES WILL BE USED:
   FOR N                   1     2     3     5     7     9
   FOR ALPHA             0.0   1.0   0.7
   FOR BETA              0.0   1.0   1.3

 ROUTINES PASS COMPUTATIONAL TESTS IF TEST RATIO IS LESS THAN   16.00

 COLUMN-MAJOR AND ROW-MAJOR DATA LAYOUTS ARE TESTED

 RELATIVE MACHINE PRECISION IS TAKEN TO BE  2.2D-16

 cblas_dgemm  PASSED THE TESTS OF ERROR-EXITS

 cblas_sgbmv  PASSED THE ROW-MAJOR    COMPUTATIONAL TESTS ( 17284 CALLS)

 cblas_ssymv  PASSED THE TESTS OF ERROR-EXITS

 cblas_ssymv  PASSED THE COLUMN-MAJOR COMPUTATIONAL TESTS (  1729 CALLS)
 cblas_dgemm  PASSED THE COLUMN-MAJOR COMPUTATIONAL TESTS ( 17496 CALLS)
 cblas_ssymv  PASSED THE ROW-MAJOR    COMPUTATIONAL TESTS (  1729 CALLS)

 cblas_ssbmv  PASSED THE TESTS OF ERROR-EXITS

 cblas_ssbmv  PASSED THE COLUMN-MAJOR COMPUTATIONAL TESTS (  6913 CALLS)
 cblas_dgemm  PASSED THE ROW-MAJOR    COMPUTATIONAL TESTS ( 17496 CALLS)

 cblas_dsymm  PASSED THE TESTS OF ERROR-EXITS

I assume the tests check that the answers are valid?
A little unsure what to make of this.

As a workaround I can copy values from the upper triangle into the lower triangle and use gemm, but I'd rather fix the problem.
Any idea what's going on?
Any suggestions of what I can look at, tests to run, etc?

@brada4
Copy link
Contributor

brada4 commented Dec 5, 2017

Can you show julia sysinfo(true) to see openblas core type used?

@brada4
Copy link
Contributor

brada4 commented Dec 5, 2017

Can you chech with bigger sample that first 8 (32 bYtes) values are corrupt but rest is fine?

@chriselrod
Copy link
Author

chriselrod commented Dec 6, 2017

Oops, the pattern definitely changes as size increases.
At n <= 8:
All answers are correct
At 8 < n <= 155:
rem(n,4) are correct , unless that remainder is zero in which case 4 are correct.
Easier to show:

julia> function check_symv(n)
           A = randn(n, 2n) |> X -> X * X';
           x = rand(n);
           gemv = A * x
           symv = Base.LinAlg.BLAS.symv('U', A, x)
           correct = gemv .≈ symv
           correct_count = sum(correct)
           eight = min(8,n)
           correct_in_first_8 = sum(@view(correct[1:eight]))
           n, correct_count, n - correct_count, correct_in_first_8, eight - correct_in_first_8
       end
check_symv (generic function with 1 method)

julia> check_symv.(1:30)
30-element Array{NTuple{5,Int64},1}:
 (1, 1, 0, 1, 0)  
 (2, 2, 0, 2, 0)  
 (3, 3, 0, 3, 0)  
 (4, 4, 0, 4, 0)  
 (5, 5, 0, 5, 0)  
 (6, 6, 0, 6, 0)  
 (7, 7, 0, 7, 0)  
 (8, 8, 0, 8, 0)  
 (9, 1, 8, 0, 8)  
 (10, 2, 8, 0, 8) 
 (11, 3, 8, 0, 8) 
 (12, 4, 8, 0, 8) 
 (13, 1, 12, 0, 8)
 (14, 2, 12, 0, 8)
 (15, 3, 12, 0, 8)
 (16, 4, 12, 0, 8)
 (17, 1, 16, 0, 8)
 (18, 2, 16, 0, 8)
 (19, 3, 16, 0, 8)
 (20, 4, 16, 0, 8)
 (21, 1, 20, 0, 8)
 (22, 2, 20, 0, 8)
 (23, 3, 20, 0, 8)
 (24, 4, 20, 0, 8)
 (25, 1, 24, 0, 8)
 (26, 2, 24, 0, 8)
 (27, 3, 24, 0, 8)
 (28, 4, 24, 0, 8)
 (29, 1, 28, 0, 8)
 (30, 2, 28, 0, 8)

Columns are:

  1. n
  2. total correct
  3. total incorrect
  4. number in first 8 that are correct
  5. number in the first 8 that are incorrect

At 156 and above the pattern becomes more complicated:

julia> check_symv.(149:172)
24-element Array{NTuple{5,Int64},1}:
 (149, 1, 148, 0, 8)
 (150, 2, 148, 0, 8)
 (151, 3, 148, 0, 8)
 (152, 4, 148, 0, 8)
 (153, 1, 152, 0, 8)
 (154, 2, 152, 0, 8)
 (155, 3, 152, 0, 8)
 (156, 8, 148, 0, 8)
 (157, 1, 156, 0, 8)
 (158, 2, 156, 0, 8)
 (159, 3, 156, 0, 8)
 (160, 4, 156, 0, 8)
 (161, 5, 156, 0, 8)
 (162, 2, 160, 0, 8)
 (163, 3, 160, 0, 8)
 (164, 4, 160, 0, 8)
 (165, 5, 160, 0, 8)
 (166, 6, 160, 0, 8)
 (167, 3, 164, 0, 8)
 (168, 4, 164, 0, 8)
 (169, 5, 164, 0, 8)
 (170, 6, 164, 0, 8)
 (171, 7, 164, 0, 8)
 (172, 4, 168, 0, 8)

As for sysinfo, is this what you wanted?

julia> versioninfo()
Julia Version 0.7.0-DEV.2767
Commit 8fb8db1 (2017-12-05 18:20 UTC)
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: AMD Ryzen Threadripper 1950X 16-Core Processor
  WORD_SIZE: 64
  BLAS: libopenblas (ZEN)
  LAPACK: liblapack
  LIBM: libopenlibm
  LLVM: libLLVM-3.9.1 (ORCJIT, znver1)
Environment:

Or

julia> versioninfo()
Julia Version 0.6.1
Commit 0d7248e* (2017-10-24 22:15 UTC)
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: AMD Ryzen Threadripper 1950X 16-Core Processor
  WORD_SIZE: 64
  BLAS: libopenblas (ZEN)
  LAPACK: liblapack
  LIBM: libopenlibm
  LLVM: libLLVM-3.9.1 (ORCJIT, generic)

Core type is ZEN.

EDIT:
A few more ranges. If there are any that would be particularly diagnostic, let me know:

julia> check_symv.(1020:1040)
21-element Array{NTuple{5,Int64},1}:
 (1020, 16, 1004, 0, 8)
 (1021, 17, 1004, 0, 8)
 (1022, 18, 1004, 0, 8)
 (1023, 19, 1004, 0, 8)
 (1024, 16, 1008, 0, 8)
 (1025, 17, 1008, 0, 8)
 (1026, 18, 1008, 0, 8)
 (1027, 19, 1008, 0, 8)
 (1028, 20, 1008, 0, 8)
 (1029, 17, 1012, 0, 8)
 (1030, 18, 1012, 0, 8)
 (1031, 19, 1012, 0, 8)
 (1032, 20, 1012, 0, 8)
 (1033, 21, 1012, 0, 8)
 (1034, 22, 1012, 0, 8)
 (1035, 23, 1012, 0, 8)
 (1036, 24, 1012, 0, 8)
 (1037, 21, 1016, 0, 8)
 (1038, 22, 1016, 0, 8)
 (1039, 23, 1016, 0, 8)
 (1040, 24, 1016, 0, 8)

julia> check_symv.(2040:2060)
21-element Array{NTuple{5,Int64},1}:
 (2040, 56, 1984, 0, 8)
 (2041, 53, 1988, 0, 8)
 (2042, 54, 1988, 0, 8)
 (2043, 55, 1988, 0, 8)
 (2044, 56, 1988, 0, 8)
 (2045, 53, 1992, 0, 8)
 (2046, 54, 1992, 0, 8)
 (2047, 55, 1992, 0, 8)
 (2048, 56, 1992, 0, 8)
 (2049, 53, 1996, 0, 8)
 (2050, 54, 1996, 0, 8)
 (2051, 55, 1996, 0, 8)
 (2052, 56, 1996, 0, 8)
 (2053, 53, 2000, 0, 8)
 (2054, 54, 2000, 0, 8)
 (2055, 55, 2000, 0, 8)
 (2056, 56, 2000, 0, 8)
 (2057, 53, 2004, 0, 8)
 (2058, 54, 2004, 0, 8)
 (2059, 55, 2004, 0, 8)
 (2060, 56, 2004, 0, 8)

julia> check_symv.(4080:5100) #Meant to do 4080-4100.
1021-element Array{NTuple{5,Int64},1}:
 (4080, 116, 3964, 0, 8)
 (4081, 113, 3968, 0, 8)
 (4082, 114, 3968, 0, 8)
 (4083, 115, 3968, 0, 8)
 (4084, 112, 3972, 0, 8)
 (4085, 113, 3972, 0, 8)
 (4086, 114, 3972, 0, 8)
 (4087, 115, 3972, 0, 8)
 (4088, 116, 3972, 0, 8)
 (4089, 117, 3972, 0, 8)
 (4090, 114, 3976, 0, 8)
 (4091, 115, 3976, 0, 8)
 (4092, 116, 3976, 0, 8)
 (4093, 117, 3976, 0, 8)
 (4094, 114, 3980, 0, 8)
 (4095, 115, 3980, 0, 8)
 (4096, 116, 3980, 0, 8)
 (4097, 117, 3980, 0, 8)
 (4098, 118, 3980, 0, 8)
 (4099, 119, 3980, 0, 8)
 (4100, 116, 3984, 0, 8)
 (4101, 117, 3984, 0, 8)
                       
 (5080, 152, 4928, 0, 8)
 (5081, 153, 4928, 0, 8)
 (5082, 150, 4932, 0, 8)
 (5083, 151, 4932, 0, 8)
 (5084, 152, 4932, 0, 8)
 (5085, 153, 4932, 0, 8)
 (5086, 154, 4932, 0, 8)
 (5087, 155, 4932, 0, 8)
 (5088, 156, 4932, 0, 8)
 (5089, 157, 4932, 0, 8)
 (5090, 154, 4936, 0, 8)
 (5091, 155, 4936, 0, 8)
 (5092, 152, 4940, 0, 8)
 (5093, 153, 4940, 0, 8)
 (5094, 154, 4940, 0, 8)
 (5095, 151, 4944, 0, 8)
 (5096, 152, 4944, 0, 8)
 (5097, 153, 4944, 0, 8)
 (5098, 154, 4944, 0, 8)
 (5099, 155, 4944, 0, 8)
 (5100, 152, 4948, 0, 8)

At small sizes, symv benchmarked at over 10 times slower than gemv with this setup, besides producing the wrong answer.

@martin-frbg
Copy link
Collaborator

There is something wrong with the fortran testcase it seems, you definitely need to uncomment the initialization statements for ys, else every second number is random uninitialized memory contents.
Haswell,Nehalem and Atom builds run on Kaby Lake produce numbers that differ in the last two digits only. Are your numbers obtained with a recent build of OpenBLAS ? The first snapshots with ZEN
support from late March, early April happened to be severely broken, see #1147

@chriselrod
Copy link
Author

chriselrod commented Dec 6, 2017

I changed beta in the symv call to 0.0, and also added the ys = 0.0 in case some uninitialized entries are treated as NaN or Infs.

Just confirmed -- I'm on the develop branch:

$ git pull
Already up-to-date.
$ git checkout develop
Already on 'develop'
Your branch is up-to-date with 'origin/develop'.

Just to confirm, after building, I ran sudo make install -- does this properly replace the old install, which would have been build a couple months ago (and I do not recall which version I had checked out at the time)?
If not, what would I have to do to correctly update?

EDIT:
Should I try building with TARGET=HASWELL ?
Update: Build fails when I run make with TARGET=HASWELL; lots of undefined references:

<snip>
linktest.c:(.text.startup+0x8490): undefined reference to `LAPACKE_sgeqpf'
linktest.c:(.text.startup+0x8497): undefined reference to `LAPACKE_sgeqpf_work'
linktest.c:(.text.startup+0x849e): undefined reference to `LAPACKE_zgeqpf'
linktest.c:(.text.startup+0x84a5): undefined reference to `LAPACKE_zgeqpf_work'
collect2: error: ld returned 1 exit status
Makefile:143: recipe for target '../libopenblas_haswellp-r0.3.0.dev.so' failed
make[1]: *** [../libopenblas_haswellp-r0.3.0.dev.so] Error 1
make[1]: Leaving directory '/home/chris/Documents/programming/OpenBLAS/exports'
Makefile:90: recipe for target 'shared' failed
make: *** [shared] Error 2

@chriselrod
Copy link
Author

Using the lower triangle instead:

julia> function check_symv(n, Uplo = 'U')
           A = randn(n, 2n) |> X -> X * X';
           x = rand(n);
           gemv = A * x
           symv = Base.LinAlg.BLAS.symv(Uplo, A, x)
           correct = gemv .≈ symv
           correct_count = sum(correct)
           eight = min(8,n)
           correct_in_first_8 = sum(@view(correct[1:eight]))
           n, correct_count, n - correct_count, correct_in_first_8, eight - correct_in_first_8
       end
check_symv (generic function with 1 method)

julia> check_symv.(1:20, 'L')
20-element Array{NTuple{5,Int64},1}:
 (1, 1, 0, 1, 0)  
 (2, 2, 0, 2, 0)  
 (3, 3, 0, 3, 0)  
 (4, 4, 0, 4, 0)  
 (5, 5, 0, 5, 0)  
 (6, 6, 0, 6, 0)  
 (7, 7, 0, 7, 0)  
 (8, 8, 0, 8, 0)  
 (9, 8, 1, 8, 0)  
 (10, 8, 2, 8, 0) 
 (11, 8, 3, 8, 0) 
 (12, 8, 4, 8, 0) 
 (13, 8, 5, 8, 0) 
 (14, 8, 6, 8, 0) 
 (15, 8, 7, 8, 0) 
 (16, 8, 8, 8, 0) 
 (17, 8, 9, 8, 0) 
 (18, 8, 10, 8, 0)
 (19, 8, 11, 8, 0)
 (20, 8, 12, 8, 0)

julia> check_symv.(149:172, 'L')
24-element Array{NTuple{5,Int64},1}:
 (149, 8, 141, 8, 0) 
 (150, 8, 142, 8, 0) 
 (151, 8, 143, 8, 0) 
 (152, 8, 144, 8, 0) 
 (153, 8, 145, 8, 0) 
 (154, 12, 142, 8, 0)
 (155, 12, 143, 8, 0)
 (156, 12, 144, 8, 0)
 (157, 12, 145, 8, 0)
 (158, 16, 142, 8, 0)
 (159, 16, 143, 8, 0)
 (160, 16, 144, 8, 0)
 (161, 16, 145, 8, 0)
 (162, 16, 146, 8, 0)
 (163, 16, 147, 8, 0)
 (164, 16, 148, 8, 0)
 (165, 16, 149, 8, 0)
 (166, 16, 150, 8, 0)
 (167, 16, 151, 8, 0)
 (168, 16, 152, 8, 0)
 (169, 16, 153, 8, 0)
 (170, 16, 154, 8, 0)
 (171, 16, 155, 8, 0)
 (172, 16, 156, 8, 0)

@chriselrod
Copy link
Author

I asked on gitter if anyone with a Ryzen processor could reproduce, and someone kindly responded with: https://pastebin.com/hJX8gF6T
They could not.

I assumed they didn't manually build OpenBLAS from source, either using a precompiled Julia binary (comes linked with OpenBLAS), or built Julia (which automatically downloads and builds OpenBLAS).
Because of this I tried checkout the last tagged version of OpenBLAS:

$ make clean
$ git checkout v0.2.20
$ make CC=gcc-7.2 FC=gfortran-7.2 COMMON_OPT=-O3 -j32
$ sudo make install

and now I get:

$ gfortran-7.2 -O3 symvtest.f08 -o symvt2 -pthread /opt/OpenBLAS/lib/libopenblas.a
$ ./symvt2
 Correct:   30.9002171       32.3528900       29.4461060       29.0806026       24.1383610       31.0188465       29.0695744       30.7085075       32.8042488       28.7348518       33.4618492       34.3150177    
 ssymv:   30.9002151       32.3528900       29.4461060       29.0806026       24.1383610       31.0188465       29.0695744       30.7085056       32.8042488       28.7348518       33.4618492       34.3150177    

So, now my problem is solved.

However, I am leaving the issue open because the development version of OpenBLAS's symv is broken on Rzyen.

@chriselrod chriselrod changed the title First 8 answers of symv incorrect? symv broken on development version for ZEN Dec 7, 2017
@martin-frbg
Copy link
Collaborator

Can you confirm that your build of "develop" had actually been installed in the correct location ? There have not been any commits since 0.2.20 that targetted the Ryzen architecture, if anything I would suspect the recent code cleanup that removed a number of (obviously) unused variables. However the target is supposed to be identical to Intel "Haswell" and so far I am unable to reproduce your problem on Intel. Ideally we would need someone with a Ryzen system to do a "git bisect" between 0.2.20 and today if there is actually a problem with the current development version.

@chriselrod
Copy link
Author

chriselrod commented Dec 7, 2017

Okay:

$ git clone git://github.com/xianyi/OpenBLAS.git
$ cd OpenBLAS
$ git checkout develop #already on develop
$ make CC=gcc-7.2 FC=gfortran-7.2 COMMON_OPT=-O3 -j32
$ sudo make install PREFIX=/opt/OpenBLAS-dev
$ gfortran-7.2 -O3 symvtest.f08 -o symvt -pthread /opt/OpenBLAS-dev/lib/libopenblas.a
$ ./symvt # Linked to the development version
 Correct:   38.3103065       28.6310291       31.0854874       38.3372383       27.0824089       29.8740845       24.2437725       28.0576820       28.3724155       33.8617172       37.3014297       31.5492420    
 ssymv:   52.3679199       38.7304001       43.0292168       51.6194725       37.2317848       41.5859642       34.4001999       38.4238358       28.3724155       33.8617172       37.3014297       31.5492420
$ gfortran-7.2 -O3 symvtest.f08 -o symvtv2.2 -pthread /opt/OpenBLAS/lib/libopenblas.a
$ ./symvtv2.2 # Linked to OpenBLAS v0.2.20, which is installed at /opt/OpenBLAS
 Correct:   32.1122055       30.0320606       38.3960342       36.7253914       39.5436707       41.6092072       42.0634842       40.0813179       42.7693214       39.7569427       40.9749603       33.1785774    
 ssymv:   32.1122055       30.0320606       38.3960342       36.7253914       39.5436707       41.6092072       42.0634842       40.0813141       42.7693214       39.7569427       40.9749603       33.1785774

Let me know if you'd like me to try anything else.

@martin-frbg
Copy link
Collaborator

Could you try with "git checkout 5f402b7" please ? That would roll back your checkout of "develop" to directly before the only commit that touched the ssymv_U kernel file. (I still do not understand how that commit - removing the declarations of four unused variables - would cause such problems, unless there is some memory corruption occuring that was previously mitigated by the padding)

@brada4
Copy link
Contributor

brada4 commented Dec 7, 2017

Im compartite in wondering about padding-looking declarations i removed around

@chriselrod
Copy link
Author

After git checkout 5f402b7 and re-making, still get the wrong answer:

$ gfortran-7.2 -O3 symvtest.f08 -o symvt -pthread /opt/OpenBLAS-dev/lib/libopenblas.a
$ ./symvt
 Correct:   28.5529785       22.0841026       27.2444382       23.6138706       22.2497787       21.6344509       22.6846790       21.3639870       27.3313675       23.4497471       24.7438240       22.2422047    
 __ssymv:   43.7869606       34.2717743       42.4019775       36.6149216       34.4411087       34.6225929       35.2519379       33.8367157       27.3313675       23.4497471       24.7438240       22.2422047
$ ./symvt
 Correct:   24.1835136       27.7404785       24.7421417       28.3680420       23.6532784       26.2750969       22.8979492       22.0192585       22.0544701       25.9655113       29.9678802       25.3623199    
 __ssymv:   28.8016720       35.4656982       30.4767952       35.6221695       29.4691963       31.3436108       28.4602337       26.6515293       22.0544701       25.9655113       29.9678802       25.3623199

=/
While I don't get why removing unused variables would change anything, it would also have been nice to have a lead. I'll try reverting to different commits tonight to pin down which is the first to break it.

@martin-frbg
Copy link
Collaborator

Thank you very much for your help. git bisect will probably speed up the process a bit, as it will automatically determine which version to checkout next to narrow down the search. (Do git bisect start followed by git bisect good 5dde4e6 and git bisect bad 5f402b7 - this will fetch the commit in the middle between 0.2.20 and what you just tried. Build and test this, then depending on outcome enter git bisect good or git bisect bad until git responds with "xxx is the first bad commit". git bisect reset followed by git checkout will then return your checked-out copy to the current "develop" state)

@martin-frbg
Copy link
Collaborator

If you do not have the time for pinning it down exactly, testing a single version from end of August should narrow it down considerably I think.

@chriselrod
Copy link
Author

chriselrod commented Dec 7, 2017

Cool, git bisect is super convenient, thanks for the explanation.

$ git bisect bad
c4e5ba1bfe8c7c4e263d5c14f4034e657347b591 is the first bad commit
commit c4e5ba1bfe8c7c4e263d5c14f4034e657347b591
Author: Martin Kroeker <[email protected]>
Date:   Wed Aug 2 00:37:58 2017 +0200

    Make sure that range_n of last thread never exceeds the actual data size when splitting the workload

:040000 040000 a41f862722d95efb3ac1fa97540df61d9e96eb34 bc42160ee82f36c3e6af2c66acd47daddbd649b2 M	driver

OpenBLAS is an awesome project, so I'm happy to help. Also worth pointing out that while ssymv produces incorrect answers (and dsymv, which is what the Julia tests used), ssymm is correct.

I don't know the source code at all, but it looks like the same change was made to several different files here.
Let me know if you want me to test the other functions / do anything else.

@martin-frbg
Copy link
Collaborator

Eww. Yes, the changes were systematic, and probably systematically wrong. Guess at the very least these all need changing the "m" on both sides of the statements to "m*num_cpu" (an error that would not be caught when testing on a lowly two-core laptop, but would wreak havoc on serious hardware). Guess the error in multithreaded dtrmv (#1332) may turn out to be such an own goal as well.

@brada4
Copy link
Contributor

brada4 commented Dec 8, 2017

The change follows fixing wrong sizing by one thread control structures, when in addition last thread was happilly multiplying way past inpputs and outputs assigned....

@martin-frbg
Copy link
Collaborator

Closed automatically by my commit - reopening for confirmation of the fix.

@martin-frbg martin-frbg reopened this Dec 9, 2017
@chriselrod
Copy link
Author

Awesome and thanks --
Checking out the development version again:

$ ./symvt
 Correct:   43.8041573       43.1802940       35.9221725       35.8977928       47.3322906       38.5851173       40.6959877       35.2439804       47.8684769       36.6580925       42.2379990       36.4648247    
 __ssymv:   43.8041573       43.1802940       35.9221725       35.8977890       47.3322945       38.5851135       40.6959877       35.2439804       47.8684769       36.6580925       42.2379990       36.4648247    

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 <[email protected]>

* 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 <[email protected]>

* 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)
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

3 participants