A lot of warnings in -Wunused-but-set-variable and Wunused-variable

[luotao1]

I grep our TeamCity log. There are 816 -Wunused-but-set-variable and 386 -Wunused-variable warnings come from OpenBLAS: PaddlePaddle/Paddle#3281 (comment)

Do you have any plans to fix these warnings?
Looking forward to your reply!

[martin-frbg]

While I can only speak for myself, I did not see them as an immediate priority problem so far given the lack of active developers on this (mainly) volunteer project. Also when this topic last came up months ago I did not want to touch the level3 blas code as it seemed another developer was actively working on the affected functions. Fixing the warnings should be straightforward if time consuming, but I would hate to clean up harmless declarations of currently unused things that actually may be hooks already put in place for later developments. (Though most likely the majority will simply be leftovers from cut-and-paste coding.)
Did you experience any problems in connection with these warnings, or is it just that this creates an unfavorable impression about the general quality of the code ?

[martin-frbg]

Regarding the warnings arising from the fallback codes in kernel/generic, I guess I could offer the additional excuse that these are historic slips of the pen, written by Kazushige Goto a decade ago for what was then libGoto or GotoBLAS. So they have been there from the beginning of (our) time :-)

[luotao1]

Thanks for your quick reply.

Did you experience any problems in connection with these warnings, or is it just that this creates an unfavorable impression about the general quality of the code

We didn't experience problems, but it creates a very long log for our teamcity.

Fixing the warnings should be straightforward if time consuming

How about add Wunused-but-set-variable and Wunused-variable in flags such as CMAKE_CXX_FLAGS, which can avoid these warnings?

[martin-frbg]

Suppressing the warnings will probably draw criticism from others for sweeping things under the rug...
At least for the "historic" codes in kernel/generic, the messages arise from varying macro expansion as often in OpenBLAS the same source file is compiled into multiple related functions for handling float/double/complex input etc. So "fixing" would mean adding a ton of #ifdefs rather than just commenting or removing truely unneeded lines. It is quite likely that this applies to the level3 blas codes as well, while at least the j and at[] variables in the x86_64 gemv and symv kernels appear to be genuinely useless. (Still time that could be spent on fixing actual bugs that lead to actual errors)

[martin-frbg]

One other thing to consider is that when you are doing a build with DYNAMIC_ARCH=1 to allow runtime selection of code for a wide range of cpus, you will be compiling the same source files over and over where they are shared with or without small adjustments between cpu models. So the actual number of unique files is much smaller than the impressive number of 800 warnings that will only jump up whenever support for another related cpu is added. (And the actual number of unique issues is smaller still, as the same file may contain several computations where one of the arguments is unused under certain build-time circumstances.)
As an aside, if your coworker thinks a project must be poorly maintained if it has 150 open issues of widely varying severity, maybe he should look at tensorflow, numpy or julialang...

[brada4]

Dead code causing both warnings is optimized away with any optimizing compiler anyway. i.e fix would be cosmetic for build process but noop for code generated

[martin-frbg]

Hmm. The unused-but-set warning about n in level3_gemm3m_thread.c may point to an actual shortcoming in the code where (I think) it may theoretically create threads even for very small workloads when range_m or range_n is set and either range does not correspond to the m or n argument. (m and in particular n is calculated from the range limits, but args->m, args->n are used in the decision - I guess that translates into "for operations on submatrices, the dimensions of the entire matrix are still used to decide whether or not to multithread here"). Not sure about actual impact, at least this has been unchanged since libGoto.
The kernel/generic/zgemm_?copy_ stuff on the other hand is probably best masked with a cast to (void) as the alternative would be adding a ton of ifdefs. As mentioned above, actual use of these
variables depends on which flavor of zgemm_?copyi, ...r, or ...b is getting built from the single source.

[brada4]

clang4 picks on a bit more like unused after increment that gcc cannot notice. I will take on those, it is mostly no-brain work.

[luotao1]

