This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][Builtins] Added a number of builtins for compatibility with XL.
ClosedPublic

Authored by stefanp on Jun 16 2021, 7:53 AM.

Details

Summary

Added a number of different builtins that exist in the XL compiler. Most of
these builtins already exist in clang under a different name.

Diff Detail

Event Timeline

stefanp created this revision.Jun 16 2021, 7:53 AM
stefanp requested review of this revision.Jun 16 2021, 7:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2021, 7:53 AM
stefanp added reviewers: rsmith, Restricted Project.Jun 16 2021, 7:55 AM
qiucf added a subscriber: qiucf.Jun 16 2021, 11:55 PM
qiucf added inline comments.
clang/test/CodeGen/builtins-ppc-xlcompat-rotate.c
56

For these tests against builtin mapping to an intrinsic, could we only keep the call line to make it simpler? (as D103986 does)

lei added inline comments.Jul 14 2021, 6:40 AM
clang/include/clang/Basic/BuiltinsPPC.def
50

I think you need sema checking for parm 1.

alignment
Must be a constant integer with a value greater than zero and of a power of two.
51

sema checking?

stefanp updated this revision to Diff 358915.Jul 15 2021, 4:04 AM

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.

stefanp updated this revision to Diff 359054.Jul 15 2021, 11:00 AM

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
15298–15299

Are these two just Ops[0], Ops[1]?

15311–15312

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?

stefanp updated this revision to Diff 359459.Jul 16 2021, 2:46 PM

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.

stefanp marked an inline comment as not done.Jul 16 2021, 2:48 PM
stefanp added inline comments.
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
15298–15299

Yes they are. I will replace where possible.

nemanjai accepted this revision.Jul 19 2021, 9:18 AM

LGTM. Thanks.

This revision is now accepted and ready to land.Jul 19 2021, 9:18 AM
This revision was landed with ongoing or failed builds.Jul 20 2021, 6:58 AM
This revision was automatically updated to reflect the committed changes.
dim added subscribers: emaste, dim.Aug 30 2021, 1:59 PM

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... :)

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?

Note that this unexpectedly broke FreeBSD's powerpc64 builds, as we've long used the following in our lib/libc/powerpc64/string/bcopy.S:

...

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?

dim added a comment.Aug 31 2021, 3:08 PM

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:

  1. Make it easy to limit it to specific targets as I suggested above
  2. 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
  3. 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.

dim added a comment.Sep 2 2021, 8:10 AM

The idea with putting all of these in a separate function was to:

  1. Make it easy to limit it to specific targets as I suggested above
  2. 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
  3. 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:

https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/powerpc/powerpc64/power7/memmove.S;h=f61949d30fa317ec487deb81b20e67ed3df05e32;hb=HEAD#l829

and

https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/powerpc/powerpc64/le/power10/memmove.S;h=7dfd57edeb37e8e47a31fe6e19f254bc1fcd312b;hb=HEAD#l312

but there might be more collisions...

The idea with putting all of these in a separate function was to:

  1. Make it easy to limit it to specific targets as I suggested above
  2. 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
  3. 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:

https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/powerpc/powerpc64/power7/memmove.S;h=f61949d30fa317ec487deb81b20e67ed3df05e32;hb=HEAD#l829

and

https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/powerpc/powerpc64/le/power10/memmove.S;h=7dfd57edeb37e8e47a31fe6e19f254bc1fcd312b;hb=HEAD#l312

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