-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add workaround for older gcc on big-endian ppc64 not supporting casts in defines #3148
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change appears to break building on powerpc and powerpc64 on FreeBSD with clang 11, with the exact same error that gcc produces, but in reverse, using "LONGCAST" in place of "(BLASLONG)" in the error.
Oops.Does it build when you throw out this change (or put |
Looks like something else is needed. I get the original error reported for this when I revert this change. |
Oops, I need to correct my original statement... this is getting built with gcc10, not with clang. Would help if I looked closer at the build logs... |
strange - gcc10 was what I had been using at the time of #3145, and the fix was only to make it compile with gcc8 and earlier. (By turning the added cast into an empty string). Are you sure your build is with actual gcc10 and not some version of clang masquerading as gcc ? |
...wonder why would there be a verbatim |
Ah, looking closely, this is all happening in the C preprocessor, where casts are not allowed, to the best of my knowledge, since the preprocessor knows nothing about types. After setting up a ports build jail, it looks like the TARGET=PPC970 we have by default is a problem. Has this been tested recently with that target type? It works fine with TARGET=POWER8 |
I see the problem now. SGEMM_DEFAULT_R does not exist for PPC970, so it uses the default, which uses GEMM_DEFAULT_ALIGN. Because GEMM_DEFAULT_ALIGN is defined as '(BLASLONG)0x03fffUL', which includes a cast, this fails in the preprocessor. The solution appears to be to either remove the cast here, or add a definition of SGEMM_DEFAULT_R for PPC970. Same goes for PPCG4 as well. Probably the same thing for the other GEMM_DEFAULT_ defines as well. Since I know nothing about the workings of OpenBLAS, I cannot create the right definitions myself, aside from working through the define expansion from common_param.h. |
Thanks for digging through this, I'll try to sort it tomorrow. |
Seems some of the early POWER targets are also missing DEFAULT_P and Q parameters, for which no calculation macro exists in common_param.h, so these have not been compilable even in GotoBLAS2. (I have no idea how common POWER3,4 or 5 are/were in their time however) |
I have certainly built OpenBLAS before for PPCG4 and PPC970. FreeBSD also has a package for powerpc64 targeting PPC970 (version 0.3.13). |
The problematic casts were introduced by #3088 shortly before 0.3.14 (git history shows them as committed in 2019, but @xoviat must have had them lingering in a private branch since then), I'm now inclined to simply remove them for PPC (and possibly POWER as well) The constant has "always" been an UL, and the casts were added across the range of architectures although the cited overflows may have been seen on some subset only - as the branch seems to have been labeled "msvc" it may have been an x86/Windows/MSVC warning that triggered them. |
That seems to me to be the most logical thing. |
for #3145