@martin-frbg @brada4 Thanks very much for your quick pull request!
Besides, there are 18 near initialization warnings come from OpenBLAS: PaddlePaddle/Paddle#3281 (comment)

[luotao1]

PaddlePaddle/Paddle#5704 compile OpenBLAS with -Wno-unused-but-set-variable -Wno-unused-variable temporary to avoid long log.

[martin-frbg]

That's actually

setparam_PRESCOTT.c:560:60: warning: initialization from incompatible pointer type
setparam_PRESCOTT.c:560:60: note: (near initialization for 'gotoblas_PRESCOTT.zgeadd_k')

repeated for all the 18 cpu variants in the build due to some confusion if the third argument to zgeadd_k should be a float or a double. The majority claims "double", only common_param.h thinks it is a float - let me see who wins...

[martin-frbg]

I believe these warnings have been all but eliminated now, at least for the x86 DYNAMIC_ARCH case.
Remaining warnings are mostly (imported) from netlib lapack, and should probably be fixed in that codebase first. OK to close ?

[brada4]

@martin-frbg still the cross targets (w32 for example) are not handled. Also many -Wunused-but-set are left, I fixed only part that gcc dos not see of that class - unused-but-incremented( += etc), which made half clang warnings earlier.
Lots of editing still ahead to clear output mentioned here, and nobody can take fame of eliminating 100kB of dead code for full build....

[martin-frbg]

Code cleanup can continue (though it is probably best to comment offending lines instead of removing them, so that any wrong assumptions or unfinished plans remain visible, in particular in original K.Goto code). I was only suggesting that the issues within the scope of this particular ticket have been resolved.

[brada4]

They are being resolved, prioritising parts that change code (and are more suspect to unearth bugs) ;)
I am even trying to keep line numbers intact....

[brada4]

Also assembly kernels that get called fortran style (i.e with pointer arguments where assembly accesses RW pointed-to data) are not understood and warning gets generated.
In principle they could be wrapped with pragma directives to silence misguided compiler warning. @martin-frbg may I?

[martin-frbg]

Which compiler are you seeing this with ? Offhand this sounds more like a compiler problem, and I am not too keen on making purely cosmetic changes all over the place just to silence a pointless warning generated by a particular compiler version that may not even affect many people.

[brada4]

I agree. It is gcc

[martin-frbg]

gcc but using some uncommon warning options I assume ? With 7.2.0 at -Wall (or even with the two options from the original issue topic), I notice a handful of the newfangled "misleading indentation" warnings and a few complaints about uninitialized variables in the tests, but the vast majority of warnings now is caused by coding style issues in netlib lapack. Hence my earlier suggestion to close this issue rather than embark on a coding style witchhunt just to placate a single (perhaps overly) picky compiler.

[brada4]

only out of warnings i addressed that look loke bugfixes were =0.0 and to lesser degree dead increment. Rest is plainly cosmetic.

[brada4]

Actually just a dozen of libm fabs and very few type mismatch warnings are left in plain make.
clang analyzer just misses asm, and sets some rarely used warning types
I did do test builds with NO_CBLAS NO_LAPACK and once sandy & piledriver are clean extra with DYNAMIC_ARCH to make it quick and not involve imported bits.

[brada4]

I think it is cleaned as much as possible. I will look other time at my yesterdays edits and make PR

is@localhost:~/OpenBLAS> make CC=gcc NO_CBLAS=1 DYNAMIC_ARCH=1 FC=DISABLED MAKE_NB_JOBS=-1 -s -j 5 2>&1 | tee ../gcc7.out
OpenBLAS: Detecting fortran compiler failed. Cannot compile LAPACK. Only compile BLAS.
ar: creating ../libopenblasp-r0.3.0.dev.a
OK.

 OpenBLAS build complete. (BLAS)

  OS               ... Linux             
  Architecture     ... x86_64               
  BINARY           ... 64bit                 
  C compiler       ... GCC  (command line : gcc)
  Library Name     ... libopenblasp-r0.3.0.dev.a (Multi threaded; Max num-threads is 6)

