Openblas: openblas fails to compile on ppc64le-linux using gcc-7

[amodra]

From https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79343
kernel/power/sasum_microk_power8.c gives error: PIC register clobbered by ‘%2’ in ‘asm’

The blas source looks to be adding r2 (and r0) to the clobbers completely unnecessarily. I suspect what the author of that code was trying to do was say that i and n are changed by the asm, which is done by making them output args as well as input. The memory clobber could be removed too, if *x1 was made an output. Oh, and the asm misses clobbers for all the vector regs used..

[martin-frbg]

Similar reservations were noted in the mailing list discussion following (and linked from) #795. @wernsaar ?

[brada4]

Does it compile with current GCC release 6.3? GCC7 is development branch and expected to carry regressions.

[amodra]

So the first comment was wrong about *x1. I meant *svec, the output. Patch posted here:
https://groups.google.com/forum/#!topic/openblas-dev/YI1of4HODmw
Not tested apart from simply compiling. I'm going to leave testing to someone else.

Yes, the current source might compile with gcc-6 since gcc-6 hasn't switched over to lra by default for ppc, but the asm is horribly broken.

[martin-frbg]

"broken" in the sense that the current code gives wrong results, or just that it is written with (I guess) a strong x86 accent ?

[amodra]

On Fri, Feb 03, 2017 at 01:50:40AM -0800, Martin Kroeker wrote:

"broken" in the sense that the current code gives wrong results, or just that it is written with (I guess) a strong x86 accent ?

The current code trashes callee saved regs fr14 to fr23. A function
that accidentally modifies the variables of its callers (and not just
the immediate caller) is horribly broken.

--
Alan Modra
Australia Development Lab, IBM

[edelsohn]

@brada4 @martin-frbg The OpenBLAS VSX code is wrong and it is not a GCC 7 regression.

[martin-frbg]

Thank you both for the clarification. Given this situation I am inclined to merge the patch as-is although I am mostly illiterate when it comes to assembly and I have no ability to test it.

[martin-frbg]

At least 24 other files appear to be equally affected, and I do not feel confident to apply these corrections even to closely similar files like dasum_microk_power8.c, sorry.

[besser82]

Commenting here for being in the loop… See: https://bugzilla.redhat.com/show_bug.cgi?id=1417385

[amodra]

On Fri, Feb 03, 2017 at 02:23:35PM -0800, Martin Kroeker wrote:

At least 24 other files appear to be equally affected

Indeed. What a mess! I'll take a look at fixing them all.

--
Alan Modra
Australia Development Lab, IBM

[amodra]

For the record, it's more than just the gcc asm() statements. The assembly files are broken too, I think, using callee-saved altivec regs without saving them.
Oh, and the patch I posted wasn't correct either. Oops. The constraints were not quite right, and I should have put vs32 etc. in the asm clobbers, not plain 32 etc. In sasum_microk_power8.c fr14..fr23 were not being trashed but some altivec regs, v20, v21, v22 and v23.
In a lot of cases by using the right vsx registers we probably don't need to touch callee-saves. There are 34 vsx regs that don't need to be saved, vs0-vs13 (corresponding to fr0-fr13),and vs32-vs51 (corresponding to v0-v19).

[edelsohn]

Thanks for grabbing this, Alan. Getting inline asm correct is tricky.

[grisuthedragon]

@brada4 I tried it on pp64le with GCC 6.3 and it works except of the #1071 bug.

[martin-frbg]

@grisuthedragon as I understood amodra, the code delivers the expected result (else I am sure wernsaar would not have committed it) but may trash unrelated bits in the process. (The difference between GCC6 and GCC7 in this context appears to be simply that GCC6 was not yet able to diagnose such errors in inline ppc assembly)
(Is there some whitepaper on power8 assembly that explains how to code for this processor so "we" can avoid making the same mistakes in the future ?)

[brada4]

@grisuthedragon Thanks for confirming, maybe this fixed will silently fix other...

[amodra]

On Tue, Feb 07, 2017 at 12:10:23AM -0800, Martin Kroeker wrote:

(Is there some whitepaper on power8 assembly that explains how to code for this processor so "we" can avoid making the same mistakes in the future ?)

I'm not aware of any such paper, but the issues in the asm are mostly
not specific to powerpc. Asm constraints are explained in the gcc
info doc.

Here are a few examples. Compare the following new implementation for
kernel/power/dgemv_n_microk_power8.c against the old. (I've changed
the parameters so that a0..a3 and alpha are not needlessly passed in
memory. The new asm calculates a0..a3 from ap and lda, and splats
alpha rather than that being done in C.)

