Fix iamax sse implementation and add utests

[marxin]

The typo in the implementation is there since the beginning (import from GotoBLAS).

[martin-frbg]

Hehe, thanks. I read the diff for the full commit three times without spotting the p-q typo in the original...

[marxin]

Hehe, thanks. I read the diff for the full commit three times without spotting the p-q typo in the original...

Yep. To be honest I found the issue while debugging in gdb. I understood the code and then I observed the suspicious comparison instruction.

[martin-frbg]

Looks as if you rattled two other cages with your new testcase - though I am not sure I understand why arm32 would fail with clang only - will try to reproduce that locally.

[marxin]

Looks as if you rattled two other cages with your new testcase - though I am not sure I understand why arm32 would fail with clang only - will try to reproduce that locally.

Apparently I had an issue in Makefile where I wrongly included '.c' and not '.o' file.
And I used VLA in C, which explains why older compilers rejected the code.

[marxin]

I've got it. There's an error in CMake configuration for /home/marxin/Programming/OpenBLAS/objdir/kernel/CMakeFiles/isamin_k.S file.

[martin-frbg]

How so ? I see it (on raspberry pi4 in 32bit mode) linking to arm/iamax_vfp.S (as defined in KERNEL.ARMV6) with USE_ABS and USE_MIN defined as it should be. All tests passed with clang7,
have not found a quick way to install clang6 as used in the CI.

[marxin]

How so ? I see it (on raspberry pi4 in 32bit mode) linking to arm/iamax_vfp.S (as defined in KERNEL.ARMV6) with USE_ABS and USE_MIN defined as it should be. All tests passed with clang7,
have not found a quick way to install clang6 as used in the CI.

Give me a minute, I've got a fix.

[martin-frbg]

Got it - the problem is in ismin, not isamin, definition. This is likely to have quite far-reaching implications, would not surprise me if it -or something like it - was behing #2396

[martin-frbg]

Can you explain this change please - why do you believe the assembly kernels were "wrongly enabled" four years ago ? If they failed the new utest - or if you benchmarked both and found the assembly inferior, this should go into a separate PR IMHO

[marxin]

Can you explain this change please - why do you believe the assembly kernels were "wrongly enabled" four years ago ? If they failed the new utest - or if you benchmarked both and found the assembly inferior, this should go into a separate PR IMHO

Ok, so I noticed the failure once I added a unit tests for this PR. Based on the results it returned value which is ought to be returned for absolute values of the input. Moreover, looking at OpenBLAS/kernel/power/isamin_power8.S, the fine contains quite some xvabssp instructions and one can't find usage of USE_ABS macro in the file. I can't read ppc64 assembly, but I'm pretty sure it gives bad values.

[martin-frbg]

Wouldn't it be using min.S rather than isamin_power8.S (which is just a precompiled version of isamin.c as a workaround for old gcc versions that cannot process the C file) ? (The same default would apply to other targets where the KERNEL.target does not specify SMINKERNEL.) Anyway my main point was that I would like to avoid having tangentially related changes in a PR that does not mention them.)

[marxin]

Wouldn't it be using min.S rather than isamin_power8.S (which is just a precompiled version of isamin.c as a workaround for old gcc versions that cannot process the C file)?

I don't know how isamin_power8.S is created. But one can see that ISMINKERNEL is implemented multiple times with arm/imin.c:

kernel/arm/KERNEL.ARMV5:ISMINKERNEL  = ../arm/imin.c
kernel/arm64/KERNEL.ARMV8:ISMINKERNEL  = ../arm/imin.c
kernel/arm64/KERNEL.CORTEXA57:ISMINKERNEL  = ../arm/imin.c
kernel/arm64/KERNEL.THUNDERX:ISMINKERNEL  = ../arm/imin.c
kernel/arm64/KERNEL.THUNDERX2T99:ISMINKERNEL  = ../arm/imin.c
kernel/arm64/KERNEL.TSV110:ISMINKERNEL  = ../arm/imin.c
kernel/power/KERNEL.POWER8:#ISMINKERNEL  = ../arm/imin.c
kernel/power/KERNEL.POWER9:#ISMINKERNEL  = ../arm/imin.c
kernel/x86/KERNEL.generic:ISMINKERNEL  = ../arm/imin.c
kernel/x86_64/KERNEL.generic:ISMINKERNEL  = ../arm/imin.c
kernel/zarch/KERNEL.Z13:ISMINKERNEL  = ../arm/imin.c
kernel/zarch/KERNEL.ZARCH_GENERIC:ISMINKERNEL  = ../arm/imin.c

which I believe is a generic implementation.

(The same default would apply to other targets where the KERNEL.target does not specify SMINKERNEL.) Anyway my main point was that I would like to avoid having tangentially related changes in a PR that does not mention them.)

Sure. I can remove the commit that's related to POWER target. Will you merge this if it has a new failing unit test that fails on POWER?

[martin-frbg]

Certainly - we know that the failure is not actually caused by this PR, and I am trying to track down and fix the POWER issue already. While arm/imin.c and arm/imax.c are indeed the generic implementations used across several architectures, in the event that the KERNEL.target lacks a definition there will be a fallback taken from the respective Makefile.Lx. So it could be that more than just POWER8 is affected. (Actually imin.S/imax.S look as if it should work, they combine a subtraction of two candidate values with a selection of one or the other depending on whether the result of the subtraction is less than zero. However I see now that Makefile.L1 actually defines iAmin.S/iAmax.S as the fallback, which does not feel correct...)

[marxin]

Good. I've just removed the POWER related change. I'm leaving that to you..