To install the library, you can run "make PREFIX=/path/to/your/installation install".

[brada4]

clang5
fabs() is defined for long double args as same as fabsl(), so warning is fake.
probably unused function had great future intended. It is shorter than warning text, and does not make any side effect at all.

is@localhost:~/Templates/OpenBLAS/trunk> make CC=clang NO_CBLAS=1 DYNAMIC_ARCH=1 FC=DISABLED MAKE_NB_JOBS=-1 -s -j 5 2>&1 | tee ../clang5.out   
OpenBLAS: Detecting fortran compiler failed. Cannot compile LAPACK. Only compile BLAS.
rotg.c:25:21: warning: absolute value function 'fabs' given an argument of type 'long double' but has parameter of type 'double' which may cause truncation of value [-Wabsolute-value]
  long double ada = fabs(da);
                    ^
rotg.c:25:21: note: use function 'fabsl' instead
  long double ada = fabs(da);
                    ^~~~
                    fabsl
rotg.c:26:21: warning: absolute value function 'fabs' given an argument of type 'long double' but has parameter of type 'double' which may cause truncation of value [-Wabsolute-value]
  long double adb = fabs(db);
                    ^
rotg.c:26:21: note: use function 'fabsl' instead
  long double adb = fabs(db);
                    ^~~~
                    fabsl
2 warnings generated.
rotg.c:25:21: warning: absolute value function 'fabs' given an argument of type 'long double' but has parameter of type 'double' which may cause truncation of value [-Wabsolute-value]
  long double ada = fabs(da);
                    ^
rotg.c:25:21: note: use function 'fabsl' instead
  long double ada = fabs(da);
                    ^~~~
                    fabsl
rotg.c:26:21: warning: absolute value function 'fabs' given an argument of type 'long double' but has parameter of type 'double' which may cause truncation of value [-Wabsolute-value]
  long double adb = fabs(db);
                    ^
rotg.c:26:21: note: use function 'fabsl' instead
  long double adb = fabs(db);
                    ^~~~
                    fabsl
2 warnings generated.
zrotg.c:17:21: warning: absolute value function 'fabs' given an argument of type 'long double' but has parameter of type 'double' which may cause truncation of value [-Wabsolute-value]
  long double ada = fabs(da_r) + fabs(da_i);
                    ^
zrotg.c:17:21: note: use function 'fabsl' instead
  long double ada = fabs(da_r) + fabs(da_i);
                    ^~~~
                    fabsl
zrotg.c:17:34: warning: absolute value function 'fabs' given an argument of type 'long double' but has parameter of type 'double' which may cause truncation of value [-Wabsolute-value]
  long double ada = fabs(da_r) + fabs(da_i);
                                 ^
zrotg.c:17:34: note: use function 'fabsl' instead
  long double ada = fabs(da_r) + fabs(da_i);
                                 ^~~~
                                 fabsl
2 warnings generated.
zrotg.c:17:21: warning: absolute value function 'fabs' given an argument of type 'long double' but has parameter of type 'double' which may cause truncation of value [-Wabsolute-value]
  long double ada = fabs(da_r) + fabs(da_i);
                    ^
zrotg.c:17:21: note: use function 'fabsl' instead
  long double ada = fabs(da_r) + fabs(da_i);
                    ^~~~
                    fabsl
zrotg.c:17:34: warning: absolute value function 'fabs' given an argument of type 'long double' but has parameter of type 'double' which may cause truncation of value [-Wabsolute-value]
  long double ada = fabs(da_r) + fabs(da_i);
                                 ^
zrotg.c:17:34: note: use function 'fabsl' instead
  long double ada = fabs(da_r) + fabs(da_i);
                                 ^~~~
                                 fabsl