static void dgemv_kernel_4x4 (long n, double *ap, long lda, double *x, double *y, double alpha)
{
double *a0;
double *a1;
double *a2;
double *a3;

__asm__
(
...
:
"=m" (y),
"+r" (n), // 1
"+b" (y), // 2
"=b" (a0), // 3
"=b" (a1), // 4
"=&b" (a2), // 5
"=&b" (a3) // 6
:
"m" (x),
"d" (alpha), // 8
"r" (x), // 9
"b" (16), // 10
"3" (ap), // 11
"4" (lda) // 12
:
"cr0",
"vs32","vs33","vs34","vs35","vs36","vs37",
"vs40","vs41","vs42","vs43","vs44","vs45","vs46","vs47"
);
}

Use "+r" (n) to say n is both an output as well as input, ie. in
this case since n isn't used outside of the asm, an input that is
modified.

Use "=m" (y) and "m" (x) to specify the memory written and read
by the function. Much better than a "memory" clobber since it tells
gcc the actual memory touched, and allows eg. gcc to delete the asm if
its output is unused.

Specify that a0..3 are needed temp regs by making them outputs, using
the same reg as ap and lda inputs in the case of a0 and a1, and using
and early clobber on a2 and a3 to say those outputs can't share the
same reg as any of the inputs. It's a pain that gcc doesn't allow
more than 30 asm inputs and outputs since if it did allow more I/Os
you could do the same sort of thing for the vsx temp regs. That would
allow gcc complete freedom to allocate vsx regs, which might be better
for inlining.

Notice that x and y both appear twice (but in fact use the same reg.
ie. the "=m" (*y) is 0(r7) and "+b" (y) is r7). It's a bit of a pain
that you need to do this sort of trick, but powerpc gcc lacks
modifiers to ask for just the base reg to be output for the "m"
constraint (or "es", "Z" constraints). So that is a powerpc trick
that you might not need on other processors, but handy to know for
generic use. Since the asm doesn't make use of %0 or %7 it doesn't
even matter if gcc allocates different regs or even a different form
of address for the "m" I/Os.

Notice also that I've used "b" rather than "r" in most cases. That is
to exclude r0, since some insns in the asm interpret an r0 encoding in
certain fields to mean zero rather than the register. eg. addi 0,0,4
is r0=4, not r0=r0+4.

I'd post the whole patch but right now I have some testsuite errors to
contend with. Hopefully just a typo somewhere.

--
Alan Modra
Australia Development Lab, IBM

[amodra]

Patch posted to https://groups.google.com/forum/#!topic/openblas-dev/YI1of4HODmw

[martin-frbg]

Thank you.I will convert it to a "pull request" ASAP.
(Pity about the remaining .S files being at least partly incorrect as well, but I guess it was to be expected given the common authorship)

[besser82]

@amodra Thank you very much! =)

[martin-frbg]

Unfortunately the "vs" registers on the clobber lists appear to cause compatibility problems with both clang(#1120) and xlc (#1248) - "unknown register name 'vs32' in asm" etc. I was ready to accept that the LLVM version may have simply been too old to be compatible, but I find it quite disconcerting that the same error comes up with the current "Community Edition" of XLC 13.1.5. Thoughts ?

[edelsohn]

The "vs" names are additional, alternate register names that overlay the FPRs and VRs. vs32-vs63 is equivalent to fr0-fr31, vs32-vs64 is equivalent to v0-v31.

[martin-frbg]

Thx. Actually it had occured to me in the meantime that I had just been using "v20" et al. myself in the register saving code I copied from the PowerISA book... perhaps I should actually read that document in full. :-/
Next up however is "the asm operand does not match the specified constraint" for
"=wa" (t0) in the same file (sasum_microk_power8.c) so there seems to be more gasm idiom here that "something in xlc" does not like.

[edelsohn]

asm constraints should not be gated on XLC compatibility

[martin-frbg]

"=wa" appears to specify "a vsx register", t0 is declared as a "__vector float" (and is identified with "%x3" in a comment above the problematic line). Trivial substitutions like "v3" for t0 or "wf" for wa based on reading the "asm constraints" section of the gcc docs only lead to different errors at best.

[martin-frbg]

I can get it to compile if I use "=v" (Altivec vector register) in place of "=wa" - not sure if I am just missing a compiler option (although I already have -qarch=pwr8 -mcpu=power8 -qaltivec -q64 -qsimd=auto).

[edelsohn]

Why are you focusing on XLC? XLC is far out of date with respect to correct register constraints.

[martin-frbg]

Just for the sake of completeness (and to get rid of issue #1248). If this is "expected to fail" I'll gladly add a note in the wiki and leave it at that.

[edelsohn]

Please don't change to code to satisfy XLC. This is an XLC bug, not a bug in OpenBLAS. It would be nice for OpenBLAS to build and run with XLC, but that apparently requires a fix to XLC. The person who reported issue #1248 should report the problem to IBM as an XLC bug.

[martin-frbg]

Closing here as the (then) remaining code problems are addressed in #1263

[trudeaun]

Just to follow up: XLC uses clang technology, and so far, "vs" register names are unsupported by clang. As a workaround, you can use v0, …, v7, instead of "vs32", …, "vs39". This came up again recently in https://github.com/xianyi/OpenBLAS/issues/1699.