Page MenuHomePhabricator

[PowerPC] Added multiple PowerPC builtins
ClosedPublic

Authored by quinnp on May 13 2021, 2:20 PM.

Details

Reviewers
nemanjai
stefanp
Group Reviewers
Restricted Project
Commits
rG62b5df7fe2b3: [PowerPC] Added multiple PowerPC builtins
Summary

This is the first in a series of patches to provide builtins for
compatibility with the XL compiler. Most of the builtins already had
intrinsics and only needed to be implemented in the front end.
Intrinsics were created for the three iospace builtins, eieio, and icbt.
Pseudo instructions were created for eieio and iospace_eieio to
ensure that nops were inserted before the eieio instruction.

Diff Detail

Event Timeline

quinnp created this revision.May 13 2021, 2:20 PM
quinnp requested review of this revision.May 13 2021, 2:20 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 13 2021, 2:20 PM
quinnp added reviewers: nemanjai, stefanp, Restricted Project.May 13 2021, 2:22 PM
quinnp updated this revision to Diff 345510.May 14 2021, 11:20 AM

Fixed some formatting.

quinnp updated this revision to Diff 345512.May 14 2021, 11:37 AM

[PowerPC] Fixing previous update.

quinnp updated this revision to Diff 345515.May 14 2021, 11:40 AM

Removing redundant tests.

quinnp updated this revision to Diff 345517.May 14 2021, 11:49 AM

Re-added some changes that were lost in the previous updates.

quinnp updated this revision to Diff 345519.May 14 2021, 11:56 AM

Removed some unused checks in a test.

qiucf added a subscriber: qiucf.May 17 2021, 3:05 AM

Is there any motivation to add these builtins? I don't see them implemented in GCC. Besides, in Clang I think a prefix __builtin is preferred for them.

clang/include/clang/Basic/BuiltinsPPC.def
29

On PPC we have __builtin_popcount, but I only saw popcntw and popcntd generated. (I didn't verify it carefully)

In terms of the motivation for this, the description of the patch should include something along the lines of:

"This is the first in a series of patches to provide builtins for compatibility with the XL compiler."

Since these builtins don't really conform to the typical naming in clang (i.e. using the __builtin prefix), perhaps @rjmccall or @rsmith have a suggestion as to how we should proceed here.
If the names with two underscores are acceptable, great (there won't be any name clashes with existing builtins).
However if those names are not acceptable, we might be able to implement some sort of compatibility layer in Clang where the names are intercepted if compiling for a PPC target and handled in CGBuiltin.cpp.

clang/include/clang/Basic/BuiltinsPPC.def
29

There isn't really a good candidate builtin for this since this does a popcount in each byte separately. Of course, it could be used for a popcount of i8, but that is just a subset of what the builtin/instruction do.

quinnp edited the summary of this revision. (Show Details)May 17 2021, 6:48 AM

If these builtins are generally supposed to be accessed through a compiler-provided header, you could make that header define these functions using underlying builtins that have a __builtin prefix. That's not completely necessary, though; we do support some other non-library builtins that lack the __builtin prefix. And it sounds like maybe there isn't such a header anyway.

quinnp updated this revision to Diff 347029.May 21 2021, 7:52 AM

Adding a comment in BuiltinsPPC.def for motivation.

The Clang part of this looks fine to me; I can't review the backend changes, but you have partial approval.

quinnp updated this revision to Diff 347777.EditedMay 25 2021, 1:49 PM

This update is motivated by comments made by @nemanjai. All of the builtin names have been changed to follow the convention of __builtin_<arch>_<func>. To fulfill the original goal of compatibility with the XL compiler, macros have been added to map __<func> to __builtin_ppc_<func>.

Alright, that's fine with me, too.

nemanjai accepted this revision.Wed, May 26, 8:51 AM

LGTM. Thanks for implementing this. The redundant pseudo can be removed when committing.

llvm/lib/Target/PowerPC/PPCInstrInfo.td
2549

These two are identical. Please only define one.

This revision is now accepted and ready to land.Wed, May 26, 8:51 AM
quinnp updated this revision to Diff 348332.Thu, May 27, 11:04 AM

Addressing the last comments made by @nemanjai.

stefanp updated this revision to Diff 348341.Thu, May 27, 11:36 AM

Rebased revision to top of trunk.

stefanp updated this revision to Diff 348387.Thu, May 27, 2:12 PM

Updated author to Quinn.

This revision was landed with ongoing or failed builds.Thu, May 27, 2:23 PM
This revision was automatically updated to reflect the committed changes.