Added a number of different builtins that exist in the XL compiler. Most of
these builtins already exist in clang under a different name.
Details
- Reviewers
nemanjai lei rsmith - Group Reviewers
Restricted Project - Commits
- rG02cd937945f8: [PowerPC][Builtins] Added a number of builtins for compatibility with XL.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Sorry for letting this patch sit for so long.
I have updated the tests to simplify them so that we only include the
intrinsics (As per a comment). This reduces quite a bit of clutter.
Added Sema Checking for the builtins that were created.
Realized after I updated the patch that I got the darn tests wrong.
I've updated the patch and fixed that.
Can the test cases that just check for specific IR being produced be merged together? Seems unnecessary to have all of these separate test cases.
Perhaps something along the lines of:
- Test case for diagnostics of invalid use
- Test case for produced IR
- Test case for specific metadata being produced
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
9732 ↗ | (On Diff #359054) | I think this comes from another patch that is up for review. You should base this patch on top of that patch and mark the review as a dependency. It makes the review easier if the review only contains code that is meant to go in this commit. |
clang/lib/CodeGen/CGBuiltin.cpp | ||
15135–15136 | Are these two just Ops[0], Ops[1]? | |
15148–15149 | Same comment as above. | |
clang/test/CodeGen/builtins-ppc-xlcompat-expect.c | ||
2 | Should this not test for the meta data that __builtin_expect produces? |
Addressed a number of comments.
Replaced with Ops[0], Ops[1] where possible.
Rebased on top of the patch that defines SemaValueIsRunOfOnes. That patch is now
comitted anyway.
Merged a number of the test files. I was not able to merge all of the test files
as some of the builtins required Power 8 and up or Power 9 and up. Did not merge
existing files. Also did not merge the complex test file as it requires a
different check for LE, BE, and AIX.
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
9732 ↗ | (On Diff #359054) | Yes it does. That patch has actually already gone in so I've just rebased on top of it and solved this issue that way. |
clang/lib/CodeGen/CGBuiltin.cpp | ||
15135–15136 | Yes they are. I will replace where possible. |
Note that this unexpectedly broke FreeBSD's powerpc64 builds, as we've long used the following in our lib/libc/powerpc64/string/bcopy.S:
#ifndef FN_NAME #ifdef MEMMOVE #define FN_NAME __memmove WEAK_REFERENCE(__memmove, memmove); #else #define FN_NAME __bcopy WEAK_REFERENCE(__bcopy, bcopy); #endif #endif
so now we're getting:
lib/libc/powerpc64/string/bcopy.S:51:25: error: Recursive use of 'bcopy' .weak bcopy; .equ bcopy,bcopy; ^
However, I think I can make do with adding -U__bcopy to the clang command line. It would have been nice if these aliases were behind some --xl-compat flag... :)
I am really sorry that we broke you. Would it help if we defined this macro only if compiling for Linux/AIX? If so, are there any others (or perhaps all of them) that you'd like us to conditionally define only on AIX/Linux?
...
However, I think I can make do with adding -U__bcopy to the clang command line. It would have been nice if these aliases were behind some --xl-compat flag... :)
I am really sorry that we broke you. Would it help if we defined this macro only if compiling for Linux/AIX? If so, are there any others (or perhaps all of them) that you'd like us to conditionally define only on AIX/Linux?
Well, it seems that defineXLCompatMacros was rather small in the beginning (when it was introduced in rG62b5df7fe2b3fda1772befeda15598fbef96a614) so there were not so many chances for collisions. This is probably why we've not noticed these definitions, and also because they're not there for clang <= 12. :)
That said, if you think there is a use case for sometimes compiling with the XL compiler on systems *other* than Linux/AIX, then your suggested workaround might not be adequate. @jhibbits @adalava did you ever use the XL compiler on FreeBSD?
Just encountered another similar error, apparently introduced in rGc8937b6cb9751807de1e69f0f0f70a9a58f8f5dc (as more additions to the defineXLCompatMacros function, by @lei):
In file included from /home/dim/src/llvm-13-update/lib/msun/powerpc/fenv.c:32: /home/dim/src/llvm-13-update/lib/msun/powerpc/fenv.h:114:9: error: '__mtfsf' macro redefined [-Werror,-Wmacro-redefined] #define __mtfsf(__env) \ ^ <built-in>:375:9: note: previous definition is here #define __mtfsf __builtin_ppc_mtfsf ^ 1 error generated.
To unblock my builds, I'm just going to use -U to undef these macros manually for now...
The idea with putting all of these in a separate function was to:
- Make it easy to limit it to specific targets as I suggested above
- Have them all in one place to easily identify which ones are added for this compatibility so we can eventually pull this support once they are no longer needed
- Just kind of isolate this to keep it out of the way
I really think the best way forward might be to limit this to Linux and AIX. I don't think IBM provided XLC/C++ on FreeBSD.
Well, glibc also uses at least some of these __ aliases, e.g. for __bcopy:
and
but there might be more collisions...
I think it is reasonable for the GLIBC build to undef these macros in the build system. Patch to restrict this to AIX/Linux in https://reviews.llvm.org/D110213
I think you need sema checking for parm 1.