2 warnings generated.
ar: creating ../libopenblasp-r0.3.0.dev.a
setparam_PRESCOTT.c:673:23: warning: unused function 'get_l3_size' [-Wunused-function]
static __inline__ int get_l3_size(void){
                      ^
1 warning generated.
setparam_CORE2.c:673:23: warning: unused function 'get_l3_size' [-Wunused-function]
static __inline__ int get_l3_size(void){
                      ^
1 warning generated.
setparam_PENRYN.c:673:23: warning: unused function 'get_l3_size' [-Wunused-function]
static __inline__ int get_l3_size(void){
                      ^
1 warning generated.
setparam_DUNNINGTON.c:673:23: warning: unused function 'get_l3_size' [-Wunused-function]
static __inline__ int get_l3_size(void){
                      ^
1 warning generated.
setparam_NEHALEM.c:673:23: warning: unused function 'get_l3_size' [-Wunused-function]
static __inline__ int get_l3_size(void){
                      ^
1 warning generated.
setparam_OPTERON.c:673:23: warning: unused function 'get_l3_size' [-Wunused-function]
static __inline__ int get_l3_size(void){
                      ^
1 warning generated.
setparam_OPTERON_SSE3.c:673:23: warning: unused function 'get_l3_size' [-Wunused-function]
static __inline__ int get_l3_size(void){
                      ^
1 warning generated.
setparam_BARCELONA.c:673:23: warning: unused function 'get_l3_size' [-Wunused-function]
static __inline__ int get_l3_size(void){
                      ^
1 warning generated.
setparam_BOBCAT.c:673:23: warning: unused function 'get_l3_size' [-Wunused-function]
static __inline__ int get_l3_size(void){
                      ^
1 warning generated.
setparam_ATOM.c:673:23: warning: unused function 'get_l3_size' [-Wunused-function]
static __inline__ int get_l3_size(void){
                      ^
1 warning generated.
setparam_NANO.c:673:23: warning: unused function 'get_l3_size' [-Wunused-function]
static __inline__ int get_l3_size(void){
                      ^
1 warning generated.
setparam_SANDYBRIDGE.c:673:23: warning: unused function 'get_l3_size' [-Wunused-function]
static __inline__ int get_l3_size(void){
                      ^
1 warning generated.
setparam_BULLDOZER.c:673:23: warning: unused function 'get_l3_size' [-Wunused-function]
static __inline__ int get_l3_size(void){
                      ^
1 warning generated.
setparam_PILEDRIVER.c:673:23: warning: unused function 'get_l3_size' [-Wunused-function]
static __inline__ int get_l3_size(void){
                      ^
1 warning generated.
setparam_STEAMROLLER.c:673:23: warning: unused function 'get_l3_size' [-Wunused-function]
static __inline__ int get_l3_size(void){
                      ^
1 warning generated.
setparam_EXCAVATOR.c:673:23: warning: unused function 'get_l3_size' [-Wunused-function]
static __inline__ int get_l3_size(void){
                      ^
1 warning generated.
setparam_HASWELL.c:673:23: warning: unused function 'get_l3_size' [-Wunused-function]
static __inline__ int get_l3_size(void){
                      ^
1 warning generated.
setparam_ZEN.c:673:23: warning: unused function 'get_l3_size' [-Wunused-function]
static __inline__ int get_l3_size(void){
                      ^
1 warning generated.
OK.

 OpenBLAS build complete. (BLAS)

  OS               ... Linux             
  Architecture     ... x86_64               
  BINARY           ... 64bit                 
  C compiler       ... CLANG  (command line : clang)
  Library Name     ... libopenblasp-r0.3.0.dev.a (Multi threaded; Max num-threads is 6)

[martin-frbg]

Can't we give that a rest ? I am not even sure that fabsl would be portable across all supported platforms.

[brada4]

Primary concern of unmanageable warnings is addressed on concerned platform+compiler combination as far as OpenBLAS hands could reach. I give it a rest.
Those 2 warnings are just noise. I share your doubt about fabsl()
Probably I will check around i386 as/if time permits, maybe aarch64 if i get gadget of that